On Sun, 26 Sep 2021, Laine Stump wrote:
> On 9/26/21 10:35 AM, Ani Sinha wrote:
> > This change adds qemu backend command line support for enabling or disabling
> > hotplug on the pci-root controller using the 'target' sub-element of the
> > pci-root controller as shown below:
> >
> > <controller type='pci' model='pci-root'>
> > <target hotplug='off'/>
> > </controller>
> >
> > '<target hotplug='off/on'/>' is only valid for pc (x86) machines and turns
> > on
> > the following command line option that is passed to qemu for x86 guests:
> >
> > -global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
> >
> > This change also adds the required qemuxml2argv unit tests in order to test
> > correct qemu arguments. Unit tests have also been added to test qemu
> > capability
> > validation checks.
> >
> > Signed-off-by: Ani Sinha <ani@xxxxxxxxxxx>
> > ---
> > src/qemu/qemu_command.c | 12 +++++++
> > .../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++++++++++++++++++
> > .../pc-i440fx-acpi-root-hotplug-enable.err | 1 +
> > tests/qemuxml2argvtest.c | 6 ++++
> > 4 files changed, 50 insertions(+)
> > create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
> > create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index b60ee1192b..0cdc10bf29 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -6029,6 +6029,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
> > qemuDomainObjPrivate *priv)
> > {
> > virQEMUCaps *qemuCaps = priv->qemuCaps;
> > + int n;
> > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
> > /* with new qemu we always want '-no-shutdown' on startup and we
> > set
> > @@ -6074,6 +6075,17 @@ qemuBuildPMCommandLine(virCommand *cmd,
> > pm_object, def->pm.s4 ==
> > VIR_TRISTATE_BOOL_NO);
> > }
> > + for (n = 0; n < def->ncontrollers; n++) {
> > + virDomainControllerDef *cdef = def->controllers[n];
> > + if (cdef->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> > + cdef->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> > + cdef->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> > + virCommandAddArg(cmd, "-global");
> > + virCommandAddArgFormat(cmd,
> > "PIIX4_PM.acpi-root-pci-hotplug=%s",
> > +
> > virTristateSwitchTypeToString(cdef->opts.pciopts.hotplug));
> > + }
> > + }
> > +
>
> It would make more sense to include this in the commandline generator for PCI
> controllers, since that's where all other XML for PCI controllers is converted
> into a qemu commandline argument. That's a bit convoluted though,
> unfortunately.
>
> The loop going through all the controllers is in
> qemuBuildControllersByTypeCommandLine() which then calls
> qemuBuildControllerDevStr() and *that* is the function that builds the
> argument to the -device for each controller. For two reasons, we can't add the
> code for this new option alongside the other PCI controller commandline
> generation code in qemuBuildControllerDevStr() though:
>
> 1) the output of qemuBuildControllerDevStr() is assumed to be following a
> "-device" argument on the commandline, so we would end up with:
>
> "-device -global PIIX4_PM.acpi-root-pci-hotplug=off"
>
> which is *not* what we want.
>
> 2) the top of the loop in qemuBuildControllersByTypeCommandLine() calls
> qemuBuildSkipController(), which tells the loop to skip generating any
> commandline for a pci-root controller (unless it's a P-series guest and the
> index is non-0).
>
Yes I saw this code and debated whether to put here or from the function
that adds PM commandlines. I put it there because I thought all PM related
options should go together. Also I did see qemuBuildSkipController() that
would have skipped through the pci-root controllers and I was not sure
philosophically if putting it qemuBuildControllersByTypeCommandLine()
would be right.
The loop yes, that was a bummer and I literally copied the loop over the
controllers from this code in qemuBuildControllersByTypeCommandLine().
> So the only way to make this work is to add a special case to
> qemuBuildControllersByTypeCommandLine() *before* the call to
> qemuBuildSkipController() - if the type is pci, model is pci-root, and index
> is 0 then conditionally add "-global", "PIIX4...." and continue (skip the rest
> of the body of the loop).
>
> As with what you've already done here, this is also not 100% clean and
> consistent with the rest of the code, but since neither method is perfect I
> think putting it in the function that generates all the rest of the
> commandline args associated with PCI controllers is more logical and easier to
> follow.
OK now in v5 I have written it that way and I do realize that not having
to put another loop is a lot cleaner. Since you prefer this approach I
have made the changes in v5. I think its a tad bit better and simpler too.
(If anyone disagrees and thinks that the commandline for this should
> be generated in the place this patch already does it, please speak up!)
>
>
> > return 0;
> > }
> > diff --git
> > a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
> > b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
> > new file mode 100644
> > index 0000000000..1fb68381d9
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
> > @@ -0,0 +1,31 @@
> > +LC_ALL=C \
> > +PATH=/bin \
> > +HOME=/tmp/lib/domain--1-i440fx \
> > +USER=test \
> > +LOGNAME=test \
> > +XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
> > +XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
> > +XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
> > +QEMU_AUDIO_DRV=none \
> > +/usr/bin/qemu-system-x86_64 \
> > +-name guest=i440fx,debug-threads=on \
> > +-S \
> > +-object
> > secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes
> > \
> > +-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
> > +-m 1024 \
> > +-realtime mlock=off \
> > +-smp 1,sockets=1,cores=1,threads=1 \
> > +-uuid 56f5055c-1b8d-490c-844a-ad646a1caaaa \
> > +-display none \
> > +-no-user-config \
> > +-nodefaults \
> > +-chardev
> > socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off
> > \
> > +-mon chardev=charmonitor,id=monitor,mode=control \
> > +-rtc base=utc \
> > +-no-shutdown \
> > +-no-acpi \
> > +-global PIIX4_PM.acpi-root-pci-hotplug=off \
> > +-boot strict=on \
> > +-usb \
> > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
> > +-msg timestamp=on
> > diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
> > b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
> > new file mode 100644
> > index 0000000000..827c93a7ba
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
> > @@ -0,0 +1 @@
> > +unsupported configuration: setting the hotplug property on a pci-root
> > device is not supported by this QEMU binary
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 13e387df3f..e1b07f591f 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -2571,6 +2571,12 @@ mymain(void)
> > QEMU_CAPS_DEVICE_IOH3420,
> > QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI,
> > QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4);
> > + DO_TEST("pc-i440fx-acpi-root-hotplug-disable",
> > + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> > + QEMU_CAPS_DEVICE_IOH3420,
> > + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>
> Are these three caps ^^^ actually needed? I hope not,
Hmm. Weird. When I was experimenting it seemed it was needed. However, I
removed them and the unit tests still passed. So I have removed them in
v5.
> because that would
> indicate something seriously wrong with the code (those caps are related to
> PCIe controllers, and this option only applies to machinetypes that use
> conventional pci-root).
>
> > + QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);
> > + DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-enable");
>
> Ah, now I understand the origin of pc-i440fx-acpi-root-hotplug-enable.xml in
> the previous patch - you had created it to be test case for this negative
> test. Actually I think the negative test should be done using the ...-disable
> test case with no caps; in the case that someone has "hotplug='on'" and the
> option is missing, I think we should just ignore it, since "hotplug='on'" is
> what they're going to get anyway (that could be added into the validation that
> was added in the previous patch). You still you add the -enable test case to
> qemuxml2xmltest.c as I suggested in the previous patch.
No. Lets make hotplug=on/off explicit either way. Qemu defaults keep
changing :-)
I have added the additional unit test in v5.
