Re: [Qemu-devel] [PATCH] pci-assign: add a way to blacklist loading of unstable roms

[Alex Williamson]

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 <address@hidden>
> ---
> 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) {