Re: [PATCH v8 14/15] hw/pci: Determine if rombar is explicitly enabled

[Markus Armbruster]

Akihiko Odaki  writes:

> vfio determines if rombar is explicitly enabled by inspecting QDict.
> Inspecting QDict is not nice because QDict is untyped and depends on the
> details on the external interface. Add an infrastructure to determine if
> rombar is explicitly enabled to hw/pci.
>
> This changes the semantics of UINT32_MAX, which has always been a valid
> value to explicitly say rombar is enabled to denote the implicit default
> value. Nobody should have been set UINT32_MAX to rombar however,
> considering that its meaning was no different from 1 and typing a
> literal UINT32_MAX (0x or 4294967295) is more troublesome.
>
> Signed-off-by: Akihiko Odaki 

Remark on the previous patch applies.

Reviewed-by: Markus Armbruster 

[PATCH v8 14/15] hw/pci: Determine if rombar is explicitly enabled

[Akihiko Odaki]

vfio determines if rombar is explicitly enabled by inspecting QDict.
Inspecting QDict is not nice because QDict is untyped and depends on the
details on the external interface. Add an infrastructure to determine if
rombar is explicitly enabled to hw/pci.

This changes the semantics of UINT32_MAX, which has always been a valid
value to explicitly say rombar is enabled to denote the implicit default
value. Nobody should have been set UINT32_MAX to rombar however,
considering that its meaning was no different from 1 and typing a
literal UINT32_MAX (0x or 4294967295) is more troublesome.

Signed-off-by: Akihiko Odaki 
---
 include/hw/pci/pci_device.h | 5 +
 hw/vfio/pci.c   | 3 +--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index ca151325085d..6be0f989ebe0 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -205,6 +205,11 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
 return PCI_BUILD_BDF(pci_bus_num(pci_get_bus(dev)), dev->devfn);
 }
 
+static inline bool pci_rom_bar_explicitly_enabled(PCIDevice *dev)
+{
+return dev->rom_bar && dev->rom_bar != UINT32_MAX;
+}
+
 static inline void pci_set_power(PCIDevice *pci_dev, bool state)
 {
 /*
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fa387f0430d..647f15b2a060 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,7 +1045,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
 }
 
 if (vfio_opt_rom_in_denylist(vdev)) {
-if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
+if (pci_rom_bar_explicitly_enabled(>pdev)) {
 warn_report("Device at %s is known to cause system instability"
 " issues during option rom execution",
 vdev->vbasedev.name);

-- 
2.43.2