[PATCH 1/2] vfio/pci: Remove redundant declaration of vfio_pci_driver

[PATCH 1/2] vfio/pci: Remove redundant declaration of vfio_pci_driver

[Zenghui Yu]

It was added by commit 137e5531351d ("vfio/pci: Add sriov_configure
support") and actually unnecessary. Remove it.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 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

[PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device

[Zenghui Yu]

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 <yuzenghui@huawei.com>
---
 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

Re: [PATCH 1/2] vfio/pci: Remove redundant declaration of vfio_pci_driver

[Cornelia Huck]

On Thu, 17 Sep 2020 11:31:27 +0800
Zenghui Yu <yuzenghui@huawei.com> wrote:

> It was added by commit 137e5531351d ("vfio/pci: Add sriov_configure
> support") and actually unnecessary. Remove it.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  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 <cohuck@redhat.com>

Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device

[Cornelia Huck]

On Thu, 17 Sep 2020 11:31:28 +0800
Zenghui Yu <yuzenghui@huawei.com> 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 <yuzenghui@huawei.com>
> ---
>  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 <cohuck@redhat.com>

Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device

[Alex Williamson]

On Thu, 17 Sep 2020 13:35:37 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 17 Sep 2020 11:31:28 +0800
> Zenghui Yu <yuzenghui@huawei.com> 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 <yuzenghui@huawei.com>
> > ---
> >  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

Re: [PATCH 1/2] vfio/pci: Remove redundant declaration of vfio_pci_driver

[Alex Williamson]

On Thu, 17 Sep 2020 11:31:27 +0800
Zenghui Yu <yuzenghui@huawei.com> 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 <yuzenghui@huawei.com>
> ---
>  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)

Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device

[Zenghui Yu]

Hi Alex,

On 2020/9/18 6:07, Alex Williamson wrote:
> On Thu, 17 Sep 2020 13:35:37 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 17 Sep 2020 11:31:28 +0800
>> Zenghui Yu <yuzenghui@huawei.com> 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 <yuzenghui@huawei.com>
>>> ---
>>>   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

Re: [PATCH 1/2] vfio/pci: Remove redundant declaration of vfio_pci_driver

[Zenghui Yu]

On 2020/9/18 6:22, Alex Williamson wrote:
> On Thu, 17 Sep 2020 11:31:27 +0800
> Zenghui Yu <yuzenghui@huawei.com> 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

Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device

[Alex Williamson]

On Sat, 19 Sep 2020 09:54:00 +0800
Zenghui Yu <yuzenghui@huawei.com> wrote:

> Hi Alex,
> 
> On 2020/9/18 6:07, Alex Williamson wrote:
> > On Thu, 17 Sep 2020 13:35:37 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> >> On Thu, 17 Sep 2020 11:31:28 +0800
> >> Zenghui Yu <yuzenghui@huawei.com> 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 <yuzenghui@huawei.com>
> >>> ---
> >>>   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

Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device

[Zenghui Yu]

On 2020/9/19 10:11, Alex Williamson wrote:
> On Sat, 19 Sep 2020 09:54:00 +0800
> Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
>> Hi Alex,
>>
>> On 2020/9/18 6:07, Alex Williamson wrote:
>>> On Thu, 17 Sep 2020 13:35:37 +0200
>>> Cornelia Huck <cohuck@redhat.com> wrote:
>>>    
>>>> On Thu, 17 Sep 2020 11:31:28 +0800
>>>> Zenghui Yu <yuzenghui@huawei.com> 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 <yuzenghui@huawei.com>
>>>>> ---
>>>>>    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