RE: [PATCH RESEND v10 2/5] conf: expose SGX feature in domain capabilities

["Huang, Haibin" <haibin.huang@xxxxxxxxx>]

Ok, I will merge 2/5 and 3/5, Thank you!

> -----Original Message-----
> From: Michal Prívozník <mprivozn@xxxxxxxxxx>
> Sent: Wednesday, February 16, 2022 6:25 PM
> To: Huang, Haibin <haibin.huang@xxxxxxxxx>; libvir-list@xxxxxxxxxx;
> berrange@xxxxxxxxxx; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Yang, Lin A
> <lin.a.yang@xxxxxxxxx>; Lu, Lianhao <lianhao.lu@xxxxxxxxx>
> Subject: Re: [PATCH RESEND v10 2/5] conf: expose SGX feature in
> domain capabilities
> 
> On 2/8/22 06:21, Haibin Huang wrote:
> > Extend hypervisor capabilities to include sgx feature. When available,
> > the hypervisor supports launching an VM with SGX on Intel platfrom.
> > The SGX feature tag privides additional details like section size and
> > sgx1 or sgx2.
> >
> > Signed-off-by: Haibin Huang <haibin.huang@xxxxxxxxx>
> > ---
> >  docs/formatdomaincaps.html.in  | 26 ++++++++++++++++++++++++++
> >  docs/schemas/domaincaps.rng    | 22 +++++++++++++++++++++-
> >  src/conf/domain_capabilities.c | 21 +++++++++++++++++++++
> >  src/qemu/qemu_capabilities.c   | 24 ++++++++++++++++++++++++
> >  4 files changed, 92 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/formatdomaincaps.html.in
> > b/docs/formatdomaincaps.html.in index 35b8bf3def..d932e6df80 100644
> > --- a/docs/formatdomaincaps.html.in
> > +++ b/docs/formatdomaincaps.html.in
> > @@ -598,6 +598,10 @@
> >        &lt;cbitpos&gt;47&lt;/cbitpos&gt;
> >        &lt;reduced-phys-bits&gt;1&lt;/reduced-phys-bits&gt;
> >      &lt;/sev&gt;
> > +    &lt;sgx&gt;
> > +      &lt;flc&gt;no&lt;/flc&gt;
> > +      &lt;epc_size&gt;1&lt;/epc_size&gt;
> > +    &lt;/sgx&gt;
> >    &lt;/features&gt;
> >  &lt;/domainCapabilities&gt;
> >  </pre>
> > @@ -689,5 +693,27 @@
> >        This value may be configurable in the firmware for some hosts.</dd>
> >      </dl>
> >
> > +    <h4><a id="elementsSGX">SGX capabilities</a></h4>
> > +
> > +    <p>Intel Software Guard Extensions (Intel SGX) capabilities are exposed
> under
> > +    the <code>sgx</code> element.
> > +    Intel SGX helps protect data in use via unique application isolation
> technology.
> > +    Protect selected code and data from modification using hardened enclaves
> with
> > +    Intel SGX.</p>
> > +
> > +    <p>
> > +      For more details on the SGX feature, please follow resources in the
> > +      SGX developer's document store. In order to use SGX with libvirt have
> > +      a look at <a href="formatdomain.html#memory-devices">SGX in domain
> XML</a>
> > +    </p>
> > +
> > +    <dl>
> > +      <dt><code>flc</code></dt>
> > +      <dd>FLC (Flexible Launch Control), not strictly part of SGX2, but was not
> part
> > +      of original SGX hardware either.</dd>
> > +      <dt><code>epc_size</code></dt>
> > +      <dd>The size of the SGX enclave page cache (called EPC).</dd>
> > +    </dl>
> > +
> >    </body>
> >  </html>
> > diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
> > index 9cbc2467ab..5ace30ae0d 100644
> > --- a/docs/schemas/domaincaps.rng
> > +++ b/docs/schemas/domaincaps.rng
> > @@ -270,6 +270,9 @@
> >        <optional>
> >          <ref name="sev"/>
> >        </optional>
> > +      <optional>
> > +        <ref name='sgx'/>
> > +      </optional>
> >      </element>
> >    </define>
> >
> > @@ -330,7 +333,24 @@
> >      </element>
> >    </define>
> >
> > -  <define name="value">
> > +  <define name='sgx'>
> > +    <element name='sgx'>
> > +      <ref name='supported'/>
> > +      <optional>
> > +        <element name='flc'>
> > +          <data type='string'/>
> > +        </element>
> > +        <element name='epc_size'>
> > +          <attribute name="unit">
> > +            <value>KiB</value>
> > +          </attribute>
> > +          <data type='unsignedInt'/>
> > +        </element>
> > +      </optional>
> > +    </element>
> > +  </define>
> > +
> > +  <define name='value'>
> >      <zeroOrMore>
> >        <element name="value">
> >          <text/>
> > diff --git a/src/conf/domain_capabilities.c
> > b/src/conf/domain_capabilities.c index 1170fd26df..2e9f0ec225 100644
> > --- a/src/conf/domain_capabilities.c
> > +++ b/src/conf/domain_capabilities.c
> > @@ -100,6 +100,7 @@ virDomainCapsDispose(void *obj)
> >      virObjectUnref(caps->cpu.custom);
> >      virCPUDefFree(caps->cpu.hostModel);
> >      virSEVCapabilitiesFree(caps->sev);
> > +    virSGXCapabilitiesFree(caps->sgx);
> >
> >      values = &caps->os.loader.values;
> >      for (i = 0; i < values->nvalues; i++) @@ -618,6 +619,25 @@
> > virDomainCapsFeatureSEVFormat(virBuffer *buf,
> >      return;
> >  }
> >
> > +static void
> > +virDomainCapsFeatureSGXFormat(virBuffer *buf,
> > +                              const virSGXCapability *sgx) {
> > +    if (!sgx) {
> > +        return; // will delete in test patch
> 
> No. The whole point of separating a feature into smaller patches is so that
> individual patches can be backported (plus it's easier for review).
> But this creates a dependency on the next commit, which removes this return.
> 
> IOW, the next patch should be squashed in.
> 
[Haibin] So, You mean merge this patch with the patch that follows, right?
> > +        virBufferAddLit(buf, "<sgx supported='no'/>\n");
> > +    } else {
> > +        return; // will delete in test patch
> > +        virBufferAddLit(buf, "<sgx supported='yes'>\n");
> > +        virBufferAdjustIndent(buf, 2);
> > +        virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
> > +        virBufferAsprintf(buf, "<epc_size unit='KiB'>%d</epc_size>\n", sgx-
> >epc_size);
> > +        virBufferAdjustIndent(buf, -2);
> > +        virBufferAddLit(buf, "</sgx>\n");
> > +    }
> > +
> > +    return;
> > +}
> >
> 
> Michal