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(-)

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",

-- 
2.45.2

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