On 6/22/21 10:10 AM, Boris Fiuczynski wrote:

Make use of virDomainLaunchSecurity enum and automatic memory freeing.

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

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

src/conf/domain_conf.c | 123 +++++++++++++++++++++-------------------- src/conf/domain_conf.h | 2 + 2 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index af2fd03d3c..93ec50ff5d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3490,7 +3490,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl) }

-static void +void virDomainSEVDefFree(virDomainSEVDef *def) { if (!def) @@ -14719,73 +14719,66 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - virDomainSEVDef *def; unsigned long policy; g_autofree char *type = NULL; int rc = -1;

- def = g_new0(virDomainSEVDef, 1); + g_autoptr(virDomainSEVDef) 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; + return NULL; }

def->sectype = virDomainLaunchSecurityTypeFromString(type); switch ((virDomainLaunchSecurity) def->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: - break; + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("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); + if (rc == 0) { + def->haveCbitpos = true; + } else if (rc == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid format for launch security cbitpos")); + return NULL; + } + + 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")); + return NULL; + } + + def->policy = policy; + def->dh_cert = virXPathString("string(./dhCert)", ctxt); + def->session = virXPathString("string(./session)", ctxt); + + return g_steal_pointer(&def); 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 for " - "launch security type SEV")); - 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; + return NULL; } - - def->policy = policy; - def->dh_cert = virXPathString("string(./dhCert)", ctxt); - def->session = virXPathString("string(./session)", ctxt); - - return def; - - error: - virDomainSEVDefFree(def); - return NULL; }

@@ -26844,25 +26837,33 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) if (!sev) return;

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

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

- virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</launchSecurity>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</launchSecurity>\n"); + 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 f706c498ff..512c7c8bd7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3285,6 +3285,8 @@ 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 virDomainDeviceDefFree(virDomainDeviceDef *def);

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);

On 6/25/21 10:39 AM, Pavel Hrdina wrote:

On Tue, Jun 22, 2021 at 03:10:45PM +0200, Boris Fiuczynski wrote:

...

Make use of virDomainLaunchSecurity enum and automatic memory freeing.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 123 +++++++++++++++++++++-------------------- src/conf/domain_conf.h | 2 + 2 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index af2fd03d3c..93ec50ff5d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3490,7 +3490,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl) }

-static void +void virDomainSEVDefFree(virDomainSEVDef *def) { if (!def) @@ -14719,73 +14719,66 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - virDomainSEVDef *def; unsigned long policy; g_autofree char *type = NULL; int rc = -1;

No need for this empty line.

...

- def = g_new0(virDomainSEVDef, 1); + g_autoptr(virDomainSEVDef) 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; + return NULL; }

def->sectype = virDomainLaunchSecurityTypeFromString(type); switch ((virDomainLaunchSecurity) def->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: - break; + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("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); + if (rc == 0) { + def->haveCbitpos = true; + } else if (rc == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid format for launch security cbitpos")); + return NULL; + } + + 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")); + return NULL; + } + + def->policy = policy; + def->dh_cert = virXPathString("string(./dhCert)", ctxt); + def->session = virXPathString("string(./session)", ctxt); + + return g_steal_pointer(&def); 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 for " - "launch security type SEV")); - 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; + return NULL; } - - def->policy = policy; - def->dh_cert = virXPathString("string(./dhCert)", ctxt); - def->session = virXPathString("string(./session)", ctxt); - - return def; - - error: - virDomainSEVDefFree(def); - return NULL; }

@@ -26844,25 +26837,33 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) if (!sev) return;

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

I would keep this line out out of the switch as it can be reused by other launchSecurity types.

In the spirit of modernizing the parse/format code you can user the following pattern witch will make addition of other types with sub-elements easier:

g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;

virBufferAsprintf(buf, " type='%s'", virDomainLaunchSecurityTypeToString(sev->sectype));

switch ((virDomainLaunchSecurity) sev->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: if (sev->haveCbitpos) virBufferAsprintf(childBuf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);

...

}

virXMLFormatElement(buf, "launchSecurity", &attrBuf, &childBuf);

With this helper we don't have to duplicate the code formatting launchSecurity type and it will also correctly handle if there are any sub-elements or not.

Otherwise looks good.

Pavel

Good idea. I will change it. Thanks for the review.

...

+ virBufferAdjustIndent(buf, 2);

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

- virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</launchSecurity>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</launchSecurity>\n"); + 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 f706c498ff..512c7c8bd7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3285,6 +3285,8 @@ 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 virDomainDeviceDefFree(virDomainDeviceDef *def);

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree); -- 2.30.2

-- 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 6/22/21 10:10 AM, Boris Fiuczynski wrote:

Adding virDomainSecDef for general launch security data and moving virDomainSEVDef as an element for SEV data.

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

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

src/conf/domain_conf.c | 127 +++++++++++++++++++++++------------- src/conf/domain_conf.h | 11 +++- src/conf/virconftypes.h | 2 + src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 44 +++++++++++-- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_firmware.c | 33 ++++++---- src/qemu/qemu_namespace.c | 20 ++++-- src/qemu/qemu_process.c | 33 ++++++++-- src/qemu/qemu_validate.c | 22 +++++-- src/security/security_dac.c | 6 +- 11 files changed, 218 insertions(+), 87 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 93ec50ff5d..2bd5210a16 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def) g_free(def); }

+ +void +virDomainSecDefFree(virDomainSecDef *def) +{ + if (!def) + return; + + virDomainSEVDefFree(def->sev); + + g_free(def); +} + + static void virDomainOSDefClear(virDomainOSDef *os) { @@ -3703,7 +3716,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);

@@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, { VIR_XPATH_NODE_AUTORESTORE(ctxt) unsigned long policy; - g_autofree char *type = NULL; int rc = -1;

g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);

ctxt->node = sevNode;

- if (!(type = virXMLPropString(sevNode, "type"))) { + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing launch security type")); + _("failed to get launch security policy for " + "launch security type SEV")); return NULL; }

- def->sectype = virDomainLaunchSecurityTypeFromString(type); - switch ((virDomainLaunchSecurity) def->sectype) { - case VIR_DOMAIN_LAUNCH_SECURITY_SEV: - if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("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); + if (rc == 0) { + def->haveCbitpos = true; + } else if (rc == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid format for launch security cbitpos")); + 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); - if (rc == 0) { - def->haveCbitpos = true; - } else if (rc == -2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid format for launch security cbitpos")); - return NULL; - } + 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")); + return NULL; + }

- 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")); - return NULL; - } + def->policy = policy; + def->dh_cert = virXPathString("string(./dhCert)", ctxt); + def->session = virXPathString("string(./session)", ctxt); + + return g_steal_pointer(&def); +} + + +static virDomainSecDef * +virDomainSecDefParseXML(xmlNodePtr lsecNode, + xmlXPathContextPtr ctxt) +{ + g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1); + g_autofree char *type = NULL;

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

- return g_steal_pointer(&def); + 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: @@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, type); return NULL; } + + return g_steal_pointer(&sec); }

@@ -20098,10 +20128,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; }

@@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)

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

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

if (sev->haveCbitpos) @@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) virBufferAddLit(buf, "</launchSecurity>\n"); break; } + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: break; @@ -28272,7 +28307,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 512c7c8bd7..fa7ab1895d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2651,7 +2651,6 @@ typedef enum {

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

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

typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL, @@ -2871,8 +2874,8 @@ struct _virDomainDef {

virDomainKeyWrapDef *keywrap;

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

/* Application-specific custom metadata */ xmlNodePtr metadata; @@ -3287,6 +3290,8 @@ 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 b21068486e..21420ba8ea 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -202,6 +202,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 038d6478b2..f2d99abcfa 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -856,7 +856,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 ea513693f7..4135a8444a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM)) qemuAppendLoadparmMachineParm(&buf, def);

- if (def->sev) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) { - virBufferAddLit(&buf, ",confidential-guest-support=sev0"); - } else { - virBufferAddLit(&buf, ",memory-encryption=sev0"); + if (def->sec) { + switch ((virDomainLaunchSecurity) def->sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) { + virBufferAddLit(&buf, ",confidential-guest-support=sev0"); + } else { + 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; } }

@@ -9860,6 +9869,29 @@ 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) @@ -10559,7 +10591,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 235f575901..9973875092 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19830,7 +19830,8 @@ 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 e17b024b06..6d1bab181e 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; }

- if (def->sev && - def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { - if (!supportsSEV) { - VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", - path); - return false; - } + if (def->sec) { + switch ((virDomainLaunchSecurity) def->sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + if (!supportsSEV) { + VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", + path); + return false; + }

- if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY && - !supportsSEVES) { - VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it", - path); - return false; + if (def->sec->sev && + def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY && + !supportsSEVES) { + VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it", + path); + return false; + } + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); + return -1; } }

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 98495e8ef8..35c8eb83fd 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 2b03b0ab98..d9073fb3a3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6480,7 +6480,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 @@ -6636,7 +6636,8 @@ 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; @@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm, static int qemuProcessPrepareSEVGuestInput(virDomainObj *vm) { - virDomainSEVDef *sev = vm->def->sev; + virDomainSEVDef *sev = vm->def->sec->sev;

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

VIR_DEBUG("Preparing SEV guest");

@@ -6695,6 +6696,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); + 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, @@ -6874,7 +6897,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 382473d03b..957dbc906c 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 4909107fcc..b874dd4ab6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1958,7 +1958,8 @@ 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; } @@ -2165,7 +2166,8 @@ 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; }

On 6/25/21 10:51 AM, Pavel Hrdina wrote:

On Tue, Jun 22, 2021 at 03:10:46PM +0200, Boris Fiuczynski wrote:

...

Adding virDomainSecDef for general launch security data and moving virDomainSEVDef as an element for SEV data.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 127 +++++++++++++++++++++++------------- src/conf/domain_conf.h | 11 +++- src/conf/virconftypes.h | 2 + src/qemu/qemu_cgroup.c | 4 +- src/qemu/qemu_command.c | 44 +++++++++++-- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_firmware.c | 33 ++++++---- src/qemu/qemu_namespace.c | 20 ++++-- src/qemu/qemu_process.c | 33 ++++++++-- src/qemu/qemu_validate.c | 22 +++++-- src/security/security_dac.c | 6 +- 11 files changed, 218 insertions(+), 87 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 93ec50ff5d..2bd5210a16 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def) g_free(def); }

+ +void +virDomainSecDefFree(virDomainSecDef *def) +{ + if (!def) + return; + + virDomainSEVDefFree(def->sev); + + g_free(def); +} + + static void virDomainOSDefClear(virDomainOSDef *os) { @@ -3703,7 +3716,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);

@@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, { VIR_XPATH_NODE_AUTORESTORE(ctxt) unsigned long policy; - g_autofree char *type = NULL; int rc = -1;

g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);

ctxt->node = sevNode;

- if (!(type = virXMLPropString(sevNode, "type"))) { + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing launch security type")); + _("failed to get launch security policy for " + "launch security type SEV")); return NULL; }

- def->sectype = virDomainLaunchSecurityTypeFromString(type); - switch ((virDomainLaunchSecurity) def->sectype) { - case VIR_DOMAIN_LAUNCH_SECURITY_SEV: - if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("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); + if (rc == 0) { + def->haveCbitpos = true; + } else if (rc == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid format for launch security cbitpos")); + 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); - if (rc == 0) { - def->haveCbitpos = true; - } else if (rc == -2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid format for launch security cbitpos")); - return NULL; - } + 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")); + return NULL; + }

- 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")); - return NULL; - } + def->policy = policy; + def->dh_cert = virXPathString("string(./dhCert)", ctxt); + def->session = virXPathString("string(./session)", ctxt); + + return g_steal_pointer(&def); +} + + +static virDomainSecDef * +virDomainSecDefParseXML(xmlNodePtr lsecNode, + xmlXPathContextPtr ctxt) +{ + g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1); + g_autofree char *type = NULL;

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

- return g_steal_pointer(&def); + 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: @@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, type); return NULL; } + + return g_steal_pointer(&sec); }

@@ -20098,10 +20128,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; }

@@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap)

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

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

if (sev->haveCbitpos) @@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) virBufferAddLit(buf, "</launchSecurity>\n"); break; } + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: break; @@ -28272,7 +28307,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 512c7c8bd7..fa7ab1895d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2651,7 +2651,6 @@ typedef enum {

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

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

I would rather use union here like we do in other similar internal structures:

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

or

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

depending if we need to have the specific SEV structure as pointer or not based on its usage. I personally think we can do it without the pointer as it should not happen that sectype will be set to SEV but we will not have any data.

OK, will change it.

Pavel

...

+};

typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL, @@ -2871,8 +2874,8 @@ struct _virDomainDef {

virDomainKeyWrapDef *keywrap;

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

/* Application-specific custom metadata */ xmlNodePtr metadata; @@ -3287,6 +3290,8 @@ 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 b21068486e..21420ba8ea 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -202,6 +202,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 038d6478b2..f2d99abcfa 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -856,7 +856,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 ea513693f7..4135a8444a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM)) qemuAppendLoadparmMachineParm(&buf, def);

- if (def->sev) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) { - virBufferAddLit(&buf, ",confidential-guest-support=sev0"); - } else { - virBufferAddLit(&buf, ",memory-encryption=sev0"); + if (def->sec) { + switch ((virDomainLaunchSecurity) def->sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) { + virBufferAddLit(&buf, ",confidential-guest-support=sev0"); + } else { + 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; } }

@@ -9860,6 +9869,29 @@ 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) @@ -10559,7 +10591,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 235f575901..9973875092 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19830,7 +19830,8 @@ 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 e17b024b06..6d1bab181e 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; }

- if (def->sev && - def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { - if (!supportsSEV) { - VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", - path); - return false; - } + if (def->sec) { + switch ((virDomainLaunchSecurity) def->sec->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + if (!supportsSEV) { + VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", + path); + return false; + }

- if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY && - !supportsSEVES) { - VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it", - path); - return false; + if (def->sec->sev && + def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY && + !supportsSEVES) { + VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it", + path); + return false; + } + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); + return -1; } }

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 98495e8ef8..35c8eb83fd 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 2b03b0ab98..d9073fb3a3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6480,7 +6480,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 @@ -6636,7 +6636,8 @@ 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; @@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm, static int qemuProcessPrepareSEVGuestInput(virDomainObj *vm) { - virDomainSEVDef *sev = vm->def->sev; + virDomainSEVDef *sev = vm->def->sec->sev;

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

This should not happen as we would abort if allocation of virDomainSEVDef failed. In addition if we go with the union where the data would not be a pointer there is no need for this check at all.

OK, I will remove it.

Pavel

...

VIR_DEBUG("Preparing SEV guest");

@@ -6695,6 +6696,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); + 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, @@ -6874,7 +6897,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 382473d03b..957dbc906c 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 4909107fcc..b874dd4ab6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1958,7 +1958,8 @@ 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; } @@ -2165,7 +2166,8 @@ 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; } -- 2.30.2

-- 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 Tue, Jun 22, 2021 at 03:10:48PM +0200, Boris Fiuczynski wrote:

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

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.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_firmware.c | 1 + src/qemu/qemu_namespace.c | 1 + src/qemu/qemu_process.c | 1 + src/qemu/qemu_validate.c | 9 +++++ .../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 ++ 17 files changed, 229 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 8c1b6c3a09..b81c51728d 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> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2bd5210a16..a7fc8cd65f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1401,6 +1401,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity, VIR_DOMAIN_LAUNCH_SECURITY_LAST, "", "sev", + "s390-pv", );

static virClass *virDomainObjClass; @@ -14799,6 +14800,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: @@ -26895,6 +26898,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 fa7ab1895d..9d9acab50c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2645,6 +2645,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 4135a8444a..3ab803f7ce 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6975,6 +6975,9 @@ qemuBuildMachineCommandLine(virCommand *cmd, virBufferAddLit(&buf, ",memory-encryption=sev0"); } break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + virBufferAddLit(&buf, ",confidential-guest-support=pv0"); + break;

This could be possible shared for all launchSecurity types as well but it can be done as followup. That would mean using for example lsec0 instead of sev0, pv0, somethingelse0 and so on. It's just an id which can be anything.

case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST: @@ -9870,6 +9873,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) @@ -9881,6 +9904,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_firmware.c b/src/qemu/qemu_firmware.c index 6d1bab181e..3b408fa7b8 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1070,6 +1070,7 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; } 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_namespace.c b/src/qemu/qemu_namespace.c index 35c8eb83fd..156ee84292 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 d9073fb3a3..9f9904cc6c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6707,6 +6707,7 @@ qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm) switch ((virDomainLaunchSecurity) sec->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: return qemuProcessPrepareSEVGuestInput(vm); + 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 957dbc906c..c39dc5136d 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1224,6 +1224,15 @@ qemuValidateDomainDef(const virDomainDef *def, return -1; } break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) || + !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>

I thing we should not ignore invalid XML bits and error out instead.

+ </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>

This doesn't look correct, we should not format dhCert or session with s390-pv because based on the patches they are not used at all. Pavel

+ </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 ef6afae586..3268e4fe37 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3461,6 +3461,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"); -- 2.30.2

On Fri, Jul 02, 2021 at 03:46:43PM +0200, Boris Fiuczynski wrote:

On 6/25/21 12:00 PM, Pavel Hrdina wrote:

...

On Tue, Jun 22, 2021 at 03:10:48PM +0200, Boris Fiuczynski wrote:

...

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

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.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_firmware.c | 1 + src/qemu/qemu_namespace.c | 1 + src/qemu/qemu_process.c | 1 + src/qemu/qemu_validate.c | 9 +++++ .../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 ++ 17 files changed, 229 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

<snip>

...

...

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4135a8444a..3ab803f7ce 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6975,6 +6975,9 @@ qemuBuildMachineCommandLine(virCommand *cmd, virBufferAddLit(&buf, ",memory-encryption=sev0"); } break; + case VIR_DOMAIN_LAUNCH_SECURITY_PV: + virBufferAddLit(&buf, ",confidential-guest-support=pv0"); + break;

This could be possible shared for all launchSecurity types as well but it can be done as followup. That would mean using for example lsec0 instead of sev0, pv0, somethingelse0 and so on. It's just an id which can be anything.

I will add an follow-up patch which changes the id to a common id 'lsec0'.

...

...

case VIR_DOMAIN_LAUNCH_SECURITY_NONE: break; case VIR_DOMAIN_LAUNCH_SECURITY_LAST:

<snip/>

...

...

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>

I thing we should not ignore invalid XML bits and error out instead.

This would result in s390-pv checking for the existence of any child elements and if there is a child fail. If other launchSecurity types come along with new or shared child elements checking for SEV and the new types would have to added/changed. Wouldn't that get messy quickly?

No necessarily, one approach that we use is to do a while() loop over all elements, match only known elements and for unknown print error. For each launchSecurity type we will have dedicated parser function where we would do the while loop where we would error out for any unknown sub-element. In case of s390-pv-guest we would simply error out if there is any sub-element. Looking at the current code we don't use this approach for SEV so keep the code as it is. In this XML-TO-XML test it makes sense to validate that the unknown sub-elements are ignored, but ... [1]

...

...

+ </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>

This doesn't look correct, we should not format dhCert or session with s390-pv because based on the patches they are not used at all.

Isn't this the same issue as before about unsued elements being ignored?

[1] ... here it is useless as it is already tested by XML-TO-XML, so the ignore variant can be dropped from XML-TO-ARGV. Pavel

On Tue, Jun 22, 2021 at 03:10:49PM +0200, Boris Fiuczynski wrote:

Add documentation for launch security type s390-pv.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.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 c6dede053f..a1b028c4ad 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -8078,6 +8078,13 @@ Note: DEA/TDEA is synonymous with DES/TDES. Launch Security ---------------

+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. For more required host and guest preparation steps, see +`Protected Virtualization on s390 <kbase/s390_protected_virt.html>`__ +:since:`Since 7.5.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..66203568d9 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.5.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.

I don't know how s390-pv works but for example with AMD SEV it is required to use `iommu='on'` otherwise the device is not visible inside the VM so I would like to make sure there is no misunderstanding and it is correct. Pavel

::

-- 2.30.2