Hi Akihiko,
On 10/2/24 11:24, Akihiko Odaki wrote:
> 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. PCIDevice::rom_bar is changed to
> have -1 by the default to tell rombar is explicitly enabled. It is
> consistent with other properties like addr and romsize.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> include/hw/pci/pci_device.h | 5 +++++
> hw/pci/pci.c | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> index d3dd0f64b273..54fa0676abf1 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;
Or return dev->rom_bar >= 0;
> +}
> +
> uint16_t pci_requester_id(PCIDevice *dev);
>
> /* DMA access functions */
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 76080af580d7..d08548d8ffe9 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, -1),
> - DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> + DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, -1),
You are changing a default affecting many devices. Please
document in the commit description it is safe because there
are few uses and you audited them:
$ git grep -w rom_bar
hw/display/qxl.c:328: QXLRom *rom =
memory_region_get_ram_ptr(&d->rom_bar);
hw/display/qxl.c:431: qxl_set_dirty(&qxl->rom_bar, 0, qxl->rom_size);
hw/display/qxl.c:1048: QXLRom *rom =
memory_region_get_ram_ptr(&qxl->rom_bar);
hw/display/qxl.c:2131: memory_region_init_rom(&qxl->rom_bar,
OBJECT(qxl), "qxl.vrom",
hw/display/qxl.c:2154:
PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->rom_bar);
hw/display/qxl.h:101: MemoryRegion rom_bar;
hw/pci/pci.c:74: DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
hw/pci/pci.c:2329: if (!pdev->rom_bar) {
hw/vfio/pci.c:1019: if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
hw/xen/xen_pt_load_rom.c:29: if (dev->romfile || !dev->rom_bar) {
include/hw/pci/pci_device.h:150: uint32_t rom_bar;
Alternatively split this patch in 2, first introduce
pci_rom_bar_explicitly_enabled() and use it, then change the default
value and adapt it.
Regards,
Phil.