On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

To allow other types of launch security the SEV type specific parameters like e.g. policy need to be optional and be separated from other new launch security types. A test is added to ensure the previously required and now optional launch security policy remains required when launch security type is SEV.

I think you missed a breakline up there. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/schemas/domaincommon.rng | 12 +- src/conf/domain_conf.c | 156 +++++++++++------- src/conf/domain_conf.h | 13 +- src/conf/virconftypes.h | 2 + src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 38 ++++- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_firmware.c | 4 +- src/qemu/qemu_namespace.c | 20 ++- src/qemu/qemu_process.c | 35 +++- src/qemu/qemu_validate.c | 22 ++- src/security/security_dac.c | 4 +- ...urity-sev-missing-policy.x86_64-2.12.0.err | 1 + .../launch-security-sev-missing-policy.xml | 34 ++++ tests/qemuxml2argvtest.c | 1 + 15 files changed, 253 insertions(+), 95 deletions(-) create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a2e5c50c1d..3df13a0cf1 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -483,7 +483,9 @@ <define name="launchSecurity"> <element name="launchSecurity"> <attribute name="type"> - <value>sev</value> + <choice> + <value>sev</value> + </choice> </attribute> <interleave> <optional> @@ -496,9 +498,11 @@ <data type="unsignedInt"/> </element> </optional> - <element name="policy"> - <ref name="hexuint"/> - </element> + <optional> + <element name="policy"> + <ref name="hexuint"/> + </element> + </optional> <optional> <element name="handle"> <ref name="unsignedInt"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3ed3a9c5a..228de5d715 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3481,8 +3481,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl) }

-static void -virDomainSEVDefFree(virDomainSEVDef *def) +void virDomainSEVDefFree(virDomainSEVDef *def) { if (!def) return; @@ -3493,6 +3492,17 @@ virDomainSEVDefFree(virDomainSEVDef *def) g_free(def); }

+void virDomainSecDefFree(virDomainSecDef *def) +{ + if (!def) + return; + + virDomainSEVDefFree(def->sev); + + g_free(def); +} + + static void virDomainOSDefClear(virDomainOSDef *os) { @@ -3694,7 +3704,7 @@ void virDomainDefFree(virDomainDef *def) if (def->namespaceData && def->ns.free) (def->ns.free)(def->namespaceData);

- virDomainSEVDefFree(def->sev); + virDomainSecDefFree(def->sec);

xmlFreeNode(def->metadata);

@@ -14688,72 +14698,80 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - virDomainSEVDef *def; + g_autoptr(virDomainSEVDef) sev = g_new0(virDomainSEVDef, 1); unsigned long policy; - g_autofree char *type = NULL; int rc = -1;

- def = g_new0(virDomainSEVDef, 1); - ctxt->node = sevNode;

- if (!(type = virXMLPropString(sevNode, "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing launch security type")); - goto error; - } - - def->sectype = virDomainLaunchSecurityTypeFromString(type); - switch ((virDomainLaunchSecurity) def->sectype) { - case VIR_DOMAIN_LAUNCH_SECURITY_SEV: - break; - case VIR_DOMAIN_LAUNCH_SECURITY_NONE: - case VIR_DOMAIN_LAUNCH_SECURITY_LAST: - default: - virReportError(VIR_ERR_XML_ERROR, - _("unsupported launch security type '%s'"), - type); - goto error; - } - if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("failed to get launch security policy")); - goto error; + _("Failed to get launch security policy for " + "launch security type SEV")); + return NULL; }

/* the following attributes are platform dependent and if missing, we can * autofill them from domain capabilities later */ - rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos); + rc = virXPathUInt("string(./cbitpos)", ctxt, &sev->cbitpos); if (rc == 0) { - def->haveCbitpos = true; + sev->haveCbitpos = true; } else if (rc == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid format for launch security cbitpos")); - goto error; + return NULL; }

rc = virXPathUInt("string(./reducedPhysBits)", ctxt, - &def->reduced_phys_bits); + &sev->reduced_phys_bits); if (rc == 0) { - def->haveReducedPhysBits = true; + sev->haveReducedPhysBits = true; } else if (rc == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid format for launch security " "reduced-phys-bits")); - goto error; + return NULL; }

- def->policy = policy; - def->dh_cert = virXPathString("string(./dhCert)", ctxt); - def->session = virXPathString("string(./session)", ctxt); + sev->policy = policy; + sev->dh_cert = virXPathString("string(./dhCert)", ctxt); + sev->session = virXPathString("string(./session)", ctxt);

- return def; + return g_steal_pointer(&sev); +}

- error: - virDomainSEVDefFree(def); - return NULL; + +static virDomainSecDef * +virDomainSecDefParseXML(xmlNodePtr lsecNode, + xmlXPathContextPtr ctxt) +{ + g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1); + g_autofree char *type = NULL; + + if (!(type = virXMLPropString(lsecNode, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing launch security type")); + return NULL; + } + + sec->sectype = virDomainLaunchSecurityTypeFromString(type); + switch ((virDomainLaunchSecurity) sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + sec->sev = virDomainSEVDefParseXML(lsecNode, ctxt); + if (!sec->sev) + return NULL; + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + default: + virReportError(VIR_ERR_XML_ERROR, + _("unsupported launch security type '%s'"), + type); + return NULL; + } + + return g_steal_pointer(&sec); }

@@ -20117,10 +20135,10 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; VIR_FREE(nodes);

- /* Check for SEV feature */ + /* Check for launch security e.g. SEV feature */ if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) { - def->sev = virDomainSEVDefParseXML(node, ctxt); - if (!def->sev) + def->sec = virDomainSecDefParseXML(node, ctxt); + if (!def->sec) goto error; }

@@ -26845,30 +26863,44 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)

static void -virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) +virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec) { - if (!sev) + if (!sec) return;

- virBufferAsprintf(buf, "<launchSecurity type='%s'>\n", - virDomainLaunchSecurityTypeToString(sev->sectype)); - virBufferAdjustIndent(buf, 2); + switch ((virDomainLaunchSecurity) sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: { + virDomainSEVDef *sev = sec->sev; + if (!sev) + return;

- if (sev->haveCbitpos) - virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<launchSecurity type='%s'>\n", + virDomainLaunchSecurityTypeToString(sec->sectype)); + virBufferAdjustIndent(buf, 2);

- 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); + if (sev->haveCbitpos) + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);

- if (sev->session) - virBufferEscapeString(buf, "<session>%s</session>\n", sev->session); + 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); + + if (sev->session) + virBufferEscapeString(buf, "<session>%s</session>\n", sev->session); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</launchSecurity>\n"); + break; + } + + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + break; + }

- virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</launchSecurity>\n"); }

@@ -28306,7 +28338,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, if (def->keywrap) virDomainKeyWrapDefFormat(buf, def->keywrap);

- virDomainSEVDefFormat(buf, def->sev); + virDomainSecDefFormat(buf, def->sec);

if (def->namespaceData && def->ns.format) { if ((def->ns.format)(buf, def->namespaceData) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cf8481f1f6..dd78f30ace 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2637,7 +2637,6 @@ typedef enum {

struct _virDomainSEVDef { - int sectype; /* enum virDomainLaunchSecurity */ char *dh_cert; char *session; unsigned int policy; @@ -2647,6 +2646,10 @@ struct _virDomainSEVDef { unsigned int reduced_phys_bits; };

+struct _virDomainSecDef { + int sectype; /* enum virDomainLaunchSecurity */ + virDomainSEVDef *sev; +};

typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL, @@ -2857,8 +2860,8 @@ struct _virDomainDef {

virDomainKeyWrapDef *keywrap;

- /* SEV-specific domain */ - virDomainSEVDef *sev; + /* launch security e.g. SEV */ + virDomainSecDef *sec;

/* Application-specific custom metadata */ xmlNodePtr metadata; @@ -3271,6 +3274,10 @@ void virDomainRedirdevDefFree(virDomainRedirdevDef *def); void virDomainRedirFilterDefFree(virDomainRedirFilterDef *def); void virDomainShmemDefFree(virDomainShmemDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree); +void virDomainSEVDefFree(virDomainSEVDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree); +void virDomainSecDefFree(virDomainSecDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, virDomainSecDefFree); void virDomainDeviceDefFree(virDomainDeviceDef *def);

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree); diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index ff5d8145c3..1459bea191 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -204,6 +204,8 @@ typedef struct _virDomainResourceDef virDomainResourceDef;

typedef struct _virDomainSEVDef virDomainSEVDef;

+typedef struct _virDomainSecDef virDomainSecDef; + typedef struct _virDomainShmemDef virDomainShmemDef;

typedef struct _virDomainSmartcardDef virDomainSmartcardDef; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 0e8835fb86..72bff2cbed 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -854,7 +854,9 @@ qemuSetupDevicesCgroup(virDomainObj *vm) return -1; }

- if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0) + if (vm->def->sec && + vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && + qemuSetupSEVCgroup(vm) < 0) return -1;

return 0; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6c5308ef0..10dcf11d5b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6987,8 +6987,18 @@ qemuBuildMachineCommandLine(virCommand *cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM)) qemuAppendLoadparmMachineParm(&buf, def);

- if (def->sev) - virBufferAddLit(&buf, ",memory-encryption=sev0"); + if (def->sec) { + switch ((virDomainLaunchSecurity) def->sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + virBufferAddLit(&buf, ",memory-encryption=sev0"); + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); + return -1; + } + }

if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) { if (priv->pflash0) @@ -9868,6 +9878,28 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd, return 0; }

+ +static int +qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd, + virDomainSecDef *sec) +{ + if (!sec) + return 0; + + switch ((virDomainLaunchSecurity) sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + return qemuBuildSEVCommandLine(vm, cmd, sec->sev); + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); + return -1; + } + + return 0; +} + static int qemuBuildVMCoreInfoCommandLine(virCommand *cmd, const virDomainDef *def) @@ -10567,7 +10599,7 @@ qemuBuildCommandLine(virQEMUDriver *driver, if (qemuBuildVMCoreInfoCommandLine(cmd, def) < 0) return NULL;

- if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0) + if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0) return NULL;

if (snapshot) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c90d52edc0..c3deec439c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19853,7 +19853,7 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0) goto cleanup;

- if (vm->def->sev) { + if (vm->def->sec && vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { if (qemuDomainGetSEVMeasurement(driver, vm, params, nparams, flags) < 0) goto cleanup; } diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 2aeac635da..277ecc4b5b 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1042,8 +1042,8 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; }

- if (def->sev && - def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && + if (def->sec && + def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && !supportsSEV) { VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", path); return false; diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 6a97863d92..0dd1291c5d 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -594,16 +594,26 @@ static int qemuDomainSetupLaunchSecurity(virDomainObj *vm, GSList **paths) { - virDomainSEVDef *sev = vm->def->sev; + virDomainSecDef *sec = vm->def->sec;

- if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) + if (!sec) return 0;

- VIR_DEBUG("Setting up launch security"); + switch ((virDomainLaunchSecurity) sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + VIR_DEBUG("Setting up launch security for SEV");

- *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV)); + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV)); + + VIR_DEBUG("Set up launch security for SEV"); + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); + return -1; + }

- VIR_DEBUG("Set up launch security"); return 0; }

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b69a9d1927..a7d88015ba 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6410,7 +6410,7 @@ qemuProcessUpdateSEVInfo(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; virQEMUCaps *qemuCaps = priv->qemuCaps; - virDomainSEVDef *sev = vm->def->sev; + virDomainSEVDef *sev = vm->def->sec->sev; virSEVCapability *sevCaps = NULL;

/* if platform specific info like 'cbitpos' and 'reducedPhysBits' have @@ -6566,7 +6566,7 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, for (i = 0; i < vm->def->nshmems; i++) qemuDomainPrepareShmemChardev(vm->def->shmems[i]);

- if (vm->def->sev) { + if (vm->def->sec && vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { VIR_DEBUG("Updating SEV platform info"); if (qemuProcessUpdateSEVInfo(vm) < 0) return -1; @@ -6602,12 +6602,13 @@ qemuProcessSEVCreateFile(virDomainObj *vm,

static int -qemuProcessPrepareSEVGuestInput(virDomainObj *vm) +qemuProcessPrepareSEVGuestInput(virDomainObj *vm, + virDomainSecDef *sec) { - virDomainSEVDef *sev = vm->def->sev; + virDomainSEVDef *sev = sec->sev;

if (!sev) - return 0; + return -1;

VIR_DEBUG("Preparing SEV guest");

@@ -6625,6 +6626,28 @@ qemuProcessPrepareSEVGuestInput(virDomainObj *vm) }

+static int +qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm) +{ + virDomainSecDef *sec = vm->def->sec; + + if (!sec) + return 0; + + switch ((virDomainLaunchSecurity) sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + return qemuProcessPrepareSEVGuestInput(vm, sec); + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); + return -1; + } + + return 0; +} + + static int qemuProcessPrepareHostStorage(virQEMUDriver *driver, virDomainObj *vm, @@ -6804,7 +6827,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver, if (qemuExtDevicesPrepareHost(driver, vm) < 0) return -1;

- if (qemuProcessPrepareSEVGuestInput(vm) < 0) + if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0) return -1;

return 0; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 141203f979..78582a7c2a 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1214,12 +1214,22 @@ qemuValidateDomainDef(const virDomainDef *def, if (qemuValidateDomainDefPanic(def, qemuCaps) < 0) 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; + if (def->sec) { + switch ((virDomainLaunchSecurity) def->sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("SEV launch security is not supported with " + "this QEMU binary")); + return -1; + } + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); + return -1; + } }

if (def->naudios > 1 && diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e973964735..289b614b73 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1980,7 +1980,7 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; }

- if (def->sev) { + if (def->sec && def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { if (virSecurityDACRestoreSEVLabel(mgr, def) < 0) rc = -1; } @@ -2187,7 +2187,7 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, return -1; }

- if (def->sev) { + if (def->sec && def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { if (virSecurityDACSetSEVLabel(mgr, def) < 0) return -1; } diff --git a/tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err b/tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err new file mode 100644 index 0000000000..63eaf64071 --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err @@ -0,0 +1 @@ +XML error: Failed to get launch security policy for launch security type SEV diff --git a/tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml b/tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml new file mode 100644 index 0000000000..5461b06c9d --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml @@ -0,0 +1,34 @@ +<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'> + <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert> + <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session> + </launchSecurity> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 25b0c81f21..594a01de45 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3496,6 +3496,7 @@ mymain(void)

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_VER_PARSE_ERROR("launch-security-sev-missing-policy", "2.12.0");

DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory"); DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages");

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

Add s390-pv-guest capability.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + 3 files changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d3f24feb6a..3d0b552f62 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -629,6 +629,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 400 */ "compat-deprecated", "acpi-index", + "s390-pv-guest", );

@@ -1347,6 +1348,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "am53c974", QEMU_CAPS_SCSI_AM53C974 }, { "virtio-pmem-pci", QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI }, { "vhost-user-blk", QEMU_CAPS_DEVICE_VHOST_USER_BLK }, + { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST }, };

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index fae8492e69..b76798501b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -609,6 +609,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 400 */ QEMU_CAPS_COMPAT_DEPRECATED, /* -compat deprecated-(input|output) is supported */ QEMU_CAPS_ACPI_INDEX, /* PCI device 'acpi-index' property */ + QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml index 4c056c484f..2f840073bc 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml @@ -164,6 +164,7 @@ <flag name='rotation-rate'/> <flag name='compat-deprecated'/> <flag name='acpi-index'/> + <flag name='s390-pv-guest'/> <version>6000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100242</microcodeVersion>

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 8 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 26 ++++++++++++++ src/qemu/qemu_namespace.c | 1 + src/qemu/qemu_process.c | 1 + src/qemu/qemu_validate.c | 8 +++++ .../launch-security-s390-pv-ignore-policy.xml | 24 +++++++++++++ .../launch-security-s390-pv.xml | 18 ++++++++++ .../launch-security-s390-pv-ignore-policy.xml | 1 + tests/genericxml2xmltest.c | 2 ++ ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++++++++++++++++++ .../launch-security-s390-pv-ignore-policy.xml | 33 +++++++++++++++++ .../launch-security-s390-pv.s390x-latest.args | 35 +++++++++++++++++++ .../launch-security-s390-pv.xml | 30 ++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 16 files changed, 227 insertions(+) create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml create mode 120000 tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3df13a0cf1..7c92e4c812 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -485,6 +485,7 @@ <attribute name="type"> <choice> <value>sev</value> + <value>s390-pv</value> </choice> </attribute> <interleave>

You added a new 's390-pv' security type, but down there you're using the new confidential-guest-support feature from QEMU 6.0 which is also valid for AMD and pSeries. I think you can do a little change in the idea of these patches while keeping most of it. Instead of calling this new support 's390-pv', call it 'confidential-guest-support' or 'CGS'. My reasoning is that the QEMU community (namely David Gibson, qemu-ppc maintainer) went into a lot of discussions back and forth to develop the confidential-guest-support machine option, based on what was at first AMD-SEV specific code, with the intention of make it easier for users to enable secure guests across machine types. I believe Libvirt should follow suit and do the same - a single option to enable secure guest supports for all guests, with any differences in the support being handled by each arch deep down in the driver. Otherwise, what will end up happening is that when someone (probably myself) come along with the secure guest support for pSeries (PEF), I will need to create yet another launch type 'ppc64-pef' to do basically the same thing you're already doing for s390x, which is adding '-machine confidential-guest-support=<>' in the QEMU command line. Same thing with AMD SEV, and with any other arch that QEMU might support with the confidential-guest-support option. We're going to add extra XML parsing code and docs to handle the same thing. Note that I'm not asking you to go ahead and implement the Libvirt support for all the 3 archs. What I'm asking is to change the name of the launch security type in the domain XML and docs to reflect that this will be the same type that all other archs that has confidential-guest-support will end up using. Thanks, Daniel

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 228de5d715..11ec8c8b0c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1393,6 +1393,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity, VIR_DOMAIN_LAUNCH_SECURITY_LAST, "", "sev", + "s390-pv", );

static virClass *virDomainObjClass; @@ -14762,6 +14763,8 @@ virDomainSecDefParseXML(xmlNodePtr lsecNode, if (!sec->sev) return NULL; break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: default: @@ -26896,6 +26899,11 @@ virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec) break; }

+ case VIR_DOMAIN_LAUNCH_SECURITY_PV: + virBufferAsprintf(buf, "<launchSecurity type='%s'/>\n", + virDomainLaunchSecurityTypeToString(sec->sectype)); + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index dd78f30ace..1d92065c7b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2631,6 +2631,7 @@ struct _virDomainKeyWrapDef { typedef enum { VIR_DOMAIN_LAUNCH_SECURITY_NONE, VIR_DOMAIN_LAUNCH_SECURITY_SEV, + VIR_DOMAIN_LAUNCH_SECURITY_PV,

VIR_DOMAIN_LAUNCH_SECURITY_LAST, } virDomainLaunchSecurity; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 10dcf11d5b..67024f99b9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6992,6 +6992,9 @@ qemuBuildMachineCommandLine(virCommand *cmd, case VIR_DOMAIN_LAUNCH_SECURITY_SEV: virBufferAddLit(&buf, ",memory-encryption=sev0"); break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + virBufferAddLit(&buf, ",confidential-guest-support=pv0"); + break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST: @@ -9879,6 +9882,26 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd, }

+static int +qemuBuildPVCommandLine(virDomainObj *vm, virCommand *cmd) +{ + g_autoptr(virJSONValue) props = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + qemuDomainObjPrivate *priv = vm->privateData; + + if (qemuMonitorCreateObjectProps(&props, "s390-pv-guest", "pv0", + NULL) < 0) + return -1; + + if (qemuBuildObjectCommandlineFromJSON(&buf, props, priv->qemuCaps) < 0) + return -1; + + virCommandAddArg(cmd, "-object"); + virCommandAddArgBuffer(cmd, &buf); + return 0; +} + + static int qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd, virDomainSecDef *sec) @@ -9890,6 +9913,9 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd, case VIR_DOMAIN_LAUNCH_SECURITY_SEV: return qemuBuildSEVCommandLine(vm, cmd, sec->sev); break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + return qemuBuildPVCommandLine(vm, cmd); + break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST: diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 0dd1291c5d..7cc35986da 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -607,6 +607,7 @@ qemuDomainSetupLaunchSecurity(virDomainObj *vm,

VIR_DEBUG("Set up launch security for SEV"); break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a7d88015ba..cb94979b26 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6637,6 +6637,7 @@ qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm) switch ((virDomainLaunchSecurity) sec->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: return qemuProcessPrepareSEVGuestInput(vm, sec); + case VIR_DOMAIN_LAUNCH_SECURITY_PV: case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST: diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 78582a7c2a..0dea33d08c 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1224,6 +1224,14 @@ qemuValidateDomainDef(const virDomainDef *def, return -1; } break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("S390 PV launch security is not supported with " + "this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST: diff --git a/tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml b/tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml new file mode 100644 index 0000000000..0c398cced8 --- /dev/null +++ b/tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml @@ -0,0 +1,24 @@ +<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='s390x' machine='s390-ccw-virtio'>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> + </devices> + <launchSecurity type='s390-pv'> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + <policy>0x0001</policy> + <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert> + <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session> + </launchSecurity> +</domain> diff --git a/tests/genericxml2xmlindata/launch-security-s390-pv.xml b/tests/genericxml2xmlindata/launch-security-s390-pv.xml new file mode 100644 index 0000000000..29c7fc152d --- /dev/null +++ b/tests/genericxml2xmlindata/launch-security-s390-pv.xml @@ -0,0 +1,18 @@ +<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='s390x' machine='s390-ccw-virtio'>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> + </devices> + <launchSecurity type='s390-pv'/> +</domain> diff --git a/tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml b/tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml new file mode 120000 index 0000000000..075c72603d --- /dev/null +++ b/tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml @@ -0,0 +1 @@ +../genericxml2xmlindata/launch-security-s390-pv.xml \ No newline at end of file diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index ac89422a32..eb15f66c3c 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -233,6 +233,8 @@ mymain(void) DO_TEST("tseg");

DO_TEST("launch-security-sev"); + DO_TEST("launch-security-s390-pv"); + DO_TEST_DIFFERENT("launch-security-s390-pv-ignore-policy");

DO_TEST_DIFFERENT("cputune"); DO_TEST("device-backenddomain"); diff --git a/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args new file mode 100644 index 0000000000..c9d9b84dd3 --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args @@ -0,0 +1,35 @@ +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 \ +/usr/bin/qemu-system-s390x \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine s390-ccw-virtio,accel=kvm,usb=off,dump-guest-core=off,confidential-guest-support=pv0,memory-backend=s390.ram \ +-cpu gen15a-base,aen=on,cmmnt=on,vxpdeh=on,aefsi=on,diag318=on,csske=on,mepoch=on,msa9=on,msa8=on,msa7=on,msa6=on,msa5=on,msa4=on,msa3=on,msa2=on,msa1=on,sthyi=on,edat=on,ri=on,deflate=on,edat2=on,etoken=on,vx=on,ipter=on,mepochptff=on,ap=on,vxeh=on,vxpd=on,esop=on,msa9_pckmo=on,vxeh2=on,esort=on,apqi=on,apft=on,els=on,iep=on,apqci=on,cte=on,ais=on,bpb=on,gs=on,ppa15=on,zpci=on,sea_esop2=on,te=on,cmm=on \ +-m 214 \ +-object '{"qom-type":"memory-backend-ram","id":"s390.ram","size":224395264}' \ +-overcommit mem-lock=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=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device virtio-blk-ccw,devno=fe.0.0000,drive=libvirt-1-format,id=virtio-disk0,bootindex=1 \ +-audiodev id=audio1,driver=none \ +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001 \ +-object '{"qom-type":"s390-pv-guest","id":"pv0"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml new file mode 100644 index 0000000000..052d96dedb --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml @@ -0,0 +1,33 @@ +<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='s390x' machine='s390-ccw-virtio'>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-s390x</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </disk> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/> + </memballoon> + <panic model='s390'/> + </devices> + <launchSecurity type='s390-pv'> + <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert> + <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session> + </launchSecurity> +</domain> diff --git a/tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args b/tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args new file mode 100644 index 0000000000..c9d9b84dd3 --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args @@ -0,0 +1,35 @@ +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 \ +/usr/bin/qemu-system-s390x \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine s390-ccw-virtio,accel=kvm,usb=off,dump-guest-core=off,confidential-guest-support=pv0,memory-backend=s390.ram \ +-cpu gen15a-base,aen=on,cmmnt=on,vxpdeh=on,aefsi=on,diag318=on,csske=on,mepoch=on,msa9=on,msa8=on,msa7=on,msa6=on,msa5=on,msa4=on,msa3=on,msa2=on,msa1=on,sthyi=on,edat=on,ri=on,deflate=on,edat2=on,etoken=on,vx=on,ipter=on,mepochptff=on,ap=on,vxeh=on,vxpd=on,esop=on,msa9_pckmo=on,vxeh2=on,esort=on,apqi=on,apft=on,els=on,iep=on,apqci=on,cte=on,ais=on,bpb=on,gs=on,ppa15=on,zpci=on,sea_esop2=on,te=on,cmm=on \ +-m 214 \ +-object '{"qom-type":"memory-backend-ram","id":"s390.ram","size":224395264}' \ +-overcommit mem-lock=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=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device virtio-blk-ccw,devno=fe.0.0000,drive=libvirt-1-format,id=virtio-disk0,bootindex=1 \ +-audiodev id=audio1,driver=none \ +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001 \ +-object '{"qom-type":"s390-pv-guest","id":"pv0"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/launch-security-s390-pv.xml b/tests/qemuxml2argvdata/launch-security-s390-pv.xml new file mode 100644 index 0000000000..c40c2b4bf2 --- /dev/null +++ b/tests/qemuxml2argvdata/launch-security-s390-pv.xml @@ -0,0 +1,30 @@ +<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='s390x' machine='s390-ccw-virtio'>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-s390x</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </disk> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/> + </memballoon> + <panic model='s390'/> + </devices> + <launchSecurity type='s390-pv'/> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 594a01de45..f1475dc700 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3498,6 +3498,9 @@ mymain(void) DO_TEST_CAPS_VER("launch-security-sev-missing-platform-info", "2.12.0"); DO_TEST_CAPS_VER_PARSE_ERROR("launch-security-sev-missing-policy", "2.12.0");

+ DO_TEST_CAPS_ARCH_LATEST("launch-security-s390-pv", "s390x"); + DO_TEST_CAPS_ARCH_LATEST("launch-security-s390-pv-ignore-policy", "s390x"); + DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory"); DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages"); DO_TEST_CAPS_LATEST_PARSE_ERROR("vhost-user-fs-readonly");

On 5/19/21 9:34 PM, Daniel Henrique Barboza wrote:

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

...

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   docs/schemas/domaincommon.rng                 |  1 +   src/conf/domain_conf.c                        |  8 +++++   src/conf/domain_conf.h                        |  1 +   src/qemu/qemu_command.c                       | 26 ++++++++++++++   src/qemu/qemu_namespace.c                     |  1 +   src/qemu/qemu_process.c                       |  1 +   src/qemu/qemu_validate.c                      |  8 +++++   .../launch-security-s390-pv-ignore-policy.xml | 24 +++++++++++++   .../launch-security-s390-pv.xml               | 18 ++++++++++   .../launch-security-s390-pv-ignore-policy.xml |  1 +   tests/genericxml2xmltest.c                    |  2 ++   ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++++++++++++++++++   .../launch-security-s390-pv-ignore-policy.xml | 33 +++++++++++++++++   .../launch-security-s390-pv.s390x-latest.args | 35 +++++++++++++++++++   .../launch-security-s390-pv.xml               | 30 ++++++++++++++++   tests/qemuxml2argvtest.c                      |  3 ++   16 files changed, 227 insertions(+)   create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml   create mode 120000 tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args

  create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3df13a0cf1..7c92e4c812 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -485,6 +485,7 @@         <attribute name="type">           <choice>             <value>sev</value> +          <value>s390-pv</value>           </choice>         </attribute>         <interleave>

You added a new 's390-pv' security type, but down there you're using the new confidential-guest-support feature from QEMU 6.0 which is also valid for AMD and pSeries. I think you can do a little change in the idea of these patches while keeping most of it. Instead of calling this new support 's390-pv', call it 'confidential-guest-support' or 'CGS'.

My reasoning is that the QEMU community (namely David Gibson, qemu-ppc maintainer) went into a lot of discussions back and forth to develop the confidential-guest-support machine option, based on what was at first AMD-SEV specific code, with the intention of make it easier for users to enable secure guests across machine types. I believe Libvirt should follow suit and do the same - a single option to enable secure guest supports for all guests, with any differences in the support being handled by each arch deep down in the driver.

Otherwise, what will end up happening is that when someone (probably myself) come along with the secure guest support for pSeries (PEF), I will need to create yet another launch type 'ppc64-pef' to do basically the same thing you're already doing for s390x, which is adding '-machine confidential-guest-support=<>' in the QEMU command line. Same thing with AMD SEV, and with any other arch that QEMU might support with the confidential-guest-support option. We're going to add extra XML parsing code and docs to handle the same thing.

Note that I'm not asking you to go ahead and implement the Libvirt support for all the 3 archs. What I'm asking is to change the name of the launch security type in the domain XML and docs to reflect that this will be the same type that all other archs that has confidential-guest-support will end up using.

Thanks,

Daniel

Daniel, thanks for your review and feedback. When I looked at the QEMU commit 590466f056c4f https://git.qemu.org/?p=qemu.git;a=commit;h=590466f056c4f2a7ff87ed751cece4f4... I did not get the impression that there is a common type for confidential guest support possible as the requiered data per type differs. Also I got the impression that the different types are not necessarily architecture bound. I may have gotten the wrong impression. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 5/20/21 2:08 PM, Daniel Henrique Barboza wrote:

On 5/20/21 4:05 AM, Boris Fiuczynski wrote:

...

On 5/19/21 9:34 PM, Daniel Henrique Barboza wrote:

...

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

...

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   docs/schemas/domaincommon.rng                 |  1 +   src/conf/domain_conf.c                        |  8 +++++   src/conf/domain_conf.h                        |  1 +   src/qemu/qemu_command.c                       | 26 ++++++++++++++   src/qemu/qemu_namespace.c                     |  1 +   src/qemu/qemu_process.c                       |  1 +   src/qemu/qemu_validate.c                      |  8 +++++   .../launch-security-s390-pv-ignore-policy.xml | 24 +++++++++++++   .../launch-security-s390-pv.xml               | 18 ++++++++++   .../launch-security-s390-pv-ignore-policy.xml |  1 +   tests/genericxml2xmltest.c                    |  2 ++   ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++++++++++++++++++   .../launch-security-s390-pv-ignore-policy.xml | 33 +++++++++++++++++   .../launch-security-s390-pv.s390x-latest.args | 35 +++++++++++++++++++   .../launch-security-s390-pv.xml               | 30 ++++++++++++++++   tests/qemuxml2argvtest.c                      |  3 ++   16 files changed, 227 insertions(+)   create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml   create mode 120000 tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args

  create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3df13a0cf1..7c92e4c812 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -485,6 +485,7 @@         <attribute name="type">           <choice>             <value>sev</value> +          <value>s390-pv</value>           </choice>         </attribute>         <interleave>

You added a new 's390-pv' security type, but down there you're using the new confidential-guest-support feature from QEMU 6.0 which is also valid for AMD and pSeries. I think you can do a little change in the idea of these patches while keeping most of it. Instead of calling this new support 's390-pv', call it 'confidential-guest-support' or 'CGS'.

My reasoning is that the QEMU community (namely David Gibson, qemu-ppc maintainer) went into a lot of discussions back and forth to develop the confidential-guest-support machine option, based on what was at first AMD-SEV specific code, with the intention of make it easier for users to enable secure guests across machine types. I believe Libvirt should follow suit and do the same - a single option to enable secure guest supports for all guests, with any differences in the support being handled by each arch deep down in the driver.

Otherwise, what will end up happening is that when someone (probably myself) come along with the secure guest support for pSeries (PEF), I will need to create yet another launch type 'ppc64-pef' to do basically the same thing you're already doing for s390x, which is adding '-machine confidential-guest-support=<>' in the QEMU command line. Same thing with AMD SEV, and with any other arch that QEMU might support with the confidential-guest-support option. We're going to add extra XML parsing code and docs to handle the same thing.

Note that I'm not asking you to go ahead and implement the Libvirt support for all the 3 archs. What I'm asking is to change the name of the launch security type in the domain XML and docs to reflect that this will be the same type that all other archs that has confidential-guest-support will end up using.

Thanks,

Daniel

Daniel, thanks for your review and feedback. When I looked at the QEMU commit 590466f056c4f https://git.qemu.org/?p=qemu.git;a=commit;h=590466f056c4f2a7ff87ed751cece4f4...

I did not get the impression that there is a common type for confidential guest support possible as the requiered data per type differs. Also I got the impression that the different types are not necessarily architecture bound. I may have gotten the wrong impression.

The point I tried to make is that, although there are different types of secure VMs in QEMU, this doesn't mean that we need different launch security types in Libvirt since they're all tied in the same QEMU machine option. If you check the QEMU confidential-guest-support docs:

https://gitlab.com/qemu-project/qemu/-/blob/master/docs/confidential-guest-s...

---- To run a confidential guest you need to add two command line parameters:

1. Use "-object" to create a "confidential guest support" object.  The    type and parameters will vary with the specific mechanism to be    used 2. Set the "confidential-guest-support" machine parameter to the ID of    the object from (1). ----

This is true for all 3 currently supported types. For s390x PV:

    -object s390-pv-guest,id=pv0 \     -machine confidential-guest-support=pv0 \

AMD SEV:

    -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \     -machine confidential-guest-support=sev0 \

PAPR PEF:

    -object pef-guest,id=pef0 \     -machine confidential-guest-support=pef0 \

All the above requires libvirt code (capability checks, validation, command line generation etc) to exist per individual type that you try to hide from the user. Below the user is required to know that when he wants to use SEV he has to specify 'confidential-guest-support' as type AND in addition is also required to provide a policy for SEV. So it is not transparent for users maybe even more confusing since the old type sev must not be eliminated for backward compatibility reasons. So I agree that there is one "knob" but you need to potentially provide different data and pre-checks when "pressing" it... just looking at sev, pef and pv and not knowing what Intel's TDX requires. But I agree with you that at it's core its a matter of taste/design.

The difference between them, aside from the object types, is that AMD has configurable arguments to deal with. So my proposal is to, instead of creating a s390-pv launch type, you create a confidential-guest-support type:

  <launchSecurity type='confidential-guest-support' />

This will translate, for s390x guests, into s390-pv. All validations and constraints specific to s390-pv applies. This same XML for a PAPR guest will trigger papr-pef code, and AMD-SEV code for AMD guests. AMD will probably want to add more arguments, so they would want to extend it like it is already done with their current 'sev' launcher:

  <launchSecurity type='confidential-guest-support'>     <policy>0x0001</policy>     <cbitpos>47</cbitpos>     <reducedPhysBits>1</reducedPhysBits>     <dhCert>RBBBSDDD=FDDCCCDDDG</dhCert>     <session>AAACCCDD=FFFCCCDSDS</session>   </launchSecurity>

The advantage is that the same launchSecurity type will enable confidential-guest-support for everyone, sparing a bit of XML parsing and docs we would have if each arch adds a new launch type that maps to its own secure VM technology. Note that you can (and should) clarify in the docs that the confidential-guest-support for s390x guests is the s390-pf technology and mention all applicable features/limitations of it. This also will make for an easier time for other Libvirt users that has to deal with multiple archs (not sure there are many, but ...) since the same option enables secure VM for all archs.

I think you can argue that it's a matter of taste/design and that we should instead add a launcherSecurity type for each secure VM technology, regardless of how QEMU see all of them as a 'secure VM knob'. I would like to hear other opinions (specially from AMD) about what they feel about it. I'd rather have the same launcherSecurity type for everyone.

Thanks,

Daniel

...

-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Thu, May 20, 2021 at 09:08:28AM -0300, Daniel Henrique Barboza wrote:

On 5/20/21 4:05 AM, Boris Fiuczynski wrote:

...

On 5/19/21 9:34 PM, Daniel Henrique Barboza wrote:

...

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

...

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   docs/schemas/domaincommon.rng                 |  1 +   src/conf/domain_conf.c                        |  8 +++++   src/conf/domain_conf.h                        |  1 +   src/qemu/qemu_command.c                       | 26 ++++++++++++++   src/qemu/qemu_namespace.c                     |  1 +   src/qemu/qemu_process.c                       |  1 +   src/qemu/qemu_validate.c                      |  8 +++++   .../launch-security-s390-pv-ignore-policy.xml | 24 +++++++++++++   .../launch-security-s390-pv.xml               | 18 ++++++++++   .../launch-security-s390-pv-ignore-policy.xml |  1 +   tests/genericxml2xmltest.c                    |  2 ++   ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++++++++++++++++++   .../launch-security-s390-pv-ignore-policy.xml | 33 +++++++++++++++++   .../launch-security-s390-pv.s390x-latest.args | 35 +++++++++++++++++++   .../launch-security-s390-pv.xml               | 30 ++++++++++++++++   tests/qemuxml2argvtest.c                      |  3 ++   16 files changed, 227 insertions(+)   create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml   create mode 120000 tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3df13a0cf1..7c92e4c812 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -485,6 +485,7 @@         <attribute name="type">           <choice>             <value>sev</value> +          <value>s390-pv</value>           </choice>         </attribute>         <interleave>

You added a new 's390-pv' security type, but down there you're using the new confidential-guest-support feature from QEMU 6.0 which is also valid for AMD and pSeries. I think you can do a little change in the idea of these patches while keeping most of it. Instead of calling this new support 's390-pv', call it 'confidential-guest-support' or 'CGS'.

My reasoning is that the QEMU community (namely David Gibson, qemu-ppc maintainer) went into a lot of discussions back and forth to develop the confidential-guest-support machine option, based on what was at first AMD-SEV specific code, with the intention of make it easier for users to enable secure guests across machine types. I believe Libvirt should follow suit and do the same - a single option to enable secure guest supports for all guests, with any differences in the support being handled by each arch deep down in the driver.

Otherwise, what will end up happening is that when someone (probably myself) come along with the secure guest support for pSeries (PEF), I will need to create yet another launch type 'ppc64-pef' to do basically the same thing you're already doing for s390x, which is adding '-machine confidential-guest-support=<>' in the QEMU command line. Same thing with AMD SEV, and with any other arch that QEMU might support with the confidential-guest-support option. We're going to add extra XML parsing code and docs to handle the same thing.

Note that I'm not asking you to go ahead and implement the Libvirt support for all the 3 archs. What I'm asking is to change the name of the launch security type in the domain XML and docs to reflect that this will be the same type that all other archs that has confidential-guest-support will end up using.

Thanks,

Daniel

Daniel, thanks for your review and feedback. When I looked at the QEMU commit 590466f056c4f https://git.qemu.org/?p=qemu.git;a=commit;h=590466f056c4f2a7ff87ed751cece4f4... I did not get the impression that there is a common type for confidential guest support possible as the requiered data per type differs. Also I got the impression that the different types are not necessarily architecture bound. I may have gotten the wrong impression.

The point I tried to make is that, although there are different types of secure VMs in QEMU, this doesn't mean that we need different launch security types in Libvirt since they're all tied in the same QEMU machine option. If you check the QEMU confidential-guest-support docs:

https://gitlab.com/qemu-project/qemu/-/blob/master/docs/confidential-guest-s...

---- To run a confidential guest you need to add two command line parameters:

1. Use "-object" to create a "confidential guest support" object. The type and parameters will vary with the specific mechanism to be used 2. Set the "confidential-guest-support" machine parameter to the ID of the object from (1). ----

This is true for all 3 currently supported types. For s390x PV:

-object s390-pv-guest,id=pv0 \ -machine confidential-guest-support=pv0 \

AMD SEV:

-object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \ -machine confidential-guest-support=sev0 \

PAPR PEF:

-object pef-guest,id=pef0 \ -machine confidential-guest-support=pef0 \

The difference between them, aside from the object types, is that AMD has configurable arguments to deal with. So my proposal is to, instead of creating a s390-pv launch type, you create a confidential-guest-support type:

<launchSecurity type='confidential-guest-support' />

I strongly disagree with this proposal. What would libvirt do if any vendor decides that the current implementation is not good enough and introduces a new better/different/improved but keeps the old one around? We would have to add a sub-element or attribute or something else to give users the possibility to pick the specific implementation.

This will translate, for s390x guests, into s390-pv. All validations and constraints specific to s390-pv applies. This same XML for a PAPR guest will trigger papr-pef code, and AMD-SEV code for AMD guests. AMD will probably want to add more arguments, so they would want to extend it like it is already done with their current 'sev' launcher:

<launchSecurity type='confidential-guest-support'> <policy>0x0001</policy> <cbitpos>47</cbitpos> <reducedPhysBits>1</reducedPhysBits> <dhCert>RBBBSDDD=FDDCCCDDDG</dhCert> <session>AAACCCDD=FFFCCCDSDS</session> </launchSecurity>

The advantage is that the same launchSecurity type will enable confidential-guest-support for everyone, sparing a bit of XML parsing and

What if in the future we will have options for other confidential guests as well and not only AMD SEV? The advantage that one XML would enable it on all architectures would be gone or user would have to provide options for all architectures to make it work everywhere and libvirt would have to ignore the non-relevant options. This doesn't sound like a good idea to me. In libvirt we don't have to make a fancy knob which would do some magic to enable it on all architectures. That is a job for upper layers. Libvirt should provide necessary info in domcapabilities which can be used by upper layers to pick the correct type and provide correct VM XML with all the possible options. The original patch adding a new launchSecurity type is what we should do in libvirt.

docs we would have if each arch adds a new launch type that maps to its own secure VM technology. Note that you can (and should) clarify in the docs that the confidential-guest-support for s390x guests is the s390-pf technology and mention all applicable features/limitations of it. This also will make for an easier time for other Libvirt users that has to deal with multiple archs (not sure there are many, but ...) since the same option enables secure VM for all archs.

I think you can argue that it's a matter of taste/design and that we should instead add a launcherSecurity type for each secure VM technology, regardless of how QEMU see all of them as a 'secure VM knob'. I would like to hear other opinions (specially from AMD) about what they feel about it. I'd rather have the same launcherSecurity type for everyone.

The fact that you use confidential-guest-support is not a valid argument for having only single type in libvirt XML. We need to create an -object with correct type and specify that object using confidential-guest-support option. Pavel

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

Add documentation for launch security type s390-pv.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/formatdomain.rst | 7 ++++ docs/kbase/s390_protected_virt.rst | 55 +++++++++++++++++++++++++----- 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index fa5c14febc..635fd5a55f 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -8044,6 +8044,13 @@ Note: DEA/TDEA is synonymous with DES/TDES. Launch Security ---------------

+Specifying ``<launchSecurity type='s390-pv'\>`` in an s390 VM prepares for +running the guest in protected virtualization secure mode, also known as +IBM Secure Execution. For more required host and guest preparation steps, see

The flow is a bit awkward here. Perhaps something like: "Specifying ``<launchSecurity type='s390-pv'\>`` in a s390 domain prepares the guest to run in protected virtualization secure mode, also known as IBM Secure Execution. (...)" Other than that, Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

+`Protected Virtualization on s390 <kbase/s390_protected_virt.html>`__ +:since:`Since 7.4.0` + + The contents of the ``<launchSecurity type='sev'>`` element is used to provide the guest owners input used for creating an encrypted VM using the AMD SEV feature (Secure Encrypted Virtualization). SEV is an extension to the AMD-V diff --git a/docs/kbase/s390_protected_virt.rst b/docs/kbase/s390_protected_virt.rst index 1718a556d4..5c9ce0e21c 100644 --- a/docs/kbase/s390_protected_virt.rst +++ b/docs/kbase/s390_protected_virt.rst @@ -127,10 +127,13 @@ Protected virtualization guests support I/O using virtio devices. As the virtio data structures of secure guests are not accessible by the host, it is necessary to use shared memory ('bounce buffers').

-To enable virtio devices to use shared buffers, it is necessary -to configure them with platform_iommu enabled. This can done by adding -``iommu='on'`` to the driver element of a virtio device definition in the -guest's XML, e.g. +Since libvirt 7.4.0 the +`<launchSecurity> <https://libvirt.org/formatdomain.html#launchSecurity>`__ +element with type ``s390-pv`` should be used on protected virtualization guests. +Without ``launchSecurity`` you must enable all virtio devices to use shared +buffers by configuring them with platform_iommu enabled. +This can done by adding ``iommu='on'`` to the driver element of a virtio +device definition in the guest's XML, e.g.

::

@@ -140,8 +143,10 @@ guest's XML, e.g. <driver name='vhost' iommu='on'/> </interface>

-It is mandatory to define all virtio bus devices in this way to -prevent the host from attempting to access protected memory. +Unless you are using ``launchSecurity`` you must define all virtio bus +devices in this way to prevent the host from attempting to access +protected memory. + Ballooning will not work and is fenced by QEMU. It should be disabled by specifying

@@ -158,8 +163,42 @@ allocated 2K entries. A commonly used value for swiotlb is 262144. Example guest definition ========================

-Minimal domain XML for a protected virtualization guest, essentially -it's mostly about the ``iommu`` property +Minimal domain XML for a protected virtualization guest with +the ``launchSecurity`` element of type ``s390-pv`` + +:: + + <domain type='kvm'> + <name>protected</name> + <memory unit='KiB'>2048000</memory> + <currentMemory unit='KiB'>2048000</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='s390x'>hvm</type> + </os> + <cpu mode='host-model'/> + <devices> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none' io='native'> + <source file='/var/lib/libvirt/images/protected.qcow2'/> + <target dev='vda' bus='virtio'/> + </disk> + <interface type='network'> + <source network='default'/> + <model type='virtio'/> + </interface> + <console type='pty'/> + <memballoon model='none'/> + </devices> + <launchSecurity type='s390-pv'/> + </domain> + + +Example guest definition without launchSecurity +=============================================== + +Minimal domain XML for a protected virtualization guest using the +``iommu='on'`` setting for each virtio device.

::