Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device
On 2020/9/19 10:11, Alex Williamson wrote:
On Sat, 19 Sep 2020 09:54:00 +0800
Zenghui Yu wrote:
Hi Alex,
On 2020/9/18 6:07, Alex Williamson wrote:
On Thu, 17 Sep 2020 13:35:37 +0200
Cornelia Huck wrote:
On Thu, 17 Sep 2020 11:31:28 +0800
Zenghui Yu 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
---
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
Re: [PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device
On Sat, 19 Sep 2020 09:54:00 +0800
Zenghui Yu wrote:
> Hi Alex,
>
> On 2020/9/18 6:07, Alex Williamson wrote:
> > On Thu, 17 Sep 2020 13:35:37 +0200
> > Cornelia Huck wrote:
> >
> >> On Thu, 17 Sep 2020 11:31:28 +0800
> >> Zenghui Yu 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
> >>> ---
> >>> 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
Hi Alex,
On 2020/9/18 6:07, Alex Williamson wrote:
On Thu, 17 Sep 2020 13:35:37 +0200
Cornelia Huck wrote:
On Thu, 17 Sep 2020 11:31:28 +0800
Zenghui Yu 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
---
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 2/2] vfio/pci: Remove bardirty from vfio_pci_device
On Thu, 17 Sep 2020 13:35:37 +0200
Cornelia Huck wrote:
> On Thu, 17 Sep 2020 11:31:28 +0800
> Zenghui Yu 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
> > ---
> > 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 2/2] vfio/pci: Remove bardirty from vfio_pci_device
On Thu, 17 Sep 2020 11:31:28 +0800
Zenghui Yu 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
> ---
> 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
[PATCH 2/2] vfio/pci: Remove bardirty from vfio_pci_device
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
---
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 {
boolvirq_disabled;
boolreset_works;
boolextended_caps;
- boolbardirty;
boolhas_vga;
boolneeds_reset;
boolnointx;
--
2.19.1
