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