On Tue, Aug 19, 2025 at 18:22:22 +0200, Andrea Bolognani via Devel wrote:
> These checks enforce some expectations that were, until now,
> documented solely through comments or not spelled out at all.
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/qemu/qemu_postparse.c | 38 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
> index ab39dfe138..e2004993f3 100644
> --- a/src/qemu/qemu_postparse.c
> +++ b/src/qemu/qemu_postparse.c
> @@ -1383,6 +1383,41 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> break;
> }
>
> + /* Sanity check. If the machine type does not support PCI, asking
> + * for PCI(e) root to be added is an obvious mistake */
> + if ((addPCIRoot ||
> + addPCIeRoot) &&
> + !qemuDomainSupportsPCI(def)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Machine type '%1$s' wants PCI but PCI is not supported"),
> + def->os.machine);
> + return -1;
> + }
I agree with this one.
> +
> + /* Sanity check. If the machine type supports PCI, we need to reflect
> + * that fact in the XML or other parts of the machine handling code
> + * might misbehave */
This one is a bit borderline. IMO you can have machine with no default
PCI but the possibility to explicily add such a bus.
> + if (qemuDomainSupportsPCI(def) &&
> + !addPCIRoot &&
> + !addPCIeRoot) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Machine type '%1$s' supports PCI but no PCI controller added"),
> + def->os.machine);
> + return -1;
> + }
> +
> + /* Sanity check. USB controllers are PCI devices, so asking for a
> + * USB controller to be added but not for a PCI(e) root to be
> + * added at the same time is likely an oversight */
I'm fairly sure there are non-PCI USB controllers so this one is
definitely false. It's just that libvirt supports only USB controllers
which are on PCI.
IMO this assumption should be validated with the USB controller itself.
> + if (addDefaultUSB &&
> + !addPCIRoot &&
> + !addPCIeRoot) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Machine type '%1$s' wants USB but no PCI controller added"),
> + def->os.machine);
> + return -1;
> + }
> +
> if (addDefaultUSB && virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0)
> virDomainDefAddUSBController(def, 0, usbModel);
>
> @@ -1391,9 +1426,6 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
>
> pciRoot = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0);
>
> - /* NB: any machine that sets addPCIRoot to true must also return
> - * true from the function qemuDomainSupportsPCI().
> - */
> if (addPCIRoot) {
> if (pciRoot >= 0) {
> if (def->controllers[pciRoot]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
> --
> 2.50.1
>