[Qemu-devel] vfio: blacklist loading of unstable roms

[Bandan Das]

Certain cards such as the Broadcom BCM57810 have rom quirks
that exhibit unstable system behavior duing device assignment. In
the particular case of 57810, rom execution hangs and if a FLR
follows, the device becomes inoperable until a power cycle.

This is a simple change to disable rom loading for such cards.
In terms of implementation change, rombar now has a default value
of 2. Existing code shouldn't be affected by changing the default value
of rombar since all relevant decisions only rely on whether rom_bar is
zero or non-zero. The motivation behind this change is that in
certain cases such as a firmware upgrade, the user might
want to override this blacklisting behavior and can do so
by running with rombar = 1. Same reasoning applies to running with
romfile.

Signed-off-by: Bandan Das <address@hidden>
---
 hw/misc/vfio.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pci.c   |  3 ++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 8db182f..f5021f4 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -209,6 +209,16 @@ typedef struct VFIOGroup {
     QLIST_ENTRY(VFIOGroup) container_next;
 } VFIOGroup;
 
+typedef struct VFIORomQList {
+    unsigned int vendor_id;
+    unsigned int device_id;
+} VFIORomQList;
+
+static const VFIORomQList romqdevlist[] = {
+    /* Broadcom BCM 57810 */
+    { 0x14e4, 0x168e }
+};
+
 #define MSIX_CAP_LENGTH 12
 
 static QLIST_HEAD(, VFIOContainer)
@@ -1197,16 +1207,69 @@ static const MemoryRegionOps vfio_rom_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    unsigned int vendor_id, device_id;
+    int count = 0;
+
+    vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
+    device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
+
+    while (count < ARRAY_SIZE(romqdevlist)) {
+        if (romqdevlist[count].vendor_id == vendor_id &&
+            romqdevlist[count].device_id == device_id) {
+                return true;
+        }
+        count++;
+    }
+
+    return false;
+}
+
 static void vfio_pci_size_rom(VFIODevice *vdev)
 {
     uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
     off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
     char name[32];
+    int rom_quirk = 0;
+
+    if (vfio_blacklist_opt_rom(vdev)) {
+        rom_quirk = 1;
+    }
 
     if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
+        /* Since pci handles romfile, just print a message and return */
+        if (rom_quirk && vdev->pdev.romfile) {
+            error_printf("Warning : Device at %04x:%02x:%02x.%x "
+                         "is known to cause system instability issues during "
+                         "option rom execution. "
+                         "Proceeding anyway since user specified romfile\n",
+                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                         vdev->host.function);
+        }
         return;
     }
 
+    if (rom_quirk && vdev->pdev.rom_bar) {
+        if (vdev->pdev.rom_bar == 1) {
+            error_printf("Warning : Device at %04x:%02x:%02x.%x "
+                         "is known to cause system instability issues during "
+                         "option rom execution. "
+                         "Proceeding anyway since user specified rombar=1\n",
+                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                         vdev->host.function);
+        } else {
+            error_printf("Warning : Rom loading for device at "
+                         "%04x:%02x:%02x.%x has been disabled due to "
+                         "system instability issues. "
+                         "Specify rombar=1 or romfile to force\n",
+                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                         vdev->host.function);
+            return;
+        }
+    }
+
     /*
      * Use the same size ROM BAR as the physical device.  The contents
      * will get filled in later when the guest tries to read it.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4e0701d..65766d8 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -53,7 +53,8 @@ static void pci_bus_finalize(Object *obj);
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
-    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
+    /* 0 = disable, 1 = user requested (on), 2 = default (on) */
+    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
     DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
                     QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
     DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
-- 
1.8.3.1