| Message ID | jpgvbwb84jr.fsf@nelium.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <bsd@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 > +} 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
Alex Williamson <alex.williamson@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 <bsd@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. >> + >> + 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
On Wed, 2014-02-19 at 13:58 -0500, Bandan Das wrote: > Alex Williamson <alex.williamson@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 <bsd@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. > >> + > >> + 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, Alex
Alex Williamson <alex.williamson@redhat.com> writes: > On Wed, 2014-02-19 at 13:58 -0500, Bandan Das wrote: >> Alex Williamson <alex.williamson@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 <bsd@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
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; +} VFIORomQList; + +static const VFIORomQList romqdevlist[] = { + /* Broadcom BCM 57810 */ + { 0x14e4, 0x168e } +}; + #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; + 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; + + 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,
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 <bsd@redhat.com> --- hw/misc/vfio.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/pci/pci.c | 3 ++- 2 files changed, 65 insertions(+), 1 deletion(-)