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: Bandan Das
Subject: Re: [Qemu-devel] vfio: blacklist loading of unstable roms
Date: Wed, 19 Feb 2014 13:58:15 -0500
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

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

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



reply via email to

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