On Thu, Sep 18, 2025 at 02:53:00PM +0200, Peter Krempa wrote:
> On Tue, Aug 19, 2025 at 18:22:19 +0200, Andrea Bolognani via Devel wrote:
> > @@ -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?
Basically the way we perform address allocation and device validation
makes this incredibly tricky to put anywhere else.
Ideally you'd just interate over all PCI devices at validate time
and, if any exists but the machine type doesn't support PCI, you'd
report an error.
However the single source of truth for whether or not a device is PCI
are the virDomainPCIConnectFlags that get assigned to it. We don't
want to duplicate that information elsewhere.
Plus the fact that virtio devices exist make things even more
complicated. Is virtio-blk a PCI device? Well, yes. Unless you are on
Arm/RISC-V and you explicitly asked for it not to be by assigning a
virtio-mmio address to it. Or you're on s390x :)
Putting this check here means that the problematic scenarios are
naturally filtered out as part of the PCI address allocation process,
with no code duplication or possibility of something falling through
the cracks.
> > +++
> > 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.
The error is not necessarily great but you will only hit it when
attempting to do something genuinely invalid and PCI-related, such as
adding a USB or PCI controller to a machine that doesn't support PCI.
--
Andrea Bolognani / Red Hat / Virtualization
