On 9/26/21 10:35 AM, Ani Sinha wrote:

This change introduces libvirt xml support to enable/disable hotplug on the pci-root controller. It adds a 'target' subelement for the pci-root controller with a 'hotplug' property. This property can be used to enable or disable hotplug for the pci-root controller. For example, in order to disable hotplug on the pci-root controller, one has to use set '<target hotplug='off'>' as shown below:

<controller type='pci' model='pci-root'> <target hotplug='off'/> </controller>

'<target hotplug='on'>' option would enable hotplug for pci-root controller. This is also the default value. This option is only available for pc machine types and is applicable for qemu only/kvm accelerator onlt.This feature was introduced from qemu version 5.2 with the following change in qemu repository:

3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")

The above qemu commit describes some reasons why users might to disable hotplug on PCI root buses.

Related unit tests to exercise the new conf option has also been added.

Signed-off-by: Ani Sinha <ani@anisinha.ca> --- docs/formatdomain.rst | 12 ++++---- docs/schemas/domaincommon.rng | 10 +++++++ src/qemu/qemu_validate.c | 9 +++++- .../pc-i440fx-acpi-root-hotplug-disable.xml | 17 +++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 17 +++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++++++++++++++++++ tests/qemuxml2xmltest.c | 2 ++ 7 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml create mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 0f5d833521..e2a1768ba7 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).` controller's "port" configuration value, which is visible to the virtual machine. If set, port must be between 0 and 255. ``hotplug`` - pcie-root-port and pcie-switch-downstream-port controllers can also have a - ``hotplug`` attribute in the ``<target>`` subelement, which is used to - disable hotplug/unplug of devices on a particular controller. The default - setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable - hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0` + pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and + pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can + also have a ``hotplug`` attribute in the ``<target>`` subelement, which is + used to disable hotplug/unplug of devices on a particular controller. The + default setting of ``hotplug`` is ``on``; it should be set to ``off`` to + disable hotplug/unplug of devices on a particular controller. + ``busNr`` pci-expander-bus and pcie-expander-bus controllers can have an optional ``busNr`` attribute (1-254). This will be the bus number of the new bus; All diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fdc04f90aa..cce50c9c89 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2668,6 +2668,16 @@ <ref name="scaledInteger"/> </element> </optional> + <!-- pci-root controller has an optional element "target" --> + <optional> + <element name="target"> + <optional> + <attribute name="hotplug"> + <ref name="virOnOff"/> + </attribute> + </optional> + </element> + </optional> </group> <group> <attribute name="model">

This chunk ^^^^ is unnecessary, since the schema already allows for a target/hotplug attribute for *all* PCI controller models (on line 2645), so this new bit in the scheme has no effect. (qemuValidateDomainDeviceDefControllerPCI is doing a more specific check to assure nobody tries to specify the hotplug attribute for controller models that don't support it, and you've added the proper check there)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 13fbfd01b2..510e766cfd 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, /* hotplug */ if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) { switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting the %s property on a pci-root device is not supported by this QEMU binary"), + "hotplug"); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) { @@ -3889,7 +3897,6 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, } break;

- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml new file mode 100644 index 0000000000..f6a4c4b16a --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>i440fx</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + <boot dev='network'/> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' model='pci-root'> + <target hotplug='off'/> + </controller> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml new file mode 100644 index 0000000000..90a568cd9b --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>i440fx</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + <boot dev='network'/> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' model='pci-root'> + <target hotplug='on'/> + </controller> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml

It makes for less maintenance (and a *miniscule* reduction in disk space used :-)) if you populate the source xml in qemuxml2argvdata with all the extra stuff that's added by the parser (ie, just copy the file from qemuxml2xmloutdata to qemuxml2argvdata) and then make the file in qemuxml2argvdata a symlink to the original file. That way if something changes in the source XML, the output XML is automatically updated.

new file mode 100644 index 0000000000..93f2779f68 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>i440fx</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + <boot dev='network'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'> + <target hotplug='off'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 49b291fadb..ef45e4d9f1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -424,6 +424,8 @@ mymain(void) DO_TEST_NOCAPS("input-usbtablet"); DO_TEST_NOCAPS("misc-acpi"); DO_TEST("misc-disable-s3", QEMU_CAPS_PIIX_DISABLE_S3); + DO_TEST("pc-i440fx-acpi-root-hotplug-disable", + QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);

You created a qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml testfile, but forgot to add it to qemuxml2xmltest.c. (again, once you do that and create the

DO_TEST("misc-disable-suspends", QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4);

So 3 items: 1) get rid of unnecessary new chunk in domaincommon.rng 2) add pc-i440fx-acpi-root-hotplug-enable test to qemuxml2xmltest.c 3) make the 2 new xml files in qemuxml2xmloutdata into symlinks to their (fully populated) mates in qemuxml2argvdata.

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@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).

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.