diff mbox series

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

Message ID 20250409134814.478903-3-john.levon@nutanix.com (mailing list archive)
State New
Headers show
Series vfio: preparation for vfio-user | expand

Commit Message

John Levon April 9, 2025, 1:48 p.m. UTC 
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(-)

Comments

Tomita Moeko April 9, 2025, 3:38 p.m. UTC | #1 
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: ");
John Levon April 9, 2025, 3:41 p.m. UTC | #2 
On Wed, Apr 09, 2025 at 11:38:01PM +0800, Tomita Moeko wrote:

> On 4/9/25 21:48, John Levon wrote:
> > Refactor the PCI config setup code out of vfio_realize() for
> > readability.

> > +    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.

This should be harmless (I think), but...

> 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.

... this means that vfio-user (when it exists) can't re-use the function, which
is the underlying reason for this refactoring originally.

regards
john
diff mbox series

Patch

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);
+
+    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: ");