RE: [RFC PATCH 3/7] conf: introduce TrustDomain element in domain

-----Original Message----- From: Peter Krempa <pkrempa(a)redhat.com&gt; Sent: Friday, June 18, 2021 7:17 PM To: Duan, Zhenzhong <zhenzhong.duan(a)intel.com&gt; Cc: libvir-list(a)redhat.com; Yamahata, Isaku <isaku.yamahata(a)intel.com>; Tian, Jun J <jun.j.tian(a)intel.com>; Qiang, Chenyi <chenyi.qiang(a)intel.com&gt; Subject: Re: [RFC PATCH 3/7] conf: introduce TrustDomain element in domain On Fri, Jun 18, 2021 at 16:50:48 +0800, Zhenzhong Duan wrote: > The TrustDomain element can be used to define the security model to > use when launching a domain. Only type 'tdx' is supported currently. > > When 'tdx' is used, the VM will launched with Intel TDX feature enabled. > TDX feature supports running encrypted VM (Trust Domain, TD) under the > control of KVM. A TD runs in a CPU model which protects the > confidentiality of its memory and its CPU state from other software > > There is a child element 'policy' in TrustDomain. In 'policy', bit 0 > is used to enable TDX debug, other bits are reserved currently. > > For example: > > <TrustDomain type='tdx'> > <policy>0x0001</policy> > </TrustDomain> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan(a)intel.com&gt; > --- > docs/schemas/domaincommon.rng | 16 ++++ > src/conf/domain_conf.c | 84 +++++++++++++++++++ > src/conf/domain_conf.h | 15 ++++ > src/conf/virconftypes.h | 2 + > src/qemu/qemu_validate.c | 8 ++ > .../genericxml2xmlindata/trust-domain-tdx.xml | 21 +++++ > tests/genericxml2xmltest.c | 1 + > 7 files changed, 147 insertions(+) > create mode 100644 tests/genericxml2xmlindata/trust-domain-tdx.xml > > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng index 5ea14b6dbf..2b39a01e84 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -89,6 +89,9 @@ > <optional> > <ref name="launchSecurity"/> > </optional> > + <optional> > + <ref name="TrustDomain"/> All our RNG schema entries ... > + </optional> > <optional> > <ref name="bhyvecmdline"/> > </optional> > @@ -518,6 +521,19 @@ > </element> > </define> > > + <define name="TrustDomain"> > + <element name="TrustDomain"> > + <attribute name="type"> > + <value>tdx</value> > + </attribute> > + <interleave> > + <element name="policy"> > + <ref name="hexuint"/> > + </element> > + </interleave> > + </element> > + </define> > + > <!-- > Enable or disable perf events for the domain. For each > of the events the following rules apply: > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index > 139cdfc0a7..a51db088c1 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1403,6 +1403,12 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity, > "sev", > ); > > +VIR_ENUM_IMPL(virDomainTrustDomain, > + VIR_DOMAIN_TRUST_DOMAIN_LAST, > + "", > + "tdx", > +); > + > static virClass *virDomainObjClass; > static virClass *virDomainXMLOptionClass; static void > virDomainObjDispose(void *obj); [...] > @@ -14793,6 +14810,53 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, > return NULL; > } > > +static virDomainTDXDef * > +virDomainTDXDefParseXML(xmlNodePtr tdxNode, > + xmlXPathContextPtr ctxt) { > + VIR_XPATH_NODE_AUTORESTORE(ctxt) > + virDomainTDXDef *def; > + unsigned long policy; > + g_autofree char *type = NULL; > + > + def = g_new0(virDomainTDXDef, 1); > + > + ctxt->node = tdxNode; > + > + if (!(type = virXMLPropString(tdxNode, "type"))) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing trust domain type")); > + goto error; > + } > + > + def->sectype = virDomainTrustDomainTypeFromString(type); > + switch ((virDomainTrustDomain) def->sectype) { > + case VIR_DOMAIN_TRUST_DOMAIN_TDX: > + break; > + case VIR_DOMAIN_TRUST_DOMAIN_NONE: > + case VIR_DOMAIN_TRUST_DOMAIN_LAST: > + default: > + virReportError(VIR_ERR_XML_ERROR, > + _("unsupported trust domain type '%s'"), > + type); > + goto error; > + } > + > + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("failed to get trust domain policy")); > + goto error; > + } > + > + def->policy = policy; > + > + return def; > + > + error: > + virDomainTDXDefFree(def); If you register an autoptr cleanup function for this type you'll be able to avoid the whole 'error' label.

> + return NULL; > +} [...] > @@ -26870,6 +26941,18 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) > virBufferAddLit(buf, "</launchSecurity>\n"); } > > +static void > +virDomainTDXDefFormat(virBuffer *buf, virDomainTDXDef *tdx) { > + if (!tdx) > + return; > + > + virBufferAsprintf(buf, "<TrustDomain type='tdx'>\n"); ... as well as XML element names don't start with capital letter, so this one shouldn't either.

> + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<policy>0x%04x</policy>\n", tdx->policy); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</TrustDomain>\n"); } > > static void [...] > virDomainPerfDefFormat(virBuffer *buf, virDomainPerfDef *perf) diff > --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index > b21068486e..dbd2eb984c 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 _virDomainTDXDef virDomainTDXDef; > + Plase don't mix generic (conf/) changes ...

> typedef struct _virDomainShmemDef virDomainShmemDef; > > typedef struct _virDomainSmartcardDef virDomainSmartcardDef; diff > --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index > 382473d03b..2efd011cc0 100644 > --- a/src/qemu/qemu_validate.c > +++ b/src/qemu/qemu_validate.c > @@ -1222,6 +1222,14 @@ qemuValidateDomainDef(const virDomainDef *def, > return -1; > } > > + if (def->tdx && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("TDX trust domain is not supported with " > + "this QEMU binary")); > + return -1; > + } ... with targetted qemu changes like this. > + > if (def->naudios > 1 && > !virQEMUCapsGet(qemuCaps, QEMU_CAPS_AUDIODEV)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", [...]