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

[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 <bsd@redhat.com>
---
 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

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

[Alex Williamson]

On Wed, 2014-02-19 at 11:12 -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 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 <bsd@redhat.com>
> ---
>  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;

uint16_t

> +} VFIORomQList;
> +
> +static const VFIORomQList romqdevlist[] = {
> +    /* Broadcom BCM 57810 */
> +    { 0x14e4, 0x168e }
> +};

Naming of these doesn't make sense, there's neither a QLIST nor are
these qdevs.  We're creating a blacklist, so I'd probably name the array
VFIORomBlacklist and the entry can simply be a VFIOBlacklistEntry.

> +
>  #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;

uint16_t

> +    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;

bool?  Actually, we don't even need this variable, just call the
blacklist test function inline.  There's not even a path that would call
it twice.

> +
> +    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,

This should be a separate patch.  Thanks,

Alex

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

[Bandan Das]

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

> On Wed, 2014-02-19 at 11:12 -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 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 <bsd@redhat.com>
>> ---
>>  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;
>
> uint16_t

Oops! yes, indeed.

>> +} VFIORomQList;
>> +
>> +static const VFIORomQList romqdevlist[] = {
>> +    /* Broadcom BCM 57810 */
>> +    { 0x14e4, 0x168e }
>> +};
>
> Naming of these doesn't make sense, there's neither a QLIST nor are
> these qdevs.  We're creating a blacklist, so I'd probably name the array
> VFIORomBlacklist and the entry can simply be a VFIOBlacklistEntry.

The naming signified abbreviation of VFIORomQuirkList and romquirkdevicelist.
Obviously, it ended up signifying something else altogether. Your suggestion
sounds fine and I will change it in the next version.

>> +
>>  #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;
>
> uint16_t
>
>> +    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;
>
> bool?  Actually, we don't even need this variable, just call the
> blacklist test function inline.  There's not even a path that would call
> it twice.

Yeah, it is actually used twice below - Once for the case 
where romfile is set and once for when rombar is set. If you
prefer, I can re-word this so that it's called once and displays
a common message instead of different ones as in the current 
version. 

>> +
>> +    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,
>
> This should be a separate patch.  Thanks,

Umm.. isn't this part of "one logical change" and be grouped together ?
Or having it in a different patch makes maintainer's work easy ?

> Alex

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

[Alex Williamson]

On Wed, 2014-02-19 at 13:58 -0500, Bandan Das wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Wed, 2014-02-19 at 11:12 -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 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 <bsd@redhat.com>
> >> ---
> >>  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;
> >
> > uint16_t
> 
> Oops! yes, indeed.
> 
> >> +} VFIORomQList;
> >> +
> >> +static const VFIORomQList romqdevlist[] = {
> >> +    /* Broadcom BCM 57810 */
> >> +    { 0x14e4, 0x168e }
> >> +};
> >
> > Naming of these doesn't make sense, there's neither a QLIST nor are
> > these qdevs.  We're creating a blacklist, so I'd probably name the array
> > VFIORomBlacklist and the entry can simply be a VFIOBlacklistEntry.
> 
> The naming signified abbreviation of VFIORomQuirkList and romquirkdevicelist.
> Obviously, it ended up signifying something else altogether. Your suggestion
> sounds fine and I will change it in the next version.
> 
> >> +
> >>  #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;
> >
> > uint16_t
> >
> >> +    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;
> >
> > bool?  Actually, we don't even need this variable, just call the
> > blacklist test function inline.  There's not even a path that would call
> > it twice.
> 
> Yeah, it is actually used twice below - Once for the case 
> where romfile is set and once for when rombar is set. If you
> prefer, I can re-word this so that it's called once and displays
> a common message instead of different ones as in the current 
> version. 

It's used twice, but there's no path that calls it more than once.

> >> +
> >> +    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,
> >
> > This should be a separate patch.  Thanks,
> 
> Umm.. isn't this part of "one logical change" and be grouped together ?
> Or having it in a different patch makes maintainer's work easy ?

This latter bit is an infrastructure change and should be evaluated on
it's own.  The rest of it just depends on that change.  Thanks,

Alex

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

[Bandan Das]

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

> On Wed, 2014-02-19 at 13:58 -0500, Bandan Das wrote:
>> Alex Williamson <alex.williamson@redhat.com> writes:
>> 
>> > On Wed, 2014-02-19 at 11:12 -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 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 <bsd@redhat.com>
>> >> ---
>> >>  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;
>> >
>> > uint16_t
>> 
>> Oops! yes, indeed.
>> 
>> >> +} VFIORomQList;
>> >> +
>> >> +static const VFIORomQList romqdevlist[] = {
>> >> +    /* Broadcom BCM 57810 */
>> >> +    { 0x14e4, 0x168e }
>> >> +};
>> >
>> > Naming of these doesn't make sense, there's neither a QLIST nor are
>> > these qdevs.  We're creating a blacklist, so I'd probably name the array
>> > VFIORomBlacklist and the entry can simply be a VFIOBlacklistEntry.
>> 
>> The naming signified abbreviation of VFIORomQuirkList and romquirkdevicelist.
>> Obviously, it ended up signifying something else altogether. Your suggestion
>> sounds fine and I will change it in the next version.
>> 
>> >> +
>> >>  #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;
>> >
>> > uint16_t
>> >
>> >> +    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;
>> >
>> > bool?  Actually, we don't even need this variable, just call the
>> > blacklist test function inline.  There's not even a path that would call
>> > it twice.
>> 
>> Yeah, it is actually used twice below - Once for the case 
>> where romfile is set and once for when rombar is set. If you
>> prefer, I can re-word this so that it's called once and displays
>> a common message instead of different ones as in the current 
>> version. 
>
> It's used twice, but there's no path that calls it more than once.

Ah! I see what you are saying now. It either gets called for the 
first "if" condition or the second. Cannot be possibly called for 
both. Ok, will remove rom_quirk in v2.

>> >> +
>> >> +    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,
>> >
>> > This should be a separate patch.  Thanks,
>> 
>> Umm.. isn't this part of "one logical change" and be grouped together ?
>> Or having it in a different patch makes maintainer's work easy ?
>
> This latter bit is an infrastructure change and should be evaluated on
> it's own.  The rest of it just depends on that change.  Thanks,

I agree, infrastructure changes such as a new function need to
evaluated based on what functionality they provide and should be 
rightfully in a separate patch.

But here, to understand why the default value of a property changed, 
I think that we need to look at the accompanying change that depends 
on it and having them all together makes the review easier.

Anyway, I don't feel too strongly either way, just wanted to 
understand your motivation :)


> Alex