Akihiko Odaki <akihiko.od...@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 <arm...@redhat.com>
> Signed-off-by: Akihiko Odaki <akihiko.od...@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 <arm...@redhat.com>
