[Qemu-devel] [PATCH 0/2 v3] vfio: blacklist loading of unstable roms

[Qemu-devel] [PATCH 0/2 v3] vfio: blacklist loading of unstable roms

[Bandan Das]

v3:
1. Change the "force" logic to depend on whether the user specified rombar,
eliminate dependence on multiple values of rom_bar
2. Avoid printing the informational message if there's no rom
3. Minor changes to commit messages

v2:
1. Break up into two patches separating the infrastructural changes
2. Change vendor_id and device_id type to uint16_t
3. Rename struct for blacklisted devid and vendorid to more meaningful names
4. Remove unnecessary rom_quirk variable and just call vfio_blacklist_opt_rom()
in vfio_pci_size_rom()

Bandan Das (2):
  qdev-monitor: set DeviceState opts before calling realize
  vfio: blacklist loading of unstable roms

 hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qdev-monitor.c |  4 +++-
 2 files changed, 63 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize

[Bandan Das]

Setting opts before the realize property is set allows the
following patch to make decisions based on whether the user
specified "rombar". This also avoids having to create a new
tristate property especially for this purpose

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 qdev-monitor.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 3a7dc0d..6673e3c 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -548,16 +548,18 @@ DeviceState *qdev_device_add(QemuOpts *opts)
                                   OBJECT(dev), NULL);
         g_free(name);
     }
+
+    dev->opts = opts;
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {
         qerror_report_err(err);
         error_free(err);
+        dev->opts = NULL;
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
         qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
-    dev->opts = opts;
     return dev;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize

[Andreas Färber]

Am 25.02.2014 05:34, schrieb Bandan Das:
> Setting opts before the realize property is set allows the
> following patch to make decisions based on whether the user
> specified "rombar". This also avoids having to create a new
> tristate property especially for this purpose
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  qdev-monitor.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Makes perfect sense,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Queuing a copy on qom-next to avoid conflicts with a v2 for Amos' link<>
issue.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 2/2 v3] 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
change blacklists loading of rom for such cards unless the user
specifies a romfile or rombar=1 on the cmd line

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 8db182f..df3ceee 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 VFIORomBlacklistEntry {
+    uint16_t vendor_id;
+    uint16_t device_id;
+} VFIORomBlacklistEntry;
+
+static const VFIORomBlacklistEntry romblacklist[] = {
+    /* Broadcom BCM 57810 */
+    { 0x14e4, 0x168e }
+};
+
 #define MSIX_CAP_LENGTH 12
 
 static QLIST_HEAD(, VFIOContainer)
@@ -1197,13 +1207,43 @@ static const MemoryRegionOps vfio_rom_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint16_t 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(romblacklist)) {
+        if (romblacklist[count].vendor_id == vendor_id &&
+            romblacklist[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;
+    DeviceState *dev = DEVICE(vdev);
     char name[32];
 
     if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
+        /* Since pci handles romfile, just print a message and return */
+        if (vfio_blacklist_opt_rom(vdev) && 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;
     }
 
@@ -1227,6 +1267,26 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
         return;
     }
 
+    if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {
+        if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
+            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 non zero value for "
+                         "rombar\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;
+        }
+    }
+
     DPRINTF("%04x:%02x:%02x.%x ROM size 0x%x\n", vdev->host.domain,
             vdev->host.bus, vdev->host.slot, vdev->host.function, size);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms

[Alex Williamson]

On Mon, 2014-02-24 at 23:34 -0500, Bandan Das wrote:
> 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
> change blacklists loading of rom for such cards unless the user
> specifies a romfile or rombar=1 on the cmd line
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 8db182f..df3ceee 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 VFIORomBlacklistEntry {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +} VFIORomBlacklistEntry;
> +
> +static const VFIORomBlacklistEntry romblacklist[] = {
> +    /* Broadcom BCM 57810 */
> +    { 0x14e4, 0x168e }
> +};
> +

Any progress on a bug reference or trying to extract a version from the
ROM so we can compare against future ROMs?  We can always file a new bug
in launchpad for tracking if needed.

>  #define MSIX_CAP_LENGTH 12
>  
>  static QLIST_HEAD(, VFIOContainer)
> @@ -1197,13 +1207,43 @@ static const MemoryRegionOps vfio_rom_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
> +{
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint16_t 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(romblacklist)) {
> +        if (romblacklist[count].vendor_id == vendor_id &&
> +            romblacklist[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;
> +    DeviceState *dev = DEVICE(vdev);
>      char name[32];
>  
>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> +        /* Since pci handles romfile, just print a message and return */
> +        if (vfio_blacklist_opt_rom(vdev) && 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;
>      }
>  
> @@ -1227,6 +1267,26 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
>          return;
>      }
>  
> +    if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {

We would have taken the return above if !rom_bar, so that test is
unnecessary here.  Thanks,

Alex

> +        if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
> +            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 non zero value for "
> +                         "rombar\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;
> +        }
> +    }
> +
>      DPRINTF("%04x:%02x:%02x.%x ROM size 0x%x\n", vdev->host.domain,
>              vdev->host.bus, vdev->host.slot, vdev->host.function, size);
>  

Re: [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms

[Bandan Das]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Mon, 2014-02-24 at 23:34 -0500, Bandan Das wrote:
>> 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
>> change blacklists loading of rom for such cards unless the user
>> specifies a romfile or rombar=1 on the cmd line
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>> 
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 8db182f..df3ceee 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 VFIORomBlacklistEntry {
>> +    uint16_t vendor_id;
>> +    uint16_t device_id;
>> +} VFIORomBlacklistEntry;
>> +
>> +static const VFIORomBlacklistEntry romblacklist[] = {
>> +    /* Broadcom BCM 57810 */
>> +    { 0x14e4, 0x168e }
>> +};
>> +
>
> Any progress on a bug reference or trying to extract a version from the
> ROM so we can compare against future ROMs?

I will get to it when there is actually a version of rom code that fixes it.
AFAIK Broadcom is looking into it but I am not sure if there will be a fix. 
Comparing versions (or something else in the rom that can be compared) is 
probably not a very good idea considering there could be other cards in the 
wild with this issue and we then have to manage a three item list for all these
cards - devid, vendorid, "version which fixed the problem", not to mention the 
comparision method might be different based on conventions that each vendor 
has adopted. (It appears I am seeing something similar with an Emulex card too!,
still investigating)
But anyway, I don't have a better idea to offer as of now.

> We can always file a new bug
> in launchpad for tracking if needed.

Sure. Should I file a bug at https://bugs.launchpad.net/qemu/ and post
a link to it in the code comments ?

>>  #define MSIX_CAP_LENGTH 12
>>  
>>  static QLIST_HEAD(, VFIOContainer)
>> @@ -1197,13 +1207,43 @@ static const MemoryRegionOps vfio_rom_ops = {
>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>>  
>> +static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
>> +{
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    uint16_t 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(romblacklist)) {
>> +        if (romblacklist[count].vendor_id == vendor_id &&
>> +            romblacklist[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;
>> +    DeviceState *dev = DEVICE(vdev);
>>      char name[32];
>>  
>>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>> +        /* Since pci handles romfile, just print a message and return */
>> +        if (vfio_blacklist_opt_rom(vdev) && 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;
>>      }
>>  
>> @@ -1227,6 +1267,26 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
>>          return;
>>      }
>>  
>> +    if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {
>
> We would have taken the return above if !rom_bar, so that test is
> unnecessary here.  Thanks,

Agreed, sorry! will fix in the next version.

> Alex
>
>> +        if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
>> +            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 non zero value for "
>> +                         "rombar\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;
>> +        }
>> +    }
>> +
>>      DPRINTF("%04x:%02x:%02x.%x ROM size 0x%x\n", vdev->host.domain,
>>              vdev->host.bus, vdev->host.slot, vdev->host.function, size);
>>