Re: [PATCH 02/14] vfio: refactor out vfio_pci_config_setup()

[Tomita Moeko <tomitamoeko@gmail.com>]

Hi, John

On 4/9/25 21:48, John Levon wrote:
> Refactor the PCI config setup code out of vfio_realize() for
> readability.
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>  hw/vfio/pci.c | 176 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 94 insertions(+), 82 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 02f23efaba..81bf0dab28 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2963,6 +2963,99 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>      vdev->req_enabled = false;
>  }
>  
> +static bool vfio_pci_config_setup(VFIOPCIDevice *vdev, Error **errp)
> +{
> +    PCIDevice *pdev = &vdev->pdev;
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +
> +    /* vfio emulates a lot for us, but some bits need extra love */
> +    vdev->emulated_config_bits = g_malloc0(vdev->config_size);
> +
> +    /* QEMU can choose to expose the ROM or not */
> +    memset(vdev->emulated_config_bits + PCI_ROM_ADDRESS, 0xff, 4);
> +    /* QEMU can also add or extend BARs */
> +    memset(vdev->emulated_config_bits + PCI_BASE_ADDRESS_0, 0xff, 6 * 4);
> +
> +    /*
> +     * The PCI spec reserves vendor ID 0xffff as an invalid value.  The
> +     * device ID is managed by the vendor and need only be a 16-bit value.
> +     * Allow any 16-bit value for subsystem so they can be hidden or changed.
> +     */
> +    if (vdev->vendor_id != PCI_ANY_ID) {
> +        if (vdev->vendor_id >= 0xffff) {
> +            error_setg(errp, "invalid PCI vendor ID provided");
> +            return false;
> +        }
> +        vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
> +        trace_vfio_pci_emulated_vendor_id(vbasedev->name, vdev->vendor_id);
> +    } else {
> +        vdev->vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
> +    }
> +
> +    if (vdev->device_id != PCI_ANY_ID) {
> +        if (vdev->device_id > 0xffff) {
> +            error_setg(errp, "invalid PCI device ID provided");
> +            return false;
> +        }
> +        vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
> +        trace_vfio_pci_emulated_device_id(vbasedev->name, vdev->device_id);
> +    } else {
> +        vdev->device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
> +    }
> +
> +    if (vdev->sub_vendor_id != PCI_ANY_ID) {
> +        if (vdev->sub_vendor_id > 0xffff) {
> +            error_setg(errp, "invalid PCI subsystem vendor ID provided");
> +            return false;
> +        }
> +        vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
> +                               vdev->sub_vendor_id, ~0);
> +        trace_vfio_pci_emulated_sub_vendor_id(vbasedev->name,
> +                                              vdev->sub_vendor_id);
> +    }
> +
> +    if (vdev->sub_device_id != PCI_ANY_ID) {
> +        if (vdev->sub_device_id > 0xffff) {
> +            error_setg(errp, "invalid PCI subsystem device ID provided");
> +            return false;
> +        }
> +        vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
> +        trace_vfio_pci_emulated_sub_device_id(vbasedev->name,
> +                                              vdev->sub_device_id);
> +    }
> +
> +    /* QEMU can change multi-function devices to single function, or reverse */
> +    vdev->emulated_config_bits[PCI_HEADER_TYPE] =
> +                                              PCI_HEADER_TYPE_MULTI_FUNCTION;
> +
> +    /* Restore or clear multifunction, this is always controlled by QEMU */
> +    if (vdev->pdev.cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> +        vdev->pdev.config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    } else {
> +        vdev->pdev.config[PCI_HEADER_TYPE] &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    }
> +
> +    /*
> +     * Clear host resource mapping info.  If we choose not to register a
> +     * BAR, such as might be the case with the option ROM, we can get
> +     * confusing, unwritable, residual addresses from the host here.
> +     */
> +    memset(&vdev->pdev.config[PCI_BASE_ADDRESS_0], 0, 24);
> +    memset(&vdev->pdev.config[PCI_ROM_ADDRESS], 0, 4);
> +
> +    vfio_pci_size_rom(vdev);
> +
> +    vfio_bars_prepare(vdev);
> +
> +    if (!vfio_msix_early_setup(vdev, errp)) {
> +        return false;
> +    }
> +
> +    vfio_bars_register(vdev);

Probably we can also put `vfio_config_quirk_setup` here as it deals with
device-spcific config space.

I would personally prefer keeping `vfio_bars_register` in `vfio_realize`
so that it matches `vfio_bars_exit` at the end, just a minor nit.

Thanks,
Moeko

> +
> +    return true;
> +}
> +
>  static bool vfio_interrupt_setup(VFIOPCIDevice *vdev, Error **errp)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> @@ -3067,91 +3160,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>  
> -    /* vfio emulates a lot for us, but some bits need extra love */
> -    vdev->emulated_config_bits = g_malloc0(vdev->config_size);
> -
> -    /* QEMU can choose to expose the ROM or not */
> -    memset(vdev->emulated_config_bits + PCI_ROM_ADDRESS, 0xff, 4);
> -    /* QEMU can also add or extend BARs */
> -    memset(vdev->emulated_config_bits + PCI_BASE_ADDRESS_0, 0xff, 6 * 4);
> -
> -    /*
> -     * The PCI spec reserves vendor ID 0xffff as an invalid value.  The
> -     * device ID is managed by the vendor and need only be a 16-bit value.
> -     * Allow any 16-bit value for subsystem so they can be hidden or changed.
> -     */
> -    if (vdev->vendor_id != PCI_ANY_ID) {
> -        if (vdev->vendor_id >= 0xffff) {
> -            error_setg(errp, "invalid PCI vendor ID provided");
> -            goto error;
> -        }
> -        vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
> -        trace_vfio_pci_emulated_vendor_id(vbasedev->name, vdev->vendor_id);
> -    } else {
> -        vdev->vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
> -    }
> -
> -    if (vdev->device_id != PCI_ANY_ID) {
> -        if (vdev->device_id > 0xffff) {
> -            error_setg(errp, "invalid PCI device ID provided");
> -            goto error;
> -        }
> -        vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
> -        trace_vfio_pci_emulated_device_id(vbasedev->name, vdev->device_id);
> -    } else {
> -        vdev->device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
> -    }
> -
> -    if (vdev->sub_vendor_id != PCI_ANY_ID) {
> -        if (vdev->sub_vendor_id > 0xffff) {
> -            error_setg(errp, "invalid PCI subsystem vendor ID provided");
> -            goto error;
> -        }
> -        vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
> -                               vdev->sub_vendor_id, ~0);
> -        trace_vfio_pci_emulated_sub_vendor_id(vbasedev->name,
> -                                              vdev->sub_vendor_id);
> -    }
> -
> -    if (vdev->sub_device_id != PCI_ANY_ID) {
> -        if (vdev->sub_device_id > 0xffff) {
> -            error_setg(errp, "invalid PCI subsystem device ID provided");
> -            goto error;
> -        }
> -        vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
> -        trace_vfio_pci_emulated_sub_device_id(vbasedev->name,
> -                                              vdev->sub_device_id);
> -    }
> -
> -    /* QEMU can change multi-function devices to single function, or reverse */
> -    vdev->emulated_config_bits[PCI_HEADER_TYPE] =
> -                                              PCI_HEADER_TYPE_MULTI_FUNCTION;
> -
> -    /* Restore or clear multifunction, this is always controlled by QEMU */
> -    if (vdev->pdev.cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> -        vdev->pdev.config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> -    } else {
> -        vdev->pdev.config[PCI_HEADER_TYPE] &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> -    }
> -
> -    /*
> -     * Clear host resource mapping info.  If we choose not to register a
> -     * BAR, such as might be the case with the option ROM, we can get
> -     * confusing, unwritable, residual addresses from the host here.
> -     */
> -    memset(&vdev->pdev.config[PCI_BASE_ADDRESS_0], 0, 24);
> -    memset(&vdev->pdev.config[PCI_ROM_ADDRESS], 0, 4);
> -
> -    vfio_pci_size_rom(vdev);
> -
> -    vfio_bars_prepare(vdev);
> -
> -    if (!vfio_msix_early_setup(vdev, errp)) {
> +    if (!vfio_pci_config_setup(vdev, errp)) {
>          goto error;
>      }
>  
> -    vfio_bars_register(vdev);
> -
>      if (!vbasedev->mdev &&
>          !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
>          error_prepend(errp, "Failed to set vIOMMU: ");