Re: [PATCH v6 13/15] hw/pci: Determine if rombar is explicitly enabled

[Akihiko Odaki]

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.