Re: [PATCH v6 3/4] qemu: command: add support for acpi-bridge-hotplug feature
On Fri, 8 Oct 2021, Laine Stump wrote:
> On 10/5/21 1:51 AM, Ani Sinha wrote:
> > This change adds backend qemu command line support for new libvirt global
> > feature 'acpi-bridge-hotplug'. This option can be used as following:
> >
> >
> >
> >
> >
> >
> >
> > The '' sub-element under '' is also newly introduced.
> >
> > 'acpi-bridge-hotplug' turns on the following command line option to qemu for
> > x86 guests:
> >
> > (pc machines): -global
> > PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
> > (q35 machines): -global
> > ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
> >
> > 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 as well as checks for using this option with the right
> > architecture.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/qemu/qemu_command.c | 14
> > .../aarch64-acpi-hotplug-bridge-disable.err | 1 +
> > ...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 +
> > .../pc-i440fx-acpi-hotplug-bridge-disable.err | 1 +
> > .../q35-acpi-hotplug-bridge-disable.args | 33 +++
> > .../q35-acpi-hotplug-bridge-disable.err | 1 +
> > tests/qemuxml2argvtest.c | 16 +
> > 7 files changed, 97 insertions(+)
> > create mode 100644
> > tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
> > create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
> > create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
> > create mode 100644
> > tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
> > create mode 100644
> > tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 08f6d735f8..6c8c99e595 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -6069,6 +6069,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
> > qemuDomainObjPrivate *priv)
> > {
> > virQEMUCaps *qemuCaps = priv->qemuCaps;
> > +int acpihp_br =
> > def->pci_features[(virDomainPCI)VIR_DOMAIN_PCI_ACPI_BRIDGE_HP];
>
>
> I'm not sure why you put the typecast on the array subscript, but it's
> unnecessary. I removed it.
Fixed in v7.
>
>
> > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
> > /* with new qemu we always want '-no-shutdown' on startup and we
> > set
> > @@ -6114,6 +6115,19 @@ qemuBuildPMCommandLine(virCommand *cmd,
> > pm_object, def->pm.s4 ==
> > VIR_TRISTATE_BOOL_NO);
> > }
> > +if (acpihp_br) {
> > +const char *pm_object = "PIIX4_PM";
> > +
> > +if (qemuDomainIsQ35(def) &&
> > +virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
> > +pm_object = "ICH9-LPC";
>
> So on Q35 systems that don't support setting "ICH9-LPC.acpi-pci-hotplug."
> it will also work to set "PIIX4_PM.acpi-pci-"? (apparently this is the
> case with the s3 and s4 acpi suspend options, which have a similar hack a few
> lines below this one).
>
> Assuming that is the case, then this patch also seems fine to me
>
No, that is not right. I have fixed it in v7. Thanks for catching it.
> Reviewed-by: Laine Stump
>
> I may want to make the documentation in formatdomain.rst a bit more clear, but
> I can do that as a separate patch after these (since I know you're leaving on
> a vacation so won't be around to do it).
Yeah I could not handle it well. I tried my best but left the rest to the
experts here :-)
>
> As long as nobody complains about the naming of the feature,
If by naming if you mean the domain xml config options, that is approved
by danpb (offline IRC chat with him).
I think I can
> push this tomorrow evening (U.S. time) (that will give time for any discussion
> about naming in the morning, then I'll have to be afk for a few hours in the
> afternoon.
>
> Thanks for your contribution, and for your perseverance in the face of my
> procrastination, and enjoy your vacation!!
Thanks Laine for so much help. Very much appretiate your suggestions and
guidance here.
>
> > +
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd,
> > "%s.acpi-pci-hotplug-with-bridge-support=%s",
> > + pm_object,
> > + virTristateSwitchTypeToString(acpihp_br));
> > +}
> > +
> > return 0;
> > }
> > diff --git
> > a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
> > b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
> > new file mode 100644
> > index 00..9f0a88b826
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
> > @@ -0,0 +1 @@
> > +unsupported
Re: [PATCH v6 3/4] qemu: command: add support for acpi-bridge-hotplug feature
On 10/5/21 1:51 AM, Ani Sinha wrote:
This change adds backend qemu command line support for new libvirt global
feature 'acpi-bridge-hotplug'. This option can be used as following:
The '' sub-element under '' is also newly introduced.
'acpi-bridge-hotplug' turns on the following command line option to qemu for
x86 guests:
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
(q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
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 as well as checks for using this option with the right
architecture.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 14
.../aarch64-acpi-hotplug-bridge-disable.err | 1 +
...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 +
.../pc-i440fx-acpi-hotplug-bridge-disable.err | 1 +
.../q35-acpi-hotplug-bridge-disable.args | 33 +++
.../q35-acpi-hotplug-bridge-disable.err | 1 +
tests/qemuxml2argvtest.c | 16 +
7 files changed, 97 insertions(+)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 08f6d735f8..6c8c99e595 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6069,6 +6069,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
{
virQEMUCaps *qemuCaps = priv->qemuCaps;
+int acpihp_br =
def->pci_features[(virDomainPCI)VIR_DOMAIN_PCI_ACPI_BRIDGE_HP];
I'm not sure why you put the typecast on the array subscript, but it's
unnecessary. I removed it.
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
/* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6114,6 +6115,19 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
}
+if (acpihp_br) {
+const char *pm_object = "PIIX4_PM";
+
+if (qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
+pm_object = "ICH9-LPC";
So on Q35 systems that don't support setting
"ICH9-LPC.acpi-pci-hotplug." it will also work to set
"PIIX4_PM.acpi-pci-"? (apparently this is the case with the s3 and
s4 acpi suspend options, which have a similar hack a few lines below
this one).
Assuming that is the case, then this patch also seems fine to me
Reviewed-by: Laine Stump
I may want to make the documentation in formatdomain.rst a bit more
clear, but I can do that as a separate patch after these (since I know
you're leaving on a vacation so won't be around to do it).
As long as nobody complains about the naming of the feature, I think I
can push this tomorrow evening (U.S. time) (that will give time for any
discussion about naming in the morning, then I'll have to be afk for a
few hours in the afternoon.
Thanks for your contribution, and for your perseverance in the face of
my procrastination, and enjoy your vacation!!
+
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd,
"%s.acpi-pci-hotplug-with-bridge-support=%s",
+ pm_object,
+ virTristateSwitchTypeToString(acpihp_br));
+}
+
return 0;
}
diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
new file mode 100644
index 00..9f0a88b826
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available for
architecture 'aarch64'
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
new file mode 100644
index 00..a1e5dc85c7
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-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
[PATCH v6 3/4] qemu: command: add support for acpi-bridge-hotplug feature
This change adds backend qemu command line support for new libvirt global
feature 'acpi-bridge-hotplug'. This option can be used as following:
The '' sub-element under '' is also newly introduced.
'acpi-bridge-hotplug' turns on the following command line option to qemu for
x86 guests:
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
(q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
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 as well as checks for using this option with the right
architecture.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 14
.../aarch64-acpi-hotplug-bridge-disable.err | 1 +
...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 +
.../pc-i440fx-acpi-hotplug-bridge-disable.err | 1 +
.../q35-acpi-hotplug-bridge-disable.args | 33 +++
.../q35-acpi-hotplug-bridge-disable.err | 1 +
tests/qemuxml2argvtest.c | 16 +
7 files changed, 97 insertions(+)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 08f6d735f8..6c8c99e595 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6069,6 +6069,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
{
virQEMUCaps *qemuCaps = priv->qemuCaps;
+int acpihp_br =
def->pci_features[(virDomainPCI)VIR_DOMAIN_PCI_ACPI_BRIDGE_HP];
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
/* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6114,6 +6115,19 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
}
+if (acpihp_br) {
+const char *pm_object = "PIIX4_PM";
+
+if (qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
+pm_object = "ICH9-LPC";
+
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd,
"%s.acpi-pci-hotplug-with-bridge-support=%s",
+ pm_object,
+ virTristateSwitchTypeToString(acpihp_br));
+}
+
return 0;
}
diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
new file mode 100644
index 00..9f0a88b826
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available for
architecture 'aarch64'
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
new file mode 100644
index 00..a1e5dc85c7
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-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-pci-hotplug-with-bridge-support=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-hotplug-bridge-disable.err
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
new file mode 100644
index 00..8c09a3cd76
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available with this QEMU
binary
diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
