[Qemu-devel] [PATCH 0/2] vfio/pci: RTL8168 quirk fix & cleanup

[Qemu-devel] [PATCH 0/2] vfio/pci: RTL8168 quirk fix & cleanup

[Alex Williamson]

A small patch for stable plus a bigger patch to make tracing of this
quirk useful and restructure and comment the read/write accessors.
Thanks,

Alex

---

Alex Williamson (2):
      vfio/pci: Fix RTL8168 NIC quirks
      vfio/pci: Cleanup RTL8168 quirk and tracing


 hw/vfio/pci.c |   90 +++++++++++++++++++++++----------------------------------
 trace-events  |   10 +++---
 2 files changed, 41 insertions(+), 59 deletions(-)

[Qemu-devel] [PATCH 1/2] vfio/pci: Fix RTL8168 NIC quirks

[Alex Williamson]

The RTL8168 quirk correctly describes using bit 31 as a signal to
mark a latch/completion, but the code mistakenly uses bit 28.  This
causes the Realtek driver to spin on this register for quite a while,
20k cycles on Windows 7 v7.092 driver.  Then it gets frustrated and
tries to set the bit itself and spins for another 20k cycles.  For
some this still results in a working driver, for others not.  About
the only thing the code really does in its current form is protect
the guest from sneaking in writes to the real hardware MSI-X table.
The fix is obviously to use bit 31 as we document that we should.

The other problem doesn't seem to affect current drivers as nobody
seems to use these window registers for writes to the MSI-X table, but
we need to use the stored data when a write is triggered, not the
value of the current write, which only provides the offset.

Note that only the Windows drivers from Realtek seem to use these
registers, the Microsoft drivers provided with Windows 8.1 do not
access them, nor do Linux in-kernel drivers.

Link: https://bugs.launchpad.net/qemu/+bug/1384892
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: qemu-stable@nongnu.org # v2.1+
---
 hw/vfio/pci.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2ed877f..0af762a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1517,7 +1517,7 @@ static uint64_t vfio_rtl8168_window_quirk_read(void *opaque,
                     memory_region_name(&quirk->mem),
                     vdev->vbasedev.name);
 
-            return quirk->data.address_match ^ 0x10000000U;
+            return quirk->data.address_match ^ 0x80000000U;
         }
         break;
     case 0: /* data */
@@ -1558,7 +1558,7 @@ static void vfio_rtl8168_window_quirk_write(void *opaque, hwaddr addr,
     switch (addr) {
     case 4: /* address */
         if ((data & 0x7fff0000) == 0x10000) {
-            if (data & 0x10000000U &&
+            if (data & 0x80000000U &&
                 vdev->pdev.cap_present & QEMU_PCI_CAP_MSIX) {
 
                 trace_vfio_rtl8168_window_quirk_write_table(
@@ -1566,11 +1566,9 @@ static void vfio_rtl8168_window_quirk_write(void *opaque, hwaddr addr,
                         vdev->vbasedev.name);
 
                 memory_region_dispatch_write(&vdev->pdev.msix_table_mmio,
-                                             (hwaddr)(quirk->data.address_match
-                                                      & 0xfff),
-                                             data,
-                                             size,
-                                             MEMTXATTRS_UNSPECIFIED);
+                                             (hwaddr)(data & 0xfff),
+                                             (uint64_t)quirk->data.address_mask,
+                                             size, MEMTXATTRS_UNSPECIFIED);
             }
 
             quirk->data.flags = 1;

[Qemu-devel] [PATCH 2/2] vfio/pci: Cleanup RTL8168 quirk and tracing

[Alex Williamson]

There's quite a bit of cleanup that can be done to the RTL8168 quirk,
as well as the tracing to prevent a spew of uninteresting accesses
for anything else the driver might choose to use the window registers
for besides the MSI-X table.  There should be no functional change,
but it's now possible to get compact and useful traces by enabling
vfio_rtl8168_quirk*, ex:

vfio_rtl8168_quirk_write 0000:04:00.0 [address]: 0x1f000
vfio_rtl8168_quirk_read 0000:04:00.0 [address]: 0x8001f000
vfio_rtl8168_quirk_read 0000:04:00.0 [data]: 0xfee0100c
vfio_rtl8168_quirk_write 0000:04:00.0 [address]: 0x1f004
vfio_rtl8168_quirk_read 0000:04:00.0 [address]: 0x8001f004
vfio_rtl8168_quirk_read 0000:04:00.0 [data]: 0x0
vfio_rtl8168_quirk_write 0000:04:00.0 [address]: 0x1f008
vfio_rtl8168_quirk_read 0000:04:00.0 [address]: 0x8001f008
vfio_rtl8168_quirk_read 0000:04:00.0 [data]: 0x49b1
vfio_rtl8168_quirk_write 0000:04:00.0 [address]: 0x1f00c
vfio_rtl8168_quirk_read 0000:04:00.0 [address]: 0x8001f00c
vfio_rtl8168_quirk_read 0000:04:00.0 [data]: 0x0

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci.c |   88 ++++++++++++++++++++++++---------------------------------
 trace-events  |   10 +++---
 2 files changed, 41 insertions(+), 57 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 0af762a..70d82d4 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1509,44 +1509,29 @@ static uint64_t vfio_rtl8168_window_quirk_read(void *opaque,
 {
     VFIOQuirk *quirk = opaque;
     VFIOPCIDevice *vdev = quirk->vdev;
+    uint64_t val = 0;
+
+    if (!quirk->data.flags) { /* Non-MSI-X table access */
+        return vfio_region_read(&vdev->bars[quirk->data.bar].region,
+                                addr + 0x70, size);
+    }
 
     switch (addr) {
     case 4: /* address */
-        if (quirk->data.flags) {
-            trace_vfio_rtl8168_window_quirk_read_fake(
-                    memory_region_name(&quirk->mem),
-                    vdev->vbasedev.name);
-
-            return quirk->data.address_match ^ 0x80000000U;
-        }
+        val = quirk->data.address_match ^ 0x80000000U; /* latch/complete */
         break;
     case 0: /* data */
-        if (quirk->data.flags) {
-            uint64_t val;
-
-            trace_vfio_rtl8168_window_quirk_read_table(
-                    memory_region_name(&quirk->mem),
-                    vdev->vbasedev.name);
-
-            if (!(vdev->pdev.cap_present & QEMU_PCI_CAP_MSIX)) {
-                return 0;
-            }
-
+        if ((vdev->pdev.cap_present & QEMU_PCI_CAP_MSIX)) {
             memory_region_dispatch_read(&vdev->pdev.msix_table_mmio,
-                                        (hwaddr)(quirk->data.address_match
-                                                 & 0xfff),
-                                        &val,
-                                        size,
-                                        MEMTXATTRS_UNSPECIFIED);
-            return val;
+                                (hwaddr)(quirk->data.address_match & 0xfff),
+                                &val, size, MEMTXATTRS_UNSPECIFIED);
         }
+        break;
     }
 
-    trace_vfio_rtl8168_window_quirk_read_direct(memory_region_name(&quirk->mem),
-                                                vdev->vbasedev.name);
-
-    return vfio_region_read(&vdev->bars[quirk->data.bar].region,
-                            addr + 0x70, size);
+    trace_vfio_rtl8168_quirk_read(vdev->vbasedev.name,
+                                  addr ? "address" : "data", val);
+    return val;
 }
 
 static void vfio_rtl8168_window_quirk_write(void *opaque, hwaddr addr,
@@ -1557,36 +1542,36 @@ static void vfio_rtl8168_window_quirk_write(void *opaque, hwaddr addr,
 
     switch (addr) {
     case 4: /* address */
-        if ((data & 0x7fff0000) == 0x10000) {
-            if (data & 0x80000000U &&
-                vdev->pdev.cap_present & QEMU_PCI_CAP_MSIX) {
-
-                trace_vfio_rtl8168_window_quirk_write_table(
-                        memory_region_name(&quirk->mem),
-                        vdev->vbasedev.name);
-
-                memory_region_dispatch_write(&vdev->pdev.msix_table_mmio,
-                                             (hwaddr)(data & 0xfff),
-                                             (uint64_t)quirk->data.address_mask,
-                                             size, MEMTXATTRS_UNSPECIFIED);
-            }
-
-            quirk->data.flags = 1;
+        if ((data & 0x7fff0000) == 0x10000) { /* MSI-X table */
+            quirk->data.flags = 1; /* Activate reads */
             quirk->data.address_match = data;
 
-            return;
+            trace_vfio_rtl8168_quirk_write(vdev->vbasedev.name, data);
+
+            if (data & 0x80000000U) { /* Do write */
+                if (vdev->pdev.cap_present & QEMU_PCI_CAP_MSIX) {
+                    hwaddr offset = data & 0xfff;
+                    uint64_t val = quirk->data.address_mask;
+
+                    trace_vfio_rtl8168_quirk_msix(vdev->vbasedev.name,
+                                                  (uint16_t)offset, val);
+
+                    /* Write to the proper guest MSI-X table instead */
+                    memory_region_dispatch_write(&vdev->pdev.msix_table_mmio,
+                                                 offset, val, size,
+                                                 MEMTXATTRS_UNSPECIFIED);
+                }
+                return; /* Do not write guest MSI-X data to hardware */
+            }
+        } else {
+            quirk->data.flags = 0; /* De-activate reads, non-MSI-X */
         }
-        quirk->data.flags = 0;
         break;
     case 0: /* data */
         quirk->data.address_mask = data;
         break;
     }
 
-    trace_vfio_rtl8168_window_quirk_write_direct(
-            memory_region_name(&quirk->mem),
-            vdev->vbasedev.name);
-
     vfio_region_write(&vdev->bars[quirk->data.bar].region,
                       addr + 0x70, data, size);
 }
@@ -1623,8 +1608,9 @@ static void vfio_probe_rtl8168_bar2_window_quirk(VFIOPCIDevice *vdev, int nr)
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
 
-    trace_vfio_probe_rtl8168_bar2_window_quirk(vdev->vbasedev.name);
+    trace_vfio_rtl8168_quirk_enable(vdev->vbasedev.name);
 }
+
 /*
  * Trap the BAR2 MMIO window to config space as well.
  */
diff --git a/trace-events b/trace-events
index 2d395c5..eec9c3d 100644
--- a/trace-events
+++ b/trace-events
@@ -1542,12 +1542,10 @@ vfio_ati_3c3_quirk_read(uint64_t data) " (0x3c3, 1) = 0x%"PRIx64
 vfio_vga_probe_ati_3c3_quirk(const char *name) "Enabled ATI/AMD quirk 0x3c3 BAR4for device %s"
 vfio_probe_ati_bar4_window_quirk(const char *name) "Enabled ATI/AMD BAR4 window quirk for device %s"
 #issue with )
-vfio_rtl8168_window_quirk_read_fake(const char *region_name, const char *name) "%s fake read(%s"
-vfio_rtl8168_window_quirk_read_table(const char *region_name, const char *name) "%s MSI-X table read(%s"
-vfio_rtl8168_window_quirk_read_direct(const char *region_name, const char *name) "%s direct read(%s"
-vfio_rtl8168_window_quirk_write_table(const char *region_name, const char *name) "%s MSI-X table write(%s"
-vfio_rtl8168_window_quirk_write_direct(const char *region_name, const char *name) "%s direct write(%s"
-vfio_probe_rtl8168_bar2_window_quirk(const char *name) "Enabled RTL8168 BAR2 window quirk for device %s"
+vfio_rtl8168_quirk_read(const char *name, const char *type, uint64_t val) "%s [%s]: 0x%"PRIx64
+vfio_rtl8168_quirk_write(const char *name, uint64_t val) "%s [address]: 0x%"PRIx64
+vfio_rtl8168_quirk_msix(const char *name, uint16_t offset, uint64_t val) "%s MSI-X table write[0x%x]: 0x%"PRIx64
+vfio_rtl8168_quirk_enable(const char *name) "%s"
 vfio_probe_ati_bar2_4000_quirk(const char *name) "Enabled ATI/AMD BAR2 0x4000 quirk for device %s"
 vfio_nvidia_3d0_quirk_read(int size, uint64_t data) " (0x3d0, %d) = 0x%"PRIx64
 vfio_nvidia_3d0_quirk_write(uint64_t data, int size) " (0x3d0, 0x%"PRIx64", %d)"