Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
On 9/27/21 12:54 AM, Ani Sinha wrote:
On Sun, 26 Sep 2021, Laine Stump wrote:
+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 :-)
Yes, now that there is a settable property its default can be changed,
but we do know that on all versions of QEMU that don't support setting
this property, hotplug is always enabled for pci-root. Anyway, this
isn't important - we can always relax it later if someone really wants it.
Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
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:
> >
> >
> >
> >
> >
> > '' 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=
> >
> > 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
> > ---
> > 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.
Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
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:
'' 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=
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
---
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 00..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
[PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
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:
'' 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=
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
---
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));
+}
+}
+
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 00..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-ad646a1c \
+-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 00..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,
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);
+DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-enable");
DO_TEST("q35-usb2",
QEMU_CAPS_DEVICE_PCI_BRIDGE,
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
--
2.25.1
