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.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/pci/pci_device.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index ca151325085d..c4fdc96ef50d 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 != -1;
+}
+
 static inline void pci_set_power(PCIDevice *pci_dev, bool state)
 {
     /*

-- 
2.43.1

Akihiko Odaki <akihiko.odaki@daynix.com> 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.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  include/hw/pci/pci_device.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> index ca151325085d..c4fdc96ef50d 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 != -1;

Compares signed with unsigned: rom_bar is uint32_t, -1 is int.

If int is at most 32 bits, the comparison converts -1 to
(uint32_t)0xffffffff.

If int is wider, it converts dev->rom_bar to int without changing its
value.  In particular, it converts the default value 0xffffffff (written
as -1 in the previous patch) to (int)0xffffffff.  Oops.

Best not to mix signed and unsigned in comparisons.  Easy enough to
avoid here.

Still, we don't actually test "rom_bar has not been set".  We test
"rom_bar has the default value".  Nothing stops a user from passing
rombar=0xffffffff to -device.  See my review of the next patch.

> +}
> +
>  static inline void pci_set_power(PCIDevice *pci_dev, bool state)
>  {
>      /*

On 2024/02/21 17:15, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> 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.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   include/hw/pci/pci_device.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
>> index ca151325085d..c4fdc96ef50d 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 != -1;
> 
> Compares signed with unsigned: rom_bar is uint32_t, -1 is int.
> 
> If int is at most 32 bits, the comparison converts -1 to
> (uint32_t)0xffffffff.
> 
> If int is wider, it converts dev->rom_bar to int without changing its
> value.  In particular, it converts the default value 0xffffffff (written
> as -1 in the previous patch) to (int)0xffffffff.  Oops.
> 
> Best not to mix signed and unsigned in comparisons.  Easy enough to
> avoid here.
> 
> Still, we don't actually test "rom_bar has not been set".  We test
> "rom_bar has the default value".  Nothing stops a user from passing
> rombar=0xffffffff to -device.  See my review of the next patch.

I followed addr and romsize properties that already use -1 as the 
default value. addr is int32_t, but romsize is uint32_t. I'll change 
romsize and rom_bar to use UINT32_MAX as the default value.

This changes the behavior of 0xffffffff, but that should be fine. Most 
people should only set 0 or 1. Maybe someone types wrongly and sets a 
number like 2 or 12, but nobody types 0xffffffff by mistake.