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 <c...@redhat.com>
> Signed-off-by: John Levon <john.le...@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: ");
