On Tue, Aug 19, 2025 at 18:22:19 +0200, Andrea Bolognani via Devel wrote:
> At the moment, we check whether the machine type supports PCI
> before attempting to allocate PCI addresses, and if it does
> not, we simply skip the step entirely.
>
> This means that an attempt to use a PCI device with a machine
> type that has no PCI support won't be rejected by libvirt, and
> only once the QEMU process is started the problem will be made
> apparent.
>
> Validate things ahead of time instead, rejecting any such
> configurations.
>
> Note that we only do this for new domains, because otherwise
> existing domains that are configured incorrectly would disappear
> and we generally try really hard to avoid that.
>
> A few tests start failing after this change, demonstrating that
> things are now working as desired.
>
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
> src/qemu/qemu_domain_address.c | 18 ++++++++---
> ...default-fallback-nousb.aarch64-latest.args | 32 -------------------
> ...-default-fallback-nousb.aarch64-latest.xml | 22 -------------
> .../usb-controller-default-fallback-nousb.xml | 1 -
> ...efault-nousb.aarch64-latest.abi-update.err | 1 +
> ...ntroller-default-nousb.aarch64-latest.args | 32 -------------------
> ...ontroller-default-nousb.aarch64-latest.err | 1 +
> ...fault-unavailable-nousb.aarch64-latest.err | 1 -
> ...fault-unavailable-nousb.aarch64-latest.xml | 22 -------------
> ...b-controller-default-unavailable-nousb.xml | 1 -
> tests/qemuxmlconftest.c | 14 ++------
> 11 files changed, 17 insertions(+), 128 deletions(-)
> delete mode 100644
> tests/qemuxmlconfdata/usb-controller-default-fallback-nousb.aarch64-latest.args
> delete mode 100644
> tests/qemuxmlconfdata/usb-controller-default-fallback-nousb.aarch64-latest.xml
> delete mode 120000
> tests/qemuxmlconfdata/usb-controller-default-fallback-nousb.xml
> create mode 100644
> tests/qemuxmlconfdata/usb-controller-default-nousb.aarch64-latest.abi-update.err
> delete mode 100644
> tests/qemuxmlconfdata/usb-controller-default-nousb.aarch64-latest.args
> create mode 100644
> tests/qemuxmlconfdata/usb-controller-default-nousb.aarch64-latest.err
> delete mode 100644
> tests/qemuxmlconfdata/usb-controller-default-unavailable-nousb.aarch64-latest.err
> delete mode 100644
> tests/qemuxmlconfdata/usb-controller-default-unavailable-nousb.aarch64-latest.xml
> delete mode 120000
> tests/qemuxmlconfdata/usb-controller-default-unavailable-nousb.xml
>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 07d6366b1b..312ed52bbd 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -2675,7 +2675,8 @@ static int
> qemuDomainAssignPCIAddresses(virDomainDef *def,
> virQEMUCaps *qemuCaps,
> virQEMUDriver *driver,
> - virDomainObj *obj)
> + virDomainObj *obj,
> + bool newDomain)
> {
> int ret = -1;
> virDomainPCIAddressSet *addrs = NULL;
> @@ -2843,10 +2844,17 @@ qemuDomainAssignPCIAddresses(virDomainDef *def,
> g_clear_pointer(&addrs, virDomainPCIAddressSetFree);
> }
>
> - if (!(addrs = qemuDomainPCIAddressSetCreate(def, qemuCaps, nbuses,
> false)))
> - goto cleanup;
> + /* We're done collecting available information, now we're going
> + * to allocate PCI addresses for real. We normally skip this part
> + * for machine type that don't support PCI, but we run it for new
> + * domains to catch situation in which the user is incorrectly
> + * asking for PCI devices to be used. If that's the case, an
> + * error will naturally be raised when attempting to allocate a
> + * PCI address since no PCI buses exist */
> + if (qemuDomainSupportsPCI(def) || newDomain) {
> + if (!(addrs = qemuDomainPCIAddressSetCreate(def, qemuCaps, nbuses,
> false)))
> + goto cleanup;
Any reason why this is here and not in e.g qemuValidateDomainDef() which
based on the description seems like the proper place?
>
> - if (qemuDomainSupportsPCI(def)) {
> if (qemuDomainValidateDevicePCISlotsChipsets(def, addrs) < 0)
> goto cleanup;
>
> --- /dev/null
> +++
> b/tests/qemuxmlconfdata/usb-controller-default-nousb.aarch64-latest.abi-update.err
> @@ -0,0 +1 @@
> +XML error: No PCI buses available
At this point it's IMO borderline whether it's a XML error or not. It's
a configuration error but some configurations which don't mention PCI
do get it normally.
Although this is pre-existing and cosmetic.
