Hi Michal,
thanks a lot for posting this. One question:
On 07/27/16 10:43, Michal Privoznik wrote:
> Since its release of 2.4.0 qemu is able to enable System
> Management Module in the firmware, or disable it. We should
> expose this capability in the XML. Unfortunately, there's no good
> way to determine whether the binary we are talking to supports
> it. I mean, if qemu's run with real machine type, the smm
> attribute can be seen in 'qom-list /machine' output. But it's not
> there when qemu's run with -M none. Therefore we're stuck with
> version based check.
Where does your information come from that says QEMU 2.4 is good enough
for this? To my knowledge, the pc-q35-2.5 machine type is minimally
required, i.e., no i440fx at all, and for q35, 2.5 or later, which can
only be provided by QEMU v2.5.0 or later.
Hmmm... I found QEMU commit 355023f2010c4 ("pc: add SMM property"), and
it's indeed part of v2.4.0. I wonder where *I* got the information from
that only pc-q35-2.5+ machtypes are suitable for SMM (as OVMF needs it).
Perhaps there were other requirements too (SMRAM emulation etc). Ah,
wait, *large* SMRAM (T-SEG) is Q35-only. The SMM property is available
on i440fx as well.
Hm, even Paolo's presentation ("Securing secure boot with
System Management Mode") mentions "QEMU: released in 2.4".
... Ah, I think I might now why. In this RHBZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1202822
Gerd set
> Gerd Hoffmann 2016-03-21 08:23:06 CET
> Fixed In Version: qemu-2.5
I checked
git log --no-merges --oneline --reverse v2.4.0..v2.5.0
but nothing says "smm", "smram", or "tseg" (case-insensitively).
I recall commit bafc90bdc594 ("q35: implement TSEG") as one of Gerd's
QEMU patches, but that's also part of v2.4.0. Confirmed by:
<http://wiki.qemu.org/ChangeLog/2.4#x86>.
... Gerd's above metadata change is dated 2016-03-21, at which point
both QEMU v2.4.0 and v2.5.0 had been tagged (on 2015-08-11 and on
2015-12-16, respectively). I guess Gerd may have mis-remembered the
exact QEMU release which included SMM support?
I don't remember ever testing SMM (using OVMF) with released 2.4 machine
types, so I feel a bit uncertain about mentioning and looking for 2.4 in
this patch.
... Anyway, looking over your patch quickly, I think it really only
concerns itself with "-machine smm=on", and for that, the 2.4 version
check should suffice. It's only OVMF that needs a lot more than that.
Acked-by: Laszlo Ersek <lersek@xxxxxxxxxx>
Thanks!
Laszlo
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 6 +++++
> docs/schemas/domaincommon.rng | 9 +++++++
> src/conf/domain_conf.c | 5 +++-
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_capabilities.c | 16 +++++++++++++
> src/qemu/qemu_capabilities.h | 4 ++++
> src/qemu/qemu_command.c | 12 ++++++++++
> tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 +
> .../caps_2.6.0-gicv2.aarch64.xml | 1 +
> .../caps_2.6.0-gicv3.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 +
> tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 +
> .../qemuxml2argv-machine-smm-opt.args | 25 +++++++++++++++++++
> .../qemuxml2argv-machine-smm-opt.xml | 28 ++++++++++++++++++++++
> tests/qemuxml2argvtest.c | 7 ++++++
> 16 files changed, 118 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 59a8bb9..3f67182 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1655,6 +1655,12 @@
> values are <code>2</code>, <code>3</code> and <code>host</code>.
> <span class="since">Since 1.2.16</span>
> </dd>
> + <dt><code>smm</code></dt>
> + <dd>Enable System Management Mode. Possible values are
> + <code>on</code> and <code>off</code>. The default is left
> + for hypervisor to decide.
> + <span class="since">Since 2.1.0</span>
> + </dd>
> </dl>
>
> <h3><a name="elementsTime">Time keeping</a></h3>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 741f268..39fcb7e 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4291,6 +4291,15 @@
> </optional>
> </element>
> </optional>
> + <optional>
> + <element name="smm">
> + <optional>
> + <attribute name="state">
> + <ref name="virOnOff"/>
> + </attribute>
> + </optional>
> + </element>
> + </optional>
> </interleave>
> </element>
> </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 13e5dc9..3b6493e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -137,7 +137,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
> "capabilities",
> "pmu",
> "vmport",
> - "gic")
> + "gic",
> + "smm")
>
> VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
> "default",
> @@ -16318,6 +16319,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> case VIR_DOMAIN_FEATURE_PMU:
> case VIR_DOMAIN_FEATURE_PVSPINLOCK:
> case VIR_DOMAIN_FEATURE_VMPORT:
> + case VIR_DOMAIN_FEATURE_SMM:
> node = ctxt->node;
> ctxt->node = nodes[i];
> if ((tmp = virXPathString("string(./@state)", ctxt))) {
> @@ -23291,6 +23293,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> case VIR_DOMAIN_FEATURE_PMU:
> case VIR_DOMAIN_FEATURE_PVSPINLOCK:
> case VIR_DOMAIN_FEATURE_VMPORT:
> + case VIR_DOMAIN_FEATURE_SMM:
> switch ((virTristateSwitch) def->features[i]) {
> case VIR_TRISTATE_SWITCH_LAST:
> case VIR_TRISTATE_SWITCH_ABSENT:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3c2f182..60b4be5 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1600,6 +1600,7 @@ typedef enum {
> VIR_DOMAIN_FEATURE_PMU,
> VIR_DOMAIN_FEATURE_VMPORT,
> VIR_DOMAIN_FEATURE_GIC,
> + VIR_DOMAIN_FEATURE_SMM,
>
> VIR_DOMAIN_FEATURE_LAST
> } virDomainFeature;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index d5b73e6..0b36819 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -339,6 +339,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> "tls-creds-x509", /* 230 */
> "display",
> "intel-iommu",
> + "smm",
> );
>
>
> @@ -3533,6 +3534,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> if (qemuCaps->version >= 2004000)
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE);
>
> + /* smm option is supported from v2.4.0 */
> + if (qemuCaps->version >= 2004000)
> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT);
> +
> /* Since 2.4.50 ARM virt machine supports gic-version option */
> if (qemuCaps->version >= 2004050)
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACH_VIRT_GIC_VERSION);
> @@ -4052,6 +4057,17 @@ virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps,
>
>
> bool
> +virQEMUCapsSupportsSMM(virQEMUCapsPtr qemuCaps,
> + const virDomainDef *def)
> +{
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT))
> + return false;
> +
> + return qemuDomainMachineIsQ35(def);
> +}
> +
> +
> +bool
> virQEMUCapsIsMachineSupported(virQEMUCapsPtr qemuCaps,
> const char *canonical_machine)
> {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index bd5c6d9..8c4bc95 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -372,6 +372,7 @@ typedef enum {
> QEMU_CAPS_OBJECT_TLS_CREDS_X509, /* -object tls-creds-x509 */
> QEMU_CAPS_DISPLAY, /* -display */
> QEMU_CAPS_DEVICE_INTEL_IOMMU, /* -device intel-iommu */
> + QEMU_CAPS_MACHINE_SMM_OPT, /* -machine xxx,smm=on/off/auto */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
> @@ -408,6 +409,9 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
> bool virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps,
> const virDomainDef *def);
>
> +bool virQEMUCapsSupportsSMM(virQEMUCapsPtr qemuCaps,
> + const virDomainDef *def);
> +
> char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps);
>
> const char *virQEMUCapsGetBinary(virQEMUCapsPtr qemuCaps);
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6295eeb..831aba1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6791,6 +6791,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> }
> } else {
> virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
> + virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
>
> virCommandAddArg(cmd, "-machine");
> virBufferAdd(&buf, def->os.machine, -1);
> @@ -6820,6 +6821,17 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> virTristateSwitchTypeToString(vmport));
> }
>
> + if (smm) {
> + if (!virQEMUCapsSupportsSMM(qemuCaps, def)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("smm is not available with this QEMU binary"));
> + goto cleanup;
> + }
> +
> + virBufferAsprintf(&buf, ",smm=%s",
> + virTristateSwitchTypeToString(smm));
> + }
> +
> if (def->mem.dump_core) {
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
> index 80085d5..339ee1f 100644
> --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
> @@ -183,6 +183,7 @@
> <flag name='drive-detect-zeroes'/>
> <flag name='display'/>
> <flag name='intel-iommu'/>
> + <flag name='smm'/>
> <version>2004000</version>
> <kvmVersion>0</kvmVersion>
> <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
> index fad3291..c1a68d0 100644
> --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
> @@ -188,6 +188,7 @@
> <flag name='tls-creds-x509'/>
> <flag name='display'/>
> <flag name='intel-iommu'/>
> + <flag name='smm'/>
> <version>2005000</version>
> <kvmVersion>0</kvmVersion>
> <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
> index 4ed88bc..85d7d3f 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
> @@ -157,6 +157,7 @@
> <flag name='drive-detect-zeroes'/>
> <flag name='tls-creds-x509'/>
> <flag name='display'/>
> + <flag name='smm'/>
> <version>2005094</version>
> <kvmVersion>0</kvmVersion>
> <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
> index 024596d..deb1257 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
> @@ -157,6 +157,7 @@
> <flag name='drive-detect-zeroes'/>
> <flag name='tls-creds-x509'/>
> <flag name='display'/>
> + <flag name='smm'/>
> <version>2005094</version>
> <kvmVersion>0</kvmVersion>
> <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
> index e66433c..2b7ea0e 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
> @@ -151,6 +151,7 @@
> <flag name='drive-detect-zeroes'/>
> <flag name='tls-creds-x509'/>
> <flag name='display'/>
> + <flag name='smm'/>
> <version>2005094</version>
> <kvmVersion>0</kvmVersion>
> <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
> index 653ec75..495c114 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
> @@ -194,6 +194,7 @@
> <flag name='tls-creds-x509'/>
> <flag name='display'/>
> <flag name='intel-iommu'/>
> + <flag name='smm'/>
> <version>2006000</version>
> <kvmVersion>0</kvmVersion>
> <package></package>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.args b/tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.args
> new file mode 100644
> index 0000000..e49d7e9
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.args
> @@ -0,0 +1,25 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest1 \
> +-S \
> +-machine q35,accel=tcg,smm=on \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device virtio-scsi-pci,id=scsi0,bus=pci.2,addr=0x1 \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-scsi0-0-0-0 \
> +-device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
> +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x2
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.xml b/tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.xml
> new file mode 100644
> index 0000000..b964b5e
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-smm-opt.xml
> @@ -0,0 +1,28 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='q35'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu</emulator>
> + <disk type='block' device='disk'>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='sda' bus='scsi'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </disk>
> + <controller type='scsi' index='0'/>
> + <memballoon model='virtio'/>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index a5d51a8..f9588d5 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -618,6 +618,13 @@ mymain(void)
> QEMU_CAPS_DUMP_GUEST_CORE);
> DO_TEST_FAILURE("machine-core-on", NONE);
> DO_TEST_FAILURE("machine-core-on", QEMU_CAPS_MACHINE_OPT);
> + DO_TEST("machine-smm-opt",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_OPT,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI);
> DO_TEST("machine-usb-opt", QEMU_CAPS_MACHINE_OPT,
> QEMU_CAPS_MACHINE_USB_OPT);
> DO_TEST("machine-vmport-opt", QEMU_CAPS_MACHINE_OPT,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list