[libvirt PATCH v2 0/4] Make SEV 'cbitpos' and 'reducedPhysBits' attributes optional
We designed them as mandatory, but these are platform dependent and can be filled from QEMU capabilities. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/57 Since v1: - moved the capability check from qemuProcessPrepareSEVGuestInput to qemuValidateDomainDef as requested Erik Skultety (4): qemu_process: sev: Drop an unused variable qemu: process: Move SEV capability check to qemuValidateDomainDef qemu: process: sev: Fill missing 'cbitpos' & 'reducedPhysBits' from caps conf: domain: sev: Make 'cbitpos' & 'reducedPhysBits' attrs optional docs/schemas/domaincommon.rng | 16 ++++--- src/conf/domain_conf.c | 46 ++++++++++++------- src/conf/domain_conf.h | 2 + src/qemu/qemu_process.c | 45 +++++++++++++----- src/qemu/qemu_validate.c | 8 ++++ ...v-missing-platform-info.x86_64-2.12.0.args | 37 +++++++++++++++ ...nch-security-sev-missing-platform-info.xml | 35 ++++++++++++++ tests/qemuxml2argvtest.c | 1 + 8 files changed, 157 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-platfo= rm-info.x86_64-2.12.0.args create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-platfo= rm-info.xml --=20 2.26.2
Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5bc76a75e3..f71bb21f09 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6394,9 +6394,8 @@ static int qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainDefPtr def = vm->def; virQEMUCapsPtr qemuCaps = priv->qemuCaps; - virDomainSEVDefPtr sev = def->sev; + virDomainSEVDefPtr sev = vm->def->sev; if (!sev) return 0; -- 2.26.2
Checks such as this one should be done at domain def validation time, not before starting the QEMU process. As for this change, existing domains will see some QEMU error when starting as opposed to a libvirt error that this QEMU binary doesn't support SEV, but that's okay, we never guaranteed error messages to remain the same. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_process.c | 9 --------- src/qemu/qemu_validate.c | 8 ++++++++ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f71bb21f09..16d6f54f66 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6393,8 +6393,6 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, static int qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUCapsPtr qemuCaps = priv->qemuCaps; virDomainSEVDefPtr sev = vm->def->sev; if (!sev) @@ -6402,13 +6400,6 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm) VIR_DEBUG("Preparing SEV guest"); - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain %s asked for 'sev' launch but this " - "QEMU does not support SEV feature"), vm->def->name); - return -1; - } - if (sev->dh_cert) { if (qemuProcessSEVCreateFile(vm, "dh_cert", sev->dh_cert) < 0) return -1; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 28eae76cca..949a5a59b7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1034,6 +1034,14 @@ qemuValidateDomainDef(const virDomainDef *def, return -1; } + if (def->sev && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("SEV launch security is not supported with " + "this QEMU binary")); + return -1; + } + return 0; } -- 2.26.2
On 10/15/20 7:45 AM, Erik Skultety wrote:
Checks such as this one should be done at domain def validation time, not before starting the QEMU process. As for this change, existing domains will see some QEMU error when starting as opposed to a libvirt error that this QEMU binary doesn't support SEV, but that's okay, we never guaranteed error messages to remain the same.
Signed-off-by: Erik Skultety <eskultet@redhat.com> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
src/qemu/qemu_process.c | 9 --------- src/qemu/qemu_validate.c | 8 ++++++++ 2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f71bb21f09..16d6f54f66 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6393,8 +6393,6 @@ qemuProcessSEVCreateFile(virDomainObjPtr vm, static int qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUCapsPtr qemuCaps = priv->qemuCaps; virDomainSEVDefPtr sev = vm->def->sev;
if (!sev) @@ -6402,13 +6400,6 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
VIR_DEBUG("Preparing SEV guest");
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain %s asked for 'sev' launch but this " - "QEMU does not support SEV feature"), vm->def->name); - return -1; - } - if (sev->dh_cert) { if (qemuProcessSEVCreateFile(vm, "dh_cert", sev->dh_cert) < 0) return -1; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 28eae76cca..949a5a59b7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1034,6 +1034,14 @@ qemuValidateDomainDef(const virDomainDef *def, return -1; }
+ if (def->sev && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("SEV launch security is not supported with " + "this QEMU binary")); + return -1; + } + return 0; }
These XML attributes have been mandatory since the introduction of SEV support to libvirt. This design decision was based on QEMU's requirement for these to be mandatory for migration purposes, as differences in these values across platforms must result in the pre-migration checks failing (not that migration with SEV works at the time of this patch). This patch enables autofill of these attributes right before launching QEMU and thus updating the live XML. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 902dd58112..cd344716a3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2491,7 +2491,9 @@ struct _virDomainSEVDef { char *dh_cert; char *session; unsigned int policy; + bool haveCbitpos; unsigned int cbitpos; + bool haveReducedPhysBits; unsigned int reduced_phys_bits; }; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 16d6f54f66..6422881a33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6233,6 +6233,33 @@ qemuProcessPrepareAllowReboot(virDomainObjPtr vm) } +static int +qemuProcessUpdateSEVInfo(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUCapsPtr qemuCaps = priv->qemuCaps; + virDomainSEVDefPtr sev = vm->def->sev; + virSEVCapabilityPtr sevCaps = NULL; + + /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have + * not been supplied, we need to autofill them from caps now as both are + * mandatory on QEMU cmdline + */ + sevCaps = virQEMUCapsGetSEVCapabilities(qemuCaps); + if (!sev->haveCbitpos) { + sev->cbitpos = sevCaps->cbitpos; + sev->haveCbitpos = true; + } + + if (!sev->haveReducedPhysBits) { + sev->reduced_phys_bits = sevCaps->reduced_phys_bits; + sev->haveReducedPhysBits = true; + } + + return 0; +} + + /** * qemuProcessPrepareDomain: * @driver: qemu driver @@ -6361,6 +6388,12 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, for (i = 0; i < vm->def->nshmems; i++) qemuDomainPrepareShmemChardev(vm->def->shmems[i]); + if (vm->def->sev) { + VIR_DEBUG("Updating SEV platform info"); + if (qemuProcessUpdateSEVInfo(vm) < 0) + return -1; + } + return 0; } -- 2.26.2
On 10/15/20 7:45 AM, Erik Skultety wrote:
These XML attributes have been mandatory since the introduction of SEV support to libvirt. This design decision was based on QEMU's requirement for these to be mandatory for migration purposes, as differences in these values across platforms must result in the pre-migration checks failing (not that migration with SEV works at the time of this patch).
This patch enables autofill of these attributes right before launching QEMU and thus updating the live XML.
Signed-off-by: Erik Skultety <eskultet@redhat.com> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
src/conf/domain_conf.h | 2 ++ src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 902dd58112..cd344716a3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2491,7 +2491,9 @@ struct _virDomainSEVDef { char *dh_cert; char *session; unsigned int policy; + bool haveCbitpos; unsigned int cbitpos; + bool haveReducedPhysBits; unsigned int reduced_phys_bits; };
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 16d6f54f66..6422881a33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6233,6 +6233,33 @@ qemuProcessPrepareAllowReboot(virDomainObjPtr vm) }
+static int +qemuProcessUpdateSEVInfo(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUCapsPtr qemuCaps = priv->qemuCaps; + virDomainSEVDefPtr sev = vm->def->sev; + virSEVCapabilityPtr sevCaps = NULL; + + /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have + * not been supplied, we need to autofill them from caps now as both are + * mandatory on QEMU cmdline + */ + sevCaps = virQEMUCapsGetSEVCapabilities(qemuCaps); + if (!sev->haveCbitpos) { + sev->cbitpos = sevCaps->cbitpos; + sev->haveCbitpos = true; + } + + if (!sev->haveReducedPhysBits) { + sev->reduced_phys_bits = sevCaps->reduced_phys_bits; + sev->haveReducedPhysBits = true; + } + + return 0; +} + + /** * qemuProcessPrepareDomain: * @driver: qemu driver @@ -6361,6 +6388,12 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, for (i = 0; i < vm->def->nshmems; i++) qemuDomainPrepareShmemChardev(vm->def->shmems[i]);
+ if (vm->def->sev) { + VIR_DEBUG("Updating SEV platform info"); + if (qemuProcessUpdateSEVInfo(vm) < 0) + return -1; + } + return 0; }
These XML attributes have been mandatory since the introduction of SEV support to libvirt. This design decision was based on QEMU's requirement for these to be mandatory for migration purposes, as differences in these values across platforms must result in the pre-migration checks failing (not that migration with SEV works at the time of this patch). Expecting the user to specify these is cumbersome and the same XML cannot be re-used across different revisions of SEV. Since we have SEV platform information saved in QEMU capabilities, we can make the attributes optional and should fill them in automatically in the QEMU driver right before starting it. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/57 Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- docs/schemas/domaincommon.rng | 16 ++++--- src/conf/domain_conf.c | 46 ++++++++++++------- ...v-missing-platform-info.x86_64-2.12.0.args | 37 +++++++++++++++ ...nch-security-sev-missing-platform-info.xml | 35 ++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 113 insertions(+), 22 deletions(-) create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c26408c400..ae25b9b1bc 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -467,12 +467,16 @@ <value>sev</value> </attribute> <interleave> - <element name="cbitpos"> - <data type="unsignedInt"/> - </element> - <element name="reducedPhysBits"> - <data type="unsignedInt"/> - </element> + <optional> + <element name="cbitpos"> + <data type="unsignedInt"/> + </element> + </optional> + <optional> + <element name="reducedPhysBits"> + <data type="unsignedInt"/> + </element> + </optional> <element name="policy"> <ref name="hexuint"/> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bbe59f61d0..efa5ac527b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16764,6 +16764,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, virDomainSEVDefPtr def; unsigned long policy; g_autofree char *type = NULL; + int rc = -1; def = g_new0(virDomainSEVDef, 1); @@ -16788,25 +16789,35 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, goto error; } - if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("failed to get launch security cbitpos")); - goto error; - } - - if (virXPathUInt("string(./reducedPhysBits)", ctxt, - &def->reduced_phys_bits) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("failed to get launch security reduced-phys-bits")); - goto error; - } - if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to get launch security policy")); goto error; } + /* the following attributes are platform dependent and if missing, we can + * autofill them from domain capabilities later + */ + rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos); + if (rc == 0) { + def->haveCbitpos = true; + } else if (rc == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid format for launch security cbitpos")); + goto error; + } + + rc = virXPathUInt("string(./reducedPhysBits)", ctxt, + &def->reduced_phys_bits); + if (rc == 0) { + def->haveReducedPhysBits = true; + } else if (rc == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid format for launch security " + "reduced-phys-bits")); + goto error; + } + def->policy = policy; def->dh_cert = virXPathString("string(./dhCert)", ctxt); def->session = virXPathString("string(./session)", ctxt); @@ -28958,9 +28969,12 @@ virDomainSEVDefFormat(virBufferPtr buf, virDomainSEVDefPtr sev) virDomainLaunchSecurityTypeToString(sev->sectype)); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); - virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n", - sev->reduced_phys_bits); + if (sev->haveCbitpos) + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + + if (sev->haveReducedPhysBits) + virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n", + sev->reduced_phys_bits); virBufferAsprintf(buf, "<policy>0x%04x</policy>\n", sev->policy); if (sev->dh_cert) virBufferEscapeString(buf, "<dhCert>%s</dhCert>\n", sev->dh_cert); diff --git a/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args new file mode 100644 index 0000000000..378c3b681c --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args @@ -0,0 +1,37 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc-1.0,accel=kvm,usb=off,dump-guest-core=off,memory-encryption=sev0 \ +-m 214 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-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 \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ +-object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\ +dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\ +session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml new file mode 100644 index 0000000000..41ec4cb872 --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.xml @@ -0,0 +1,35 @@ +<domain type='kvm'> + <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='pc-1.0'>hvm</type> + <boot dev='hd'/> + </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> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> + <launchSecurity type='sev'> + <policy>0x0001</policy> + <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert> + <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session> + </launchSecurity> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 8aa791d9f7..f9b02b232f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3316,6 +3316,7 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-auto", "s390x"); DO_TEST_CAPS_VER("launch-security-sev", "2.12.0"); + DO_TEST_CAPS_VER("launch-security-sev-missing-platform-info", "2.12.0"); DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory"); DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages"); -- 2.26.2
participants (2)
-
Daniel Henrique Barboza -
Erik Skultety