[PATCH 0/2] qemu: swtpm improvements
These patches are fallout from a discussion about TPM devices and aarch64 https://listman.redhat.com/archives/libvir-list/2021-February/msg00526.html Jim Fehlig (2): qemu: Fix swtpm device with aarch64 qemu: Validate TPM TIS device src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_validate.c | 7 ++++ .../aarch64-tpm.aarch64-latest.args | 37 +++++++++++++++++++ tests/qemuxml2argvdata/aarch64-tpm.xml | 15 ++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 63 insertions(+) create mode 100644 tests/qemuxml2argvdata/aarch64-tpm.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/aarch64-tpm.xml -- 2.29.2
Starting a VM with swtpm device fails with qemu-system-aarch64. E.g. with TPM device config <tpm model='tpm-tis'> <backend type='emulator' version='2.0'/> </tpm> QEMU reports the following error error: internal error: process exited while connecting to monitor: 2021-02-07T05:15:35.378927Z qemu-system-aarch64: -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0: 'tpm-tis' is not a valid device model name Indeed the TPM device name is 'tpm-tis-device' [1][2] for aarch64, versus the shorter 'tpm-tis' for x86. The devices are the same from a functional POV, i.e. they both emulate a TPM device conforming to the TIS specification. Account for the unfortunate name difference when building the TPM device option in qemuBuildTPMDevStr(). Also include a test case for 'tpm-tis-device'. [1] https://qemu.readthedocs.io/en/latest/specs/tpm.html [2] https://github.com/qemu/qemu/commit/c294ac327ca99342b90bd3a83d2cef9b447afaa7 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_command.c | 3 ++ .../aarch64-tpm.aarch64-latest.args | 37 +++++++++++++++++++ tests/qemuxml2argvdata/aarch64-tpm.xml | 15 ++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 56 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f0333d4f1a..96b956d7ae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9203,6 +9203,9 @@ qemuBuildTPMDevStr(const virDomainDef *def, g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *model = virDomainTPMModelTypeToString(tpm->model); + if (tpm->model == VIR_DOMAIN_TPM_MODEL_TIS && def->os.arch == VIR_ARCH_AARCH64) + model = "tpm-tis-device"; + virBufferAsprintf(&buf, "%s,tpmdev=tpm-%s,id=%s", model, tpm->info.alias, tpm->info.alias); diff --git a/tests/qemuxml2argvdata/aarch64-tpm.aarch64-latest.args b/tests/qemuxml2argvdata/aarch64-tpm.aarch64-latest.args new file mode 100644 index 0000000000..94a083d816 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-tpm.aarch64-latest.args @@ -0,0 +1,37 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-aarch64test \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-aarch64test/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-aarch64test/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-aarch64test/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 \ +-name guest=aarch64test,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-aarch64test/master-key.aes \ +-machine virt,accel=tcg,usb=off,dump-guest-core=off,gic-version=2,\ +memory-backend=mach-virt.ram \ +-cpu cortex-a15 \ +-m 1024 \ +-object memory-backend-ram,id=mach-virt.ram,size=1073741824 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \ +-chardev socket,id=chrtpm,path=/dev/test \ +-device tpm-tis-device,tpmdev=tpm-tpm0,id=tpm0 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/aarch64-tpm.xml b/tests/qemuxml2argvdata/aarch64-tpm.xml new file mode 100644 index 0000000000..d338a20f17 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-tpm.xml @@ -0,0 +1,15 @@ +<domain type="qemu"> + <name>aarch64test</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch="aarch64" machine="virt">hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <tpm model='tpm-tis'> + <backend type='emulator' version='2.0'/> + </tpm> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index faa71a7a16..d09db77d8c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2478,6 +2478,7 @@ mymain(void) DO_TEST_CAPS_LATEST("tpm-emulator-tpm2-enc"); DO_TEST_CAPS_LATEST("tpm-emulator-tpm2-pstate"); DO_TEST_CAPS_LATEST_PPC64("tpm-emulator-spapr"); + DO_TEST_CAPS_ARCH_LATEST("aarch64-tpm", "aarch64"); DO_TEST_PARSE_ERROR("pci-domain-invalid", NONE); DO_TEST_PARSE_ERROR("pci-bus-invalid", NONE); -- 2.29.2
TPM devices with model='tpm-tis' are only valid with x86 and aarch64 virt machines. Add a check to qemuValidateDomainDeviceDefTPM() to ensure VIR_DOMAIN_TPM_MODEL_TIS is only used with these architectures. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- The conditional is a bit distasteful, but so far I haven't come up with anything better. I aslo worry about future architectures gaining support for emulated TPM TIS devices. src/qemu/qemu_validate.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index a70737327e..d6ff5e5eef 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4299,6 +4299,13 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, switch (tpm->model) { case VIR_DOMAIN_TPM_MODEL_TIS: + if (!ARCH_IS_X86(def->os.arch) && (def->os.arch != VIR_ARCH_AARCH64)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("TPM model %s is only available for " + "x86 and aarch64 guests"), + virDomainTPMModelTypeToString(tpm->model)); + return -1; + } flag = QEMU_CAPS_DEVICE_TPM_TIS; break; case VIR_DOMAIN_TPM_MODEL_CRB: -- 2.29.2
On Wed, 2021-02-10 at 10:34 -0700, Jim Fehlig wrote:
I aslo worry about future architectures gaining support for emulated TPM TIS devices.
It's okay to be conservative - we can always relax the check later.
+ if (!ARCH_IS_X86(def->os.arch) && (def->os.arch != VIR_ARCH_AARCH64)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("TPM model %s is only available for " + "x86 and aarch64 guests"),
Please don't split the error message into two separate lines, and sprinkle some quotes around '%s' while you're at it. https://libvirt.org/coding-style.html#error-message-format -- Andrea Bolognani / Red Hat / Virtualization
On 2/11/21 3:37 AM, Andrea Bolognani wrote:
On Wed, 2021-02-10 at 10:34 -0700, Jim Fehlig wrote:
I aslo worry about future architectures gaining support for emulated TPM TIS devices.
It's okay to be conservative - we can always relax the check later.
+ if (!ARCH_IS_X86(def->os.arch) && (def->os.arch != VIR_ARCH_AARCH64)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("TPM model %s is only available for " + "x86 and aarch64 guests"),
Please don't split the error message into two separate lines
I'm surprised that slipped through my copy and paste since I've always preferred errors on one line for easy searching. We have a lot of split error messages throughout the code. This file is particularly bad. Is there any desire to fix existing cases, or just avoid new ones?
and sprinkle some quotes around '%s' while you're at it.
Will do.
The entire page is worth a re-read on occasion :-) Regards, Jim
On Thu, 2021-02-11 at 08:44 -0700, Jim Fehlig wrote:
On 2/11/21 3:37 AM, Andrea Bolognani wrote:
On Wed, 2021-02-10 at 10:34 -0700, Jim Fehlig wrote:
+ if (!ARCH_IS_X86(def->os.arch) && (def->os.arch != VIR_ARCH_AARCH64)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("TPM model %s is only available for " + "x86 and aarch64 guests"),
Please don't split the error message into two separate lines
I'm surprised that slipped through my copy and paste since I've always preferred errors on one line for easy searching. We have a lot of split error messages throughout the code. This file is particularly bad. Is there any desire to fix existing cases, or just avoid new ones?
I don't think we're particularly interested in going back and altering existing messages, and most importantly in the churn doing so would generate :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2021-02-10 at 10:34 -0700, Jim Fehlig wrote:
These patches are fallout from a discussion about TPM devices and aarch64
https://listman.redhat.com/archives/libvir-list/2021-February/msg00526.html
Jim Fehlig (2): qemu: Fix swtpm device with aarch64 qemu: Validate TPM TIS device
With the nit I pointed out in 2/2 addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Jim Fehlig