qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alex Williamson
Subject: Re: [Qemu-devel] vfio: blacklist loading of unstable roms
Date: Wed, 19 Feb 2014 11:08:57 -0700

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]