pci-assign: add a way to blacklist loading of unstable roms

commit 4b9430294ed added an option in vfio to blacklist
roms that are known to be unstable. Add a similar mechanism
for pci-assign as well. The default behavior is to disable
loading but can be overriden by specifying rombar or romfile

Signed-off-by: Bandan Das <bsd@redhat.com>
---
Note: ignored checkpatch reported error-
ERROR: need consistent spacing around '*' (ctx:WxV)

 hw/i386/kvm/pci-assign.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

On Fri, 2014-04-04 at 18:49 -0400, Bandan Das wrote:
> commit 4b9430294ed added an option in vfio to blacklist
> roms that are known to be unstable. Add a similar mechanism
> for pci-assign as well. The default behavior is to disable
> loading but can be overriden by specifying rombar or romfile

In what case would we not ask users to switch to vfio for this?  At some
point we need to stop improving pci-assign if we plan to remove it.
Thanks,

Alex

> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
> Note: ignored checkpatch reported error-
> ERROR: need consistent spacing around '*' (ctx:WxV)
> 
>  hw/i386/kvm/pci-assign.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index a825871..42618dd 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -59,6 +59,29 @@
>  #define DEBUG(fmt, ...)
>  #endif
>  
> +typedef struct PCIRomBlacklistEntry {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +} PCIRomBlacklistEntry;
> +
> +/*
> + * List of device ids/vendor ids for which to disable
> + * option rom loading. This avoids the guest hangs during rom
> + * execution as noticed with the BCM 57810 card for lack of a
> + * more better way to handle such issues.
> + * The  user can still override by specifying a romfile or
> + * rombar=1.
> + * Please see https://bugs.launchpad.net/qemu/+bug/1284874
> + * for an analysis of the 57810 card hang. When adding
> + * a new vendor id/device id combination below, please also add
> + * your card/environment details and information that could
> + * help in debugging to the bug tracking this issue
> + */
> +static const PCIRomBlacklistEntry romblacklist[] = {
> +    /* Broadcom BCM 57810 */
> +    { 0x14e4, 0x168e }
> +};
> +
>  typedef struct PCIRegion {
>      int type;           /* Memory or port I/O */
>      int valid;
> @@ -1834,6 +1857,26 @@ static void assign_register_types(void)
>  
>  type_init(assign_register_types)
>  
> +static bool blacklist_opt_rom(AssignedDevice *adev)
> +{
> +    PCIDevice *pdev = &adev->dev;
> +    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;
> +}
> +
>  /*
>   * Scan the assigned devices for the devices that have an option ROM, and then
>   * load the corresponding ROM data to RAM. If an error occurs while loading an
> @@ -1846,9 +1889,19 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>      uint8_t val;
>      struct stat st;
>      void *ptr;
> +    DeviceState *devstate = DEVICE(dev);
>  
>      /* If loading ROM from file, pci handles it */
>      if (dev->dev.romfile || !dev->dev.rom_bar) {
> +        /* Since pci handles romfile, just print a message and return */
> +        if (blacklist_opt_rom(dev) && dev->dev.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",
> +                         dev->host.domain, dev->host.bus, dev->host.slot,
> +                         dev->host.function);
> +        }
>          return;
>      }
>  
> @@ -1866,6 +1919,26 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>          return;
>      }
>  
> +    if (blacklist_opt_rom(dev)) {
> +        if (devstate->opts && qemu_opt_get(devstate->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",
> +                         dev->host.domain, dev->host.bus, dev->host.slot,
> +                         dev->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",
> +                         dev->host.domain, dev->host.bus, dev->host.slot,
> +                         dev->host.function);
> +            return;
> +        }
> +    }
> +
>      /* Write "1" to the ROM file to enable it */
>      fp = fopen(rom_file, "r+");
>      if (fp == NULL) {

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

> On Fri, 2014-04-04 at 18:49 -0400, Bandan Das wrote:
>> commit 4b9430294ed added an option in vfio to blacklist
>> roms that are known to be unstable. Add a similar mechanism
>> for pci-assign as well. The default behavior is to disable
>> loading but can be overriden by specifying rombar or romfile
>
> In what case would we not ask users to switch to vfio for this?  At some
> point we need to stop improving pci-assign if we plan to remove it.
> Thanks,

What if the user is still stuck with an older kernel ?

It's a few lines of change that avoids a bug that could inadvertently
force a user to cold power cycle the host. Not adding new features makes
sense but addressing an issue that could stall the host seems like 
something we should have as long as the code is around (especially since
it's a small and unobtrusive change).

> Alex
>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>> Note: ignored checkpatch reported error-
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> 
>>  hw/i386/kvm/pci-assign.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>> 
>> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
>> index a825871..42618dd 100644
>> --- a/hw/i386/kvm/pci-assign.c
>> +++ b/hw/i386/kvm/pci-assign.c
>> @@ -59,6 +59,29 @@
>>  #define DEBUG(fmt, ...)
>>  #endif
>>  
>> +typedef struct PCIRomBlacklistEntry {
>> +    uint16_t vendor_id;
>> +    uint16_t device_id;
>> +} PCIRomBlacklistEntry;
>> +
>> +/*
>> + * List of device ids/vendor ids for which to disable
>> + * option rom loading. This avoids the guest hangs during rom
>> + * execution as noticed with the BCM 57810 card for lack of a
>> + * more better way to handle such issues.
>> + * The  user can still override by specifying a romfile or
>> + * rombar=1.
>> + * Please see https://bugs.launchpad.net/qemu/+bug/1284874
>> + * for an analysis of the 57810 card hang. When adding
>> + * a new vendor id/device id combination below, please also add
>> + * your card/environment details and information that could
>> + * help in debugging to the bug tracking this issue
>> + */
>> +static const PCIRomBlacklistEntry romblacklist[] = {
>> +    /* Broadcom BCM 57810 */
>> +    { 0x14e4, 0x168e }
>> +};
>> +
>>  typedef struct PCIRegion {
>>      int type;           /* Memory or port I/O */
>>      int valid;
>> @@ -1834,6 +1857,26 @@ static void assign_register_types(void)
>>  
>>  type_init(assign_register_types)
>>  
>> +static bool blacklist_opt_rom(AssignedDevice *adev)
>> +{
>> +    PCIDevice *pdev = &adev->dev;
>> +    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;
>> +}
>> +
>>  /*
>>   * Scan the assigned devices for the devices that have an option ROM, and then
>>   * load the corresponding ROM data to RAM. If an error occurs while loading an
>> @@ -1846,9 +1889,19 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>>      uint8_t val;
>>      struct stat st;
>>      void *ptr;
>> +    DeviceState *devstate = DEVICE(dev);
>>  
>>      /* If loading ROM from file, pci handles it */
>>      if (dev->dev.romfile || !dev->dev.rom_bar) {
>> +        /* Since pci handles romfile, just print a message and return */
>> +        if (blacklist_opt_rom(dev) && dev->dev.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",
>> +                         dev->host.domain, dev->host.bus, dev->host.slot,
>> +                         dev->host.function);
>> +        }
>>          return;
>>      }
>>  
>> @@ -1866,6 +1919,26 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>>          return;
>>      }
>>  
>> +    if (blacklist_opt_rom(dev)) {
>> +        if (devstate->opts && qemu_opt_get(devstate->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",
>> +                         dev->host.domain, dev->host.bus, dev->host.slot,
>> +                         dev->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",
>> +                         dev->host.domain, dev->host.bus, dev->host.slot,
>> +                         dev->host.function);
>> +            return;
>> +        }
>> +    }
>> +
>>      /* Write "1" to the ROM file to enable it */
>>      fp = fopen(rom_file, "r+");
>>      if (fp == NULL) {