| Message ID | 20240802-reuse-v11-10-fb83bb8c19fb@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > vfio_pci_size_rom() distinguishes whether rombar is explicitly set to 1 > by checking dev->opts, bypassing the QOM property infrastructure. > > Use -1 as the default value for rombar to tell if the user explicitly > set it to 1. The property is also converted from unsigned to signed. > -1 is signed so it is safe to give it a new meaning. The values in > [2 ^ 31, 2 ^ 32) will be invalid, but nobody should have typed these > values by chance. s/will be/become invalid/ Should we document the change somewhere? I'm not sure. Opinions? > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > include/hw/pci/pci_device.h | 2 +- > hw/pci/pci.c | 2 +- > hw/vfio/pci.c | 5 ++--- > 3 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h > index 1ff3ce94e25b..8fa845beee5e 100644 > --- a/include/hw/pci/pci_device.h > +++ b/include/hw/pci/pci_device.h > @@ -148,7 +148,7 @@ struct PCIDevice { > uint32_t romsize; > bool has_rom; > MemoryRegion rom; > - uint32_t rom_bar; > + int32_t rom_bar; > > /* INTx routing notifier */ > PCIINTxRoutingNotifier intx_routing_notifier; > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 4c7be5295110..d2eaf0c51dde 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -71,7 +71,7 @@ static Property pci_props[] = { > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX), > - DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), > + DEFINE_PROP_INT32("rombar", PCIDevice, rom_bar, -1), > DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, > QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), > DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present, > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 2407720c3530..dc53837eac73 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1012,7 +1012,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *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; > int fd = vdev->vbasedev.fd; > > @@ -1046,12 +1045,12 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) > } > > if (vfio_opt_rom_in_denylist(vdev)) { > - if (dev->opts && qdict_haskey(dev->opts, "rombar")) { > + if (vdev->pdev.rom_bar > 0) { > warn_report("Device at %s is known to cause system instability" > " issues during option rom execution", > vdev->vbasedev.name); > error_printf("Proceeding anyway since user specified" > - " non zero value for rombar\n"); > + " positive value for rombar\n"); > } else { > warn_report("Rom loading for device at %s has been disabled" > " due to system instability issues", Preferably with the commit message tweak: Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 2024/08/02 19:54, Markus Armbruster wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> vfio_pci_size_rom() distinguishes whether rombar is explicitly set to 1 >> by checking dev->opts, bypassing the QOM property infrastructure. >> >> Use -1 as the default value for rombar to tell if the user explicitly >> set it to 1. The property is also converted from unsigned to signed. >> -1 is signed so it is safe to give it a new meaning. The values in >> [2 ^ 31, 2 ^ 32) will be invalid, but nobody should have typed these >> values by chance. > > s/will be/become invalid/ I'll make this change in the next version. > > Should we document the change somewhere? I'm not sure. Opinions? I think it is fine whether it is documented or not. As the commit message says, I expect nobody will be impacted with this change. But documenting this change will hurt nobody either. Regards, Akihiko Odaki
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h index 1ff3ce94e25b..8fa845beee5e 100644 --- a/include/hw/pci/pci_device.h +++ b/include/hw/pci/pci_device.h @@ -148,7 +148,7 @@ struct PCIDevice { uint32_t romsize; bool has_rom; MemoryRegion rom; - uint32_t rom_bar; + int32_t rom_bar; /* INTx routing notifier */ PCIINTxRoutingNotifier intx_routing_notifier; diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 4c7be5295110..d2eaf0c51dde 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -71,7 +71,7 @@ static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), DEFINE_PROP_STRING("romfile", PCIDevice, romfile), DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX), - DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), + DEFINE_PROP_INT32("rombar", PCIDevice, rom_bar, -1), DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present, diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 2407720c3530..dc53837eac73 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1012,7 +1012,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *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; int fd = vdev->vbasedev.fd; @@ -1046,12 +1045,12 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) } if (vfio_opt_rom_in_denylist(vdev)) { - if (dev->opts && qdict_haskey(dev->opts, "rombar")) { + if (vdev->pdev.rom_bar > 0) { warn_report("Device at %s is known to cause system instability" " issues during option rom execution", vdev->vbasedev.name); error_printf("Proceeding anyway since user specified" - " non zero value for rombar\n"); + " positive value for rombar\n"); } else { warn_report("Rom loading for device at %s has been disabled" " due to system instability issues",
vfio_pci_size_rom() distinguishes whether rombar is explicitly set to 1 by checking dev->opts, bypassing the QOM property infrastructure. Use -1 as the default value for rombar to tell if the user explicitly set it to 1. The property is also converted from unsigned to signed. -1 is signed so it is safe to give it a new meaning. The values in [2 ^ 31, 2 ^ 32) will be invalid, but nobody should have typed these values by chance. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/hw/pci/pci_device.h | 2 +- hw/pci/pci.c | 2 +- hw/vfio/pci.c | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-)