[PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
Laine Stump
laine at redhat.com
Sun Sep 26 22:36:01 UTC 2021
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 at anisinha.ca>
> ---
> 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).
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. (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, 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.
> DO_TEST("q35-usb2",
> QEMU_CAPS_DEVICE_PCI_BRIDGE,
> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>
More information about the libvir-list
mailing list