> -----Original Message-----
> From: Tim Wiederhake <twiederh@xxxxxxxxxx>
> Sent: Monday, July 5, 2021 7:32 PM
> To: Huang, Haibin <haibin.huang@xxxxxxxxx>
> Cc: libvir-list@xxxxxxxxxx; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Yang,
> Lin A <lin.a.yang@xxxxxxxxx>; Lu, Lianhao <lianhao.lu@xxxxxxxxx>
> Subject: Re: [PATCH v4 1/4] conf: Introduce SGX related element into
> domain xml
>
> On Thu, 2021-07-01 at 20:10 +0800, Haibin Huang wrote:
> > From: Lin Yang <lin.a.yang@xxxxxxxxx>
> >
> > <launchSecurity type='sgx'>
> > <epc_size unit='KiB'>1024</epc_size>
> > </launchSecurity>
>
> Please also update "docs/schemas/domaincommon.rng".
[Haibin] ok, I will do it.
>
> > ---
> > src/conf/domain_conf.c | 106 +++++++++++++++++++++++++++++-------
> --
> > --
> > src/conf/domain_conf.h | 10 ++++
> > src/conf/virconftypes.h | 3 ++
> > 3 files changed, 91 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index
> > ef67efa1da..4336dafd82 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -1336,6 +1336,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity,
> > VIR_DOMAIN_LAUNCH_SECURITY_LAST,
> > "",
> > "sev",
> > + "sgx",
> > );
> >
> > static virClassPtr virDomainObjClass; @@ -3409,6 +3410,16 @@
> > virDomainSEVDefFree(virDomainSEVDefPtr def)
> > }
> >
> >
> > +static void
> > +virDomainSGXDefFree(virDomainSGXDefPtr def) {
> > + if (!def)
> > + return;
> > +
> > + VIR_FREE(def);
> > +}
> > +
> > +
> > void virDomainDefFree(virDomainDefPtr def)
> > {
> > size_t i;
> > @@ -3597,6 +3608,7 @@ void virDomainDefFree(virDomainDefPtr def)
> > (def->ns.free)(def->namespaceData);
> >
> > virDomainSEVDefFree(def->sev);
> > + virDomainSGXDefFree(def->sgx);
> >
> > xmlFreeNode(def->metadata);
> >
> > @@ -16700,39 +16712,17 @@
> virDomainMemoryTargetDefParseXML(xmlNodePtr
> > node,
> > return 0;
> > }
> >
> > -
> > static virDomainSEVDefPtr
> > -virDomainSEVDefParseXML(xmlNodePtr sevNode,
> > - xmlXPathContextPtr ctxt)
> > +virDomainSEVDefParseXML(xmlXPathContextPtr ctxt)
> > {
> > VIR_XPATH_NODE_AUTORESTORE(ctxt);
> > virDomainSEVDefPtr def;
> > unsigned long policy;
> > - g_autofree char *type = NULL;
> >
> > if (VIR_ALLOC(def) < 0)
> > return NULL;
> >
> > - 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;
> > - }
> > + def->sectype = VIR_DOMAIN_LAUNCH_SECURITY_SEV;
> >
> > if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) < 0) {
> > virReportError(VIR_ERR_XML_ERROR, "%s", @@ -16764,6 +16754,63
> > @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
> > return NULL;
> > }
> >
> > +static virDomainSGXDefPtr
> > +virDomainSGXDefParseXML(xmlXPathContextPtr ctxt) {
> > + VIR_XPATH_NODE_AUTORESTORE(ctxt);
>
> I do not believe that this is necessary.
[Haibin] ok, I will remove " VIR_XPATH_NODE_AUTORESTORE(ctxt);"
>
> > + virDomainSGXDefPtr def;
> > +
> > + if (VIR_ALLOC(def) < 0)
> > + return NULL;
> > +
> > + def->sectype = VIR_DOMAIN_LAUNCH_SECURITY_SGX;
> > +
> > + if (virDomainParseMemory("./epc_size", "./epc_size/@unit", ctxt,
> > + &def->epc_size, false, false) < 0)
> > + goto error;
> > +
> > + return def;
> > +
> > + error:
> > + virDomainSGXDefFree(def);
> > + return NULL;
> > +}
> > +
> > +static int
> > +virDomainLaunchSecurityDefParseXML(xmlNodePtr launchSecurityNode,
> > + xmlXPathContextPtr ctxt,
> > + virDomainDefPtr def) {
> > + VIR_XPATH_NODE_AUTORESTORE(ctxt);
> > + g_autofree char *type = NULL;
> > +
> > + ctxt->node = launchSecurityNode;
> > +
> > + if (!(type = virXMLPropString(launchSecurityNode, "type"))) {
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("missing launch security type"));
> > + return -1;
> > + }
> > +
> > + switch ((virDomainLaunchSecurity)
> > virDomainLaunchSecurityTypeFromString(type)) {
> > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
> > + def->sev = virDomainSEVDefParseXML(ctxt);
>
> I believe this should return "-1" when "def->sev == NULL".
[Haibin] ok, I will add check code for def->sev.
>
> > + break;
> > + case VIR_DOMAIN_LAUNCH_SECURITY_SGX:
> > + def->sgx = virDomainSGXDefParseXML(ctxt);
>
> Similar.
[Haibin] ok, I will add check code for def->sgx.
>
> Regards,
> Tim
> > + 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 -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static virDomainMemoryDefPtr
> > virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
> > xmlNodePtr memdevNode, @@ -22227,12
> > +22274,15 @@ virDomainDefParseXML(xmlDocPtr xml,
> > ctxt->node = node;
> > VIR_FREE(nodes);
> >
> > - /* Check for SEV feature */
> > - if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) {
> > - def->sev = virDomainSEVDefParseXML(node, ctxt);
> > - if (!def->sev)
> > + /* analysis of launch security */
> > + if ((n = virXPathNodeSet("./launchSecurity", ctxt, &nodes)) < 0)
> > + goto error;
> > +
> > + for (i = 0; i < n; i++) {
> > + if (virDomainLaunchSecurityDefParseXML(nodes[i], ctxt, def)
> > != 0)
> > goto error;
> > }
> > + VIR_FREE(nodes);
> >
> > /* analysis of memory devices */
> > if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0)
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index
> > 011bf66cb4..88adf461df 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -2447,6 +2447,7 @@ struct _virDomainKeyWrapDef {
> > typedef enum {
> > VIR_DOMAIN_LAUNCH_SECURITY_NONE,
> > VIR_DOMAIN_LAUNCH_SECURITY_SEV,
> > + VIR_DOMAIN_LAUNCH_SECURITY_SGX,
> >
> > VIR_DOMAIN_LAUNCH_SECURITY_LAST,
> > } virDomainLaunchSecurity;
> > @@ -2462,6 +2463,12 @@ struct _virDomainSEVDef {
> > };
> >
> >
> > +struct _virDomainSGXDef {
> > + int sectype; /* enum virDomainLaunchSecurity */
> > + unsigned long long epc_size; /* kibibytes */ };
> > +
> > +
> > typedef enum {
> > VIR_DOMAIN_IOMMU_MODEL_INTEL,
> > VIR_DOMAIN_IOMMU_MODEL_SMMUV3,
> > @@ -2670,6 +2677,9 @@ struct _virDomainDef {
> > /* SEV-specific domain */
> > virDomainSEVDefPtr sev;
> >
> > + /* SGX-specific domain */
> > + virDomainSGXDefPtr sgx;
> > +
> > /* Application-specific custom metadata */
> > xmlNodePtr metadata;
> >
> > diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index
> > 1c62cde251..084bcc7687 100644
> > --- a/src/conf/virconftypes.h
> > +++ b/src/conf/virconftypes.h
> > @@ -291,6 +291,9 @@ typedef virDomainResourceDef
> > *virDomainResourceDefPtr;
> > typedef struct _virDomainSEVDef virDomainSEVDef;
> > typedef virDomainSEVDef *virDomainSEVDefPtr;
> >
> > +typedef struct _virDomainSGXDef virDomainSGXDef; typedef
> > +virDomainSGXDef *virDomainSGXDefPtr;
> > +
> > typedef struct _virDomainShmemDef virDomainShmemDef;
> > typedef virDomainShmemDef *virDomainShmemDefPtr;
> >
