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