RE: [libvirt][PATCH v4 1/4] conf: Introduce SGX related element into domain xml

-----Original Message----- From: Tim Wiederhake <twiederh(a)redhat.com&gt; Sent: Monday, July 5, 2021 7:32 PM To: Huang, Haibin <haibin.huang(a)intel.com&gt; Cc: libvir-list(a)redhat.com; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Yang, Lin A <lin.a.yang(a)intel.com>; Lu, Lianhao <lianhao.lu(a)intel.com&gt; Subject: Re: [libvirt][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(a)intel.com&gt; > >   <launchSecurity type='sgx'> >     <epc_size unit='KiB'>1024</epc_size> >   </launchSecurity> Please also update "docs/schemas/domaincommon.rng".

> --- >  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.

> +    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".

> +        break; > +    case VIR_DOMAIN_LAUNCH_SECURITY_SGX: > +        def->sgx = virDomainSGXDefParseXML(ctxt); Similar.

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; >