* [Qemu-devel] vfio: blacklist loading of unstable roms @ 2014-02-19 16:12 Bandan Das 2014-02-19 18:08 ` Alex Williamson 0 siblings, 1 reply; 5+ messages in thread From: Bandan Das @ 2014-02-19 16:12 UTC (permalink / raw) To: qemu-devel; +Cc: Alex Williamson, Michael Tsirkin 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; +} 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, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] vfio: blacklist loading of unstable roms 2014-02-19 16:12 [Qemu-devel] vfio: blacklist loading of unstable roms Bandan Das @ 2014-02-19 18:08 ` Alex Williamson 2014-02-19 18:58 ` Bandan Das 0 siblings, 1 reply; 5+ messages in thread From: Alex Williamson @ 2014-02-19 18:08 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel, Michael Tsirkin 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] vfio: blacklist loading of unstable roms 2014-02-19 18:08 ` Alex Williamson @ 2014-02-19 18:58 ` Bandan Das 2014-02-19 19:06 ` Alex Williamson 0 siblings, 1 reply; 5+ messages in thread From: Bandan Das @ 2014-02-19 18:58 UTC (permalink / raw) To: Alex Williamson; +Cc: qemu-devel, Michael Tsirkin 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] vfio: blacklist loading of unstable roms 2014-02-19 18:58 ` Bandan Das @ 2014-02-19 19:06 ` Alex Williamson 2014-02-19 19:32 ` Bandan Das 0 siblings, 1 reply; 5+ messages in thread From: Alex Williamson @ 2014-02-19 19:06 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel, Michael Tsirkin 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] vfio: blacklist loading of unstable roms 2014-02-19 19:06 ` Alex Williamson @ 2014-02-19 19:32 ` Bandan Das 0 siblings, 0 replies; 5+ messages in thread From: Bandan Das @ 2014-02-19 19:32 UTC (permalink / raw) To: Alex Williamson; +Cc: qemu-devel, Michael Tsirkin 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-02-19 19:32 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-19 16:12 [Qemu-devel] vfio: blacklist loading of unstable roms Bandan Das 2014-02-19 18:08 ` Alex Williamson 2014-02-19 18:58 ` Bandan Das 2014-02-19 19:06 ` Alex Williamson 2014-02-19 19:32 ` Bandan Das
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).