| Message ID | 20240220-reuse-v6-13-2e42a28b0cf2@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
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.
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) { /*
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(+)