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);
+
+ 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: ");
--
2.34.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: ");
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
© 2016 - 2025 Red Hat, Inc.