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

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

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.

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