Alex Williamson <alex.william...@redhat.com> writes:
> On Wed, 2014-02-19 at 13:58 -0500, Bandan Das wrote:
>> Alex Williamson <alex.william...@redhat.com> 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 <b...@redhat.com>
>> >> ---
>> >> 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.
>
> It's used twice, but there's no path that calls it more than once.
Ah! I see what you are saying now. It either gets called for the
first "if" condition or the second. Cannot be possibly called for
both. Ok, will remove rom_quirk in v2.
>> >> +
>> >> + 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 ?
>
> This latter bit is an infrastructure change and should be evaluated on
> it's own. The rest of it just depends on that change. Thanks,
I agree, infrastructure changes such as a new function need to
evaluated based on what functionality they provide and should be
rightfully in a separate patch.
But here, to understand why the default value of a property changed,
I think that we need to look at the accompanying change that depends
on it and having them all together makes the review easier.
Anyway, I don't feel too strongly either way, just wanted to
understand your motivation :)
> Alex
