It was added by commit 137e5531351d ("vfio/pci: Add sriov_configure
support") and actually unnecessary. Remove it.
Signed-off-by: Zenghui Yu <[email protected]>
---
drivers/vfio/pci/vfio_pci.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 1ab1f5cda4ac..da68e2f86622 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1862,7 +1862,6 @@ static const struct vfio_device_ops vfio_pci_ops = {
static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
-static struct pci_driver vfio_pci_driver;
static int vfio_pci_bus_notifier(struct notifier_block *nb,
unsigned long action, void *data)
--
2.19.1
It isn't clear what purpose the @bardirty serves. It might be used to avoid
the unnecessary vfio_bar_fixup() invoking on a user-space BAR read, which
is not required when bardirty is unset.
The variable was introduced in commit 89e1f7d4c66d ("vfio: Add PCI device
driver") but never actually used, so it shouldn't be that important. Remove
it.
Signed-off-by: Zenghui Yu <[email protected]>
---
drivers/vfio/pci/vfio_pci_config.c | 7 -------
drivers/vfio/pci/vfio_pci_private.h | 1 -
2 files changed, 8 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index d98843feddce..e93b287fea02 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -507,8 +507,6 @@ static void vfio_bar_fixup(struct vfio_pci_device *vdev)
*vbar &= cpu_to_le32((u32)mask);
} else
*vbar = 0;
-
- vdev->bardirty = false;
}
static int vfio_basic_config_read(struct vfio_pci_device *vdev, int pos,
@@ -633,9 +631,6 @@ static int vfio_basic_config_write(struct vfio_pci_device *vdev, int pos,
}
}
- if (is_bar(offset))
- vdev->bardirty = true;
-
return count;
}
@@ -1697,8 +1692,6 @@ int vfio_config_init(struct vfio_pci_device *vdev)
if (ret)
goto out;
- vdev->bardirty = true;
-
/*
* XXX can we just pci_load_saved_state/pci_restore_state?
* may need to rebuild vconfig after that
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 61ca8ab165dc..dc96a0fda548 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -122,7 +122,6 @@ struct vfio_pci_device {
bool virq_disabled;
bool reset_works;
bool extended_caps;
- bool bardirty;
bool has_vga;
bool needs_reset;
bool nointx;
--
2.19.1
On Thu, 17 Sep 2020 11:31:27 +0800
Zenghui Yu <[email protected]> wrote:
> It was added by commit 137e5531351d ("vfio/pci: Add sriov_configure
> support") and actually unnecessary. Remove it.
>
> Signed-off-by: Zenghui Yu <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 1ab1f5cda4ac..da68e2f86622 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1862,7 +1862,6 @@ static const struct vfio_device_ops vfio_pci_ops = {
>
> static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
> static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
> -static struct pci_driver vfio_pci_driver;
>
> static int vfio_pci_bus_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
Reviewed-by: Cornelia Huck <[email protected]>
On Thu, 17 Sep 2020 11:31:28 +0800
Zenghui Yu <[email protected]> wrote:
> It isn't clear what purpose the @bardirty serves. It might be used to avoid
> the unnecessary vfio_bar_fixup() invoking on a user-space BAR read, which
> is not required when bardirty is unset.
>
> The variable was introduced in commit 89e1f7d4c66d ("vfio: Add PCI device
> driver") but never actually used, so it shouldn't be that important. Remove
> it.
>
> Signed-off-by: Zenghui Yu <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci_config.c | 7 -------
> drivers/vfio/pci/vfio_pci_private.h | 1 -
> 2 files changed, 8 deletions(-)
Yes, it seems to have been write-only all the time.
Reviewed-by: Cornelia Huck <[email protected]>
On Thu, 17 Sep 2020 13:35:37 +0200
Cornelia Huck <[email protected]> wrote:
> On Thu, 17 Sep 2020 11:31:28 +0800
> Zenghui Yu <[email protected]> wrote:
>
> > It isn't clear what purpose the @bardirty serves. It might be used to avoid
> > the unnecessary vfio_bar_fixup() invoking on a user-space BAR read, which
> > is not required when bardirty is unset.
> >
> > The variable was introduced in commit 89e1f7d4c66d ("vfio: Add PCI device
> > driver") but never actually used, so it shouldn't be that important. Remove
> > it.
> >
> > Signed-off-by: Zenghui Yu <[email protected]>
> > ---
> > drivers/vfio/pci/vfio_pci_config.c | 7 -------
> > drivers/vfio/pci/vfio_pci_private.h | 1 -
> > 2 files changed, 8 deletions(-)
>
> Yes, it seems to have been write-only all the time.
I suspect the intent was that vfio_bar_fixup() could test
vdev->bardirty to avoid doing work if no BARs had been written since
they were last read. As it is now we regenerate vconfig for all the
BARs every time any offset of any of them are read. BARs aren't
re-read regularly and config space is not a performance path, but maybe
we should instead test if we see any regressions from returning without
doing any work in vfio_bar_fixup() if vdev->bardirty is false. Thanks,
Alex
On Thu, 17 Sep 2020 11:31:27 +0800
Zenghui Yu <[email protected]> wrote:
> It was added by commit 137e5531351d ("vfio/pci: Add sriov_configure
> support") and actually unnecessary. Remove it.
Looks correct, but I might clarify as:
s/unnecessary/duplicates a forward declaration earlier in the file/
I can change on commit if you approve. Thanks,
Alex
> Signed-off-by: Zenghui Yu <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 1ab1f5cda4ac..da68e2f86622 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1862,7 +1862,6 @@ static const struct vfio_device_ops vfio_pci_ops = {
>
> static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
> static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
> -static struct pci_driver vfio_pci_driver;
>
> static int vfio_pci_bus_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
Hi Alex,
On 2020/9/18 6:07, Alex Williamson wrote:
> On Thu, 17 Sep 2020 13:35:37 +0200
> Cornelia Huck <[email protected]> wrote:
>
>> On Thu, 17 Sep 2020 11:31:28 +0800
>> Zenghui Yu <[email protected]> wrote:
>>
>>> It isn't clear what purpose the @bardirty serves. It might be used to avoid
>>> the unnecessary vfio_bar_fixup() invoking on a user-space BAR read, which
>>> is not required when bardirty is unset.
>>>
>>> The variable was introduced in commit 89e1f7d4c66d ("vfio: Add PCI device
>>> driver") but never actually used, so it shouldn't be that important. Remove
>>> it.
>>>
>>> Signed-off-by: Zenghui Yu <[email protected]>
>>> ---
>>> drivers/vfio/pci/vfio_pci_config.c | 7 -------
>>> drivers/vfio/pci/vfio_pci_private.h | 1 -
>>> 2 files changed, 8 deletions(-)
>>
>> Yes, it seems to have been write-only all the time.
>
> I suspect the intent was that vfio_bar_fixup() could test
> vdev->bardirty to avoid doing work if no BARs had been written since
> they were last read. As it is now we regenerate vconfig for all the
> BARs every time any offset of any of them are read. BARs aren't
> re-read regularly and config space is not a performance path,
Yes, it seems that Qemu itself emulates all BAR registers and will read
the BAR from the kernel side only at initialization time.
> but maybe
> we should instead test if we see any regressions from returning without
> doing any work in vfio_bar_fixup() if vdev->bardirty is false. Thanks,
I will test it with the following diff. Please let me know which way do
you prefer.
diff --git a/drivers/vfio/pci/vfio_pci_config.c
b/drivers/vfio/pci/vfio_pci_config.c
index d98843feddce..77c419d536d0 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -515,7 +515,7 @@ static int vfio_basic_config_read(struct
vfio_pci_device *vdev, int pos,
int count, struct perm_bits *perm,
int offset, __le32 *val)
{
- if (is_bar(offset)) /* pos == offset for basic config */
+ if (is_bar(offset) && vdev->bardirty) /* pos == offset for basic
config */
vfio_bar_fixup(vdev);
count = vfio_default_config_read(vdev, pos, count, perm,
offset, val);
Thanks,
Zenghui
On 2020/9/18 6:22, Alex Williamson wrote:
> On Thu, 17 Sep 2020 11:31:27 +0800
> Zenghui Yu <[email protected]> wrote:
>
>> It was added by commit 137e5531351d ("vfio/pci: Add sriov_configure
>> support") and actually unnecessary. Remove it.
>
> Looks correct, but I might clarify as:
>
> s/unnecessary/duplicates a forward declaration earlier in the file/
>
> I can change on commit if you approve. Thanks,
Indeed. Please help to change it.
Thanks,
Zenghui
On Sat, 19 Sep 2020 09:54:00 +0800
Zenghui Yu <[email protected]> wrote:
> Hi Alex,
>
> On 2020/9/18 6:07, Alex Williamson wrote:
> > On Thu, 17 Sep 2020 13:35:37 +0200
> > Cornelia Huck <[email protected]> wrote:
> >
> >> On Thu, 17 Sep 2020 11:31:28 +0800
> >> Zenghui Yu <[email protected]> wrote:
> >>
> >>> It isn't clear what purpose the @bardirty serves. It might be used to avoid
> >>> the unnecessary vfio_bar_fixup() invoking on a user-space BAR read, which
> >>> is not required when bardirty is unset.
> >>>
> >>> The variable was introduced in commit 89e1f7d4c66d ("vfio: Add PCI device
> >>> driver") but never actually used, so it shouldn't be that important. Remove
> >>> it.
> >>>
> >>> Signed-off-by: Zenghui Yu <[email protected]>
> >>> ---
> >>> drivers/vfio/pci/vfio_pci_config.c | 7 -------
> >>> drivers/vfio/pci/vfio_pci_private.h | 1 -
> >>> 2 files changed, 8 deletions(-)
> >>
> >> Yes, it seems to have been write-only all the time.
> >
> > I suspect the intent was that vfio_bar_fixup() could test
> > vdev->bardirty to avoid doing work if no BARs had been written since
> > they were last read. As it is now we regenerate vconfig for all the
> > BARs every time any offset of any of them are read. BARs aren't
> > re-read regularly and config space is not a performance path,
>
> Yes, it seems that Qemu itself emulates all BAR registers and will read
> the BAR from the kernel side only at initialization time.
>
> > but maybe
> > we should instead test if we see any regressions from returning without
> > doing any work in vfio_bar_fixup() if vdev->bardirty is false. Thanks,
>
> I will test it with the following diff. Please let me know which way do
> you prefer.
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c
> b/drivers/vfio/pci/vfio_pci_config.c
> index d98843feddce..77c419d536d0 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -515,7 +515,7 @@ static int vfio_basic_config_read(struct
> vfio_pci_device *vdev, int pos,
> int count, struct perm_bits *perm,
> int offset, __le32 *val)
> {
> - if (is_bar(offset)) /* pos == offset for basic config */
> + if (is_bar(offset) && vdev->bardirty) /* pos == offset for basic
> config */
> vfio_bar_fixup(vdev);
>
> count = vfio_default_config_read(vdev, pos, count, perm,
> offset, val);
There's only one caller currently, but I'd think it cleaner to put this
in vfio_bar_fixup(), ie. return immediately if !bardirty. Thanks,
Alex
On 2020/9/19 10:11, Alex Williamson wrote:
> On Sat, 19 Sep 2020 09:54:00 +0800
> Zenghui Yu <[email protected]> wrote:
>
>> Hi Alex,
>>
>> On 2020/9/18 6:07, Alex Williamson wrote:
>>> On Thu, 17 Sep 2020 13:35:37 +0200
>>> Cornelia Huck <[email protected]> wrote:
>>>
>>>> On Thu, 17 Sep 2020 11:31:28 +0800
>>>> Zenghui Yu <[email protected]> wrote:
>>>>
>>>>> It isn't clear what purpose the @bardirty serves. It might be used to avoid
>>>>> the unnecessary vfio_bar_fixup() invoking on a user-space BAR read, which
>>>>> is not required when bardirty is unset.
>>>>>
>>>>> The variable was introduced in commit 89e1f7d4c66d ("vfio: Add PCI device
>>>>> driver") but never actually used, so it shouldn't be that important. Remove
>>>>> it.
>>>>>
>>>>> Signed-off-by: Zenghui Yu <[email protected]>
>>>>> ---
>>>>> drivers/vfio/pci/vfio_pci_config.c | 7 -------
>>>>> drivers/vfio/pci/vfio_pci_private.h | 1 -
>>>>> 2 files changed, 8 deletions(-)
>>>>
>>>> Yes, it seems to have been write-only all the time.
>>>
>>> I suspect the intent was that vfio_bar_fixup() could test
>>> vdev->bardirty to avoid doing work if no BARs had been written since
>>> they were last read. As it is now we regenerate vconfig for all the
>>> BARs every time any offset of any of them are read. BARs aren't
>>> re-read regularly and config space is not a performance path,
>>
>> Yes, it seems that Qemu itself emulates all BAR registers and will read
>> the BAR from the kernel side only at initialization time.
>>
>>> but maybe
>>> we should instead test if we see any regressions from returning without
>>> doing any work in vfio_bar_fixup() if vdev->bardirty is false. Thanks,
>>
>> I will test it with the following diff. Please let me know which way do
>> you prefer.
>>
>> diff --git a/drivers/vfio/pci/vfio_pci_config.c
>> b/drivers/vfio/pci/vfio_pci_config.c
>> index d98843feddce..77c419d536d0 100644
>> --- a/drivers/vfio/pci/vfio_pci_config.c
>> +++ b/drivers/vfio/pci/vfio_pci_config.c
>> @@ -515,7 +515,7 @@ static int vfio_basic_config_read(struct
>> vfio_pci_device *vdev, int pos,
>> int count, struct perm_bits *perm,
>> int offset, __le32 *val)
>> {
>> - if (is_bar(offset)) /* pos == offset for basic config */
>> + if (is_bar(offset) && vdev->bardirty) /* pos == offset for basic
>> config */
>> vfio_bar_fixup(vdev);
>>
>> count = vfio_default_config_read(vdev, pos, count, perm,
>> offset, val);
>
>
> There's only one caller currently, but I'd think it cleaner to put this
> in vfio_bar_fixup(), ie. return immediately if !bardirty. Thanks,
OK, I'll do that in the v2.
Thanks,
Zenghui