* [Qemu-devel] [PATCH 0/2 v3] vfio: blacklist loading of unstable roms @ 2014-02-25 4:34 Bandan Das 2014-02-25 4:34 ` [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize Bandan Das 2014-02-25 4:34 ` [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms Bandan Das 0 siblings, 2 replies; 6+ messages in thread From: Bandan Das @ 2014-02-25 4:34 UTC (permalink / raw) To: qemu-devel Cc: Alex Williamson, Michael S. Tsirkin, Andreas Färber, Markus Armbruster v3: 1. Change the "force" logic to depend on whether the user specified rombar, eliminate dependence on multiple values of rom_bar 2. Avoid printing the informational message if there's no rom 3. Minor changes to commit messages v2: 1. Break up into two patches separating the infrastructural changes 2. Change vendor_id and device_id type to uint16_t 3. Rename struct for blacklisted devid and vendorid to more meaningful names 4. Remove unnecessary rom_quirk variable and just call vfio_blacklist_opt_rom() in vfio_pci_size_rom() Bandan Das (2): qdev-monitor: set DeviceState opts before calling realize vfio: blacklist loading of unstable roms hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qdev-monitor.c | 4 +++- 2 files changed, 63 insertions(+), 1 deletion(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize 2014-02-25 4:34 [Qemu-devel] [PATCH 0/2 v3] vfio: blacklist loading of unstable roms Bandan Das @ 2014-02-25 4:34 ` Bandan Das 2014-02-25 7:39 ` Andreas Färber 2014-02-25 4:34 ` [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms Bandan Das 1 sibling, 1 reply; 6+ messages in thread From: Bandan Das @ 2014-02-25 4:34 UTC (permalink / raw) To: qemu-devel Cc: Alex Williamson, Michael S. Tsirkin, Andreas Färber, Markus Armbruster Setting opts before the realize property is set allows the following patch to make decisions based on whether the user specified "rombar". This also avoids having to create a new tristate property especially for this purpose Signed-off-by: Bandan Das <bsd@redhat.com> --- qdev-monitor.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/qdev-monitor.c b/qdev-monitor.c index 3a7dc0d..6673e3c 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -548,16 +548,18 @@ DeviceState *qdev_device_add(QemuOpts *opts) OBJECT(dev), NULL); g_free(name); } + + dev->opts = opts; object_property_set_bool(OBJECT(dev), true, "realized", &err); if (err != NULL) { qerror_report_err(err); error_free(err); + dev->opts = NULL; object_unparent(OBJECT(dev)); object_unref(OBJECT(dev)); qerror_report(QERR_DEVICE_INIT_FAILED, driver); return NULL; } - dev->opts = opts; return dev; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize 2014-02-25 4:34 ` [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize Bandan Das @ 2014-02-25 7:39 ` Andreas Färber 0 siblings, 0 replies; 6+ messages in thread From: Andreas Färber @ 2014-02-25 7:39 UTC (permalink / raw) To: Bandan Das, qemu-devel Cc: Paolo Bonzini, Alex Williamson, Amos Kong, Markus Armbruster, Michael S. Tsirkin Am 25.02.2014 05:34, schrieb Bandan Das: > Setting opts before the realize property is set allows the > following patch to make decisions based on whether the user > specified "rombar". This also avoids having to create a new > tristate property especially for this purpose > > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > qdev-monitor.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Makes perfect sense, Reviewed-by: Andreas Färber <afaerber@suse.de> Queuing a copy on qom-next to avoid conflicts with a v2 for Amos' link<> issue. Thanks, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms 2014-02-25 4:34 [Qemu-devel] [PATCH 0/2 v3] vfio: blacklist loading of unstable roms Bandan Das 2014-02-25 4:34 ` [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize Bandan Das @ 2014-02-25 4:34 ` Bandan Das 2014-02-25 15:41 ` Alex Williamson 1 sibling, 1 reply; 6+ messages in thread From: Bandan Das @ 2014-02-25 4:34 UTC (permalink / raw) To: qemu-devel Cc: Alex Williamson, Michael S. Tsirkin, Andreas Färber, Markus Armbruster 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 change blacklists loading of rom for such cards unless the user specifies a romfile or rombar=1 on the cmd line Signed-off-by: Bandan Das <bsd@redhat.com> --- hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c index 8db182f..df3ceee 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 VFIORomBlacklistEntry { + uint16_t vendor_id; + uint16_t device_id; +} VFIORomBlacklistEntry; + +static const VFIORomBlacklistEntry romblacklist[] = { + /* Broadcom BCM 57810 */ + { 0x14e4, 0x168e } +}; + #define MSIX_CAP_LENGTH 12 static QLIST_HEAD(, VFIOContainer) @@ -1197,13 +1207,43 @@ static const MemoryRegionOps vfio_rom_ops = { .endianness = DEVICE_LITTLE_ENDIAN, }; +static bool vfio_blacklist_opt_rom(VFIODevice *vdev) +{ + PCIDevice *pdev = &vdev->pdev; + uint16_t 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(romblacklist)) { + if (romblacklist[count].vendor_id == vendor_id && + romblacklist[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; + DeviceState *dev = DEVICE(vdev); char name[32]; if (vdev->pdev.romfile || !vdev->pdev.rom_bar) { + /* Since pci handles romfile, just print a message and return */ + if (vfio_blacklist_opt_rom(vdev) && 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; } @@ -1227,6 +1267,26 @@ static void vfio_pci_size_rom(VFIODevice *vdev) return; } + if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) { + if (dev->opts && qemu_opt_get(dev->opts, "rombar")) { + 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 non zero value for " + "rombar\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; + } + } + DPRINTF("%04x:%02x:%02x.%x ROM size 0x%x\n", vdev->host.domain, vdev->host.bus, vdev->host.slot, vdev->host.function, size); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms 2014-02-25 4:34 ` [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms Bandan Das @ 2014-02-25 15:41 ` Alex Williamson 2014-02-25 16:51 ` Bandan Das 0 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2014-02-25 15:41 UTC (permalink / raw) To: Bandan Das Cc: Markus Armbruster, Michael S. Tsirkin, qemu-devel, Andreas Färber On Mon, 2014-02-24 at 23:34 -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 > change blacklists loading of rom for such cards unless the user > specifies a romfile or rombar=1 on the cmd line > > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > index 8db182f..df3ceee 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 VFIORomBlacklistEntry { > + uint16_t vendor_id; > + uint16_t device_id; > +} VFIORomBlacklistEntry; > + > +static const VFIORomBlacklistEntry romblacklist[] = { > + /* Broadcom BCM 57810 */ > + { 0x14e4, 0x168e } > +}; > + Any progress on a bug reference or trying to extract a version from the ROM so we can compare against future ROMs? We can always file a new bug in launchpad for tracking if needed. > #define MSIX_CAP_LENGTH 12 > > static QLIST_HEAD(, VFIOContainer) > @@ -1197,13 +1207,43 @@ static const MemoryRegionOps vfio_rom_ops = { > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > +static bool vfio_blacklist_opt_rom(VFIODevice *vdev) > +{ > + PCIDevice *pdev = &vdev->pdev; > + uint16_t 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(romblacklist)) { > + if (romblacklist[count].vendor_id == vendor_id && > + romblacklist[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; > + DeviceState *dev = DEVICE(vdev); > char name[32]; > > if (vdev->pdev.romfile || !vdev->pdev.rom_bar) { > + /* Since pci handles romfile, just print a message and return */ > + if (vfio_blacklist_opt_rom(vdev) && 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; > } > > @@ -1227,6 +1267,26 @@ static void vfio_pci_size_rom(VFIODevice *vdev) > return; > } > > + if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) { We would have taken the return above if !rom_bar, so that test is unnecessary here. Thanks, Alex > + if (dev->opts && qemu_opt_get(dev->opts, "rombar")) { > + 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 non zero value for " > + "rombar\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; > + } > + } > + > DPRINTF("%04x:%02x:%02x.%x ROM size 0x%x\n", vdev->host.domain, > vdev->host.bus, vdev->host.slot, vdev->host.function, size); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms 2014-02-25 15:41 ` Alex Williamson @ 2014-02-25 16:51 ` Bandan Das 0 siblings, 0 replies; 6+ messages in thread From: Bandan Das @ 2014-02-25 16:51 UTC (permalink / raw) To: Alex Williamson Cc: Markus Armbruster, Michael S. Tsirkin, qemu-devel, Andreas Färber Alex Williamson <alex.williamson@redhat.com> writes: > On Mon, 2014-02-24 at 23:34 -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 >> change blacklists loading of rom for such cards unless the user >> specifies a romfile or rombar=1 on the cmd line >> >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 60 insertions(+) >> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c >> index 8db182f..df3ceee 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 VFIORomBlacklistEntry { >> + uint16_t vendor_id; >> + uint16_t device_id; >> +} VFIORomBlacklistEntry; >> + >> +static const VFIORomBlacklistEntry romblacklist[] = { >> + /* Broadcom BCM 57810 */ >> + { 0x14e4, 0x168e } >> +}; >> + > > Any progress on a bug reference or trying to extract a version from the > ROM so we can compare against future ROMs? I will get to it when there is actually a version of rom code that fixes it. AFAIK Broadcom is looking into it but I am not sure if there will be a fix. Comparing versions (or something else in the rom that can be compared) is probably not a very good idea considering there could be other cards in the wild with this issue and we then have to manage a three item list for all these cards - devid, vendorid, "version which fixed the problem", not to mention the comparision method might be different based on conventions that each vendor has adopted. (It appears I am seeing something similar with an Emulex card too!, still investigating) But anyway, I don't have a better idea to offer as of now. > We can always file a new bug > in launchpad for tracking if needed. Sure. Should I file a bug at https://bugs.launchpad.net/qemu/ and post a link to it in the code comments ? >> #define MSIX_CAP_LENGTH 12 >> >> static QLIST_HEAD(, VFIOContainer) >> @@ -1197,13 +1207,43 @@ static const MemoryRegionOps vfio_rom_ops = { >> .endianness = DEVICE_LITTLE_ENDIAN, >> }; >> >> +static bool vfio_blacklist_opt_rom(VFIODevice *vdev) >> +{ >> + PCIDevice *pdev = &vdev->pdev; >> + uint16_t 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(romblacklist)) { >> + if (romblacklist[count].vendor_id == vendor_id && >> + romblacklist[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; >> + DeviceState *dev = DEVICE(vdev); >> char name[32]; >> >> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) { >> + /* Since pci handles romfile, just print a message and return */ >> + if (vfio_blacklist_opt_rom(vdev) && 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; >> } >> >> @@ -1227,6 +1267,26 @@ static void vfio_pci_size_rom(VFIODevice *vdev) >> return; >> } >> >> + if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) { > > We would have taken the return above if !rom_bar, so that test is > unnecessary here. Thanks, Agreed, sorry! will fix in the next version. > Alex > >> + if (dev->opts && qemu_opt_get(dev->opts, "rombar")) { >> + 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 non zero value for " >> + "rombar\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; >> + } >> + } >> + >> DPRINTF("%04x:%02x:%02x.%x ROM size 0x%x\n", vdev->host.domain, >> vdev->host.bus, vdev->host.slot, vdev->host.function, size); >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-25 16:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-25 4:34 [Qemu-devel] [PATCH 0/2 v3] vfio: blacklist loading of unstable roms Bandan Das 2014-02-25 4:34 ` [Qemu-devel] [PATCH 1/2 v3] qdev-monitor: set DeviceState opts before calling realize Bandan Das 2014-02-25 7:39 ` Andreas Färber 2014-02-25 4:34 ` [Qemu-devel] [PATCH 2/2 v3] vfio: blacklist loading of unstable roms Bandan Das 2014-02-25 15:41 ` Alex Williamson 2014-02-25 16:51 ` Bandan Das
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.