[PATCH] qemu: add pcie-to-pci-bridge for q35 machines

[Peter Krempa]

On Fri, Nov 04, 2022 at 14:16:44 +0100, Denis V. Lunev wrote:
> On 11/4/22 13:03, Peter Krempa wrote:
> > On Fri, Nov 04, 2022 at 16:43:00 +0600, Oleg Vasilev wrote:
> > > Hotplugging PCI devices on pc-i440fx machines is supported without
> > > additional configuration. On q35, pcie-to-pci-bridge needs to be added
> > > prior to the machine startup in order to support hotplug [1].
> > > 
> > > The idea is to support the same workflow for creating VMs in q35, as was
> > > in pc. Namely, do not require additional configuration when hotplugging
> > > is needed. Otherwise, all libvirt clients need to be updated which there
> > > are a lot and they are maintained by different parties.
> > > 
> > > Instead, a pcie-to-pci-bridge better be created by default, so that PCI
> > > slots are readily available. Might be a good idea to make it configurable
> > > in the future.
> > > 
> > > Previously there was a pci-bridge present by default, but was removed in
> > >    d5fb8f4564 (qemu: don't add pci-bridge to Q35/arm domains unless it's needed, 2016-04-22)
> > > 
> > > [1]: https://libvirt.org/pci-hotplug.html
> > > 
> > > Signed-off-by: Oleg Vasilev <oleg.vasilev at virtuozzo.com>
> > > ---
> > [...]
> > 
> > >   73 files changed, 621 insertions(+), 361 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 9ef6c8bb64..52a393e07f 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -3890,6 +3890,7 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> > >       bool addImplicitSATA = false;
> > >       bool addPCIRoot = false;
> > >       bool addPCIeRoot = false;
> > > +    bool addPCIeToPCIBridge = false;
> > >       bool addDefaultMemballoon = true;
> > >       bool addDefaultUSBKBD = false;
> > >       bool addDefaultUSBMouse = false;
> > > @@ -3910,6 +3911,8 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> > >           if (qemuDomainIsQ35(def)) {
> > >               addPCIeRoot = true;
> > >               addImplicitSATA = true;
> > > +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE))
> > > +                addPCIeToPCIBridge = true;
> > >               /* Prefer adding a USB3 controller if supported, fall back
> > >                * to USB2 if there is no USB3 available, and if that's
> > > @@ -4046,11 +4049,7 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> > >           }
> > >       }
> > > -    /* When a machine has a pcie-root, make sure that there is always
> > > -     * a dmi-to-pci-bridge controller added as bus 1, and a pci-bridge
> > > -     * as bus 2, so that standard PCI devices can be connected
> > > -     *
> > > -     * NB: any machine that sets addPCIeRoot to true must also return
> > > +    /* NB: any machine that sets addPCIeRoot to true must also return
> > >        * true from the function qemuDomainSupportsPCI().
> > >        */
> > >       if (addPCIeRoot) {
> > > @@ -4069,6 +4068,16 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
> > >           }
> > >       }
> > > +    /* Add a pcie-to-pci-bridge and pcie-root-port to plug it into. */
> > > +    if (addPCIeToPCIBridge) {
> > > +        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1,
> > > +                                       VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT) < 0)
> > > +            return -1;
> > > +        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 2,
> > > +                                       VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE) < 0)
> > > +            return -1;
> > > +    }
> > Doing this unconditionally breaks ABI of the VM, as new guest visible
> > devices appear without any change in configuration. What's worse this
> > implementation would also break migration compatibility from older
> > versions of libvirt as there isn't any form of explicit handling for it.
> 
> Hmm, I am not so sure here. Doing this we goes exactly in the same way
> as done for balloon and thus my expectations are that the device will
> appear in the configuration.
> 
> Quick check over domain.xml seems that my opinion is correct. I am
> able to find the following device
> 
>     <controller type='pci' index='2' model='pcie-to-pci-bridge'>
>       <model name='pcie-pci-bridge'/>
>       <alias name='pci.2'/>
>       <address type='pci' domain='0x0000' bus='0x01' slot='0x00'
> function='0x0'/>
>     </controller>

Note that balloon is not a good example here. In fact it's a bit of a
counter example to the case of yours.

The balloon device was historically auto-added by qemu unless
specifically disabled. Libvirt implemented the support for controlling
ballon some time later.

This meant that there were libvirt XMLs which resulted in the balloon
device being present in the VM ABI. To correct this we auto-add the XML
bit as we historically did. If you want to disable the balloon you must
explicitly request a config where it is disabled.

Now in the case presented here we did not historically add the device so
we must not do that unless explcitly configured.

> > I unfortunately don't see any reasonable solution originating in
> > libvirt. The bridge will need to be added by users or higher level
> > management tools (e.g. virt-install) as libvirt explicitly wants to
> > support use cases when VMs are started without persisting the XML
> > definition (e.g. always re-define the XML before start or use the
> > 'createXML' api).
> > 
> That approach seems cumbersome. Even in Virtuozzo we have
> * QA test client, which creates VMs on its own
> * 'virt-install' based software
> * 'prl-disp-service' based software
> * OpenStack based clusters
> 
> With your proposal we would need to implement a code,
> which will check that the VM is created with Q35
> machine type and in that case the bridge is added
> each time the virtual machine is created. There are a lot
> of software doing that and the process seems to complex.
> In general, old PC based software have supported ordinary
> VirtIO network device hotplug without any VM specific
> pre-configuration and that was brilliant. Please correct
> me if I am wrong.
> 
> Proposed approach requires a lot of efforts and seems
> error prone.
> 
> On the other hand I could propose at least 2 options:
> * hotplugging of PCIe-to-PCI bridge at the time
>    of the first hotlug to Q35 VM

This should be okay because an explicit configuration change request
will cause it.

> * put the behavior of creating default bridge under
>    the option in libvirt configuration

If by "libvirt configuration" you mean putting it into the
/etc/libvirt/qemu.conf or similar that is not acceptable, because it has
the same set of problems with changing ABI.

The only reasonable place is somewhere in the VM xml. In fact we already
provide that if you specify the bridge manually but I guess some form of
syntax sugar to add the bridge could be (reluctantly) acceptable.

> In general, hotplugging of bridge seems much more
> guest specific but potentially could work.