Re: [PATCH v2 1/6] hw/pci: Determine if rombar is explicitly enabled

[Philippe Mathieu-Daudé]

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.