On Fri, Sep 19, 2025 at 08:21:50 -0500, Andrea Bolognani wrote:
> 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.
I wanted to complain that there actually are multiple places that still
need fixing, namely hotplug, as that allocates addresses in a different
way (for each device individually). But looking a bit deeper it results
in the same code being called.
I still think that validation ought to be together with validation but
your explanation makes sense.
> > > +++
> > > 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
>
