[PATCH v14 10/15] conf: expose SGX feature in domain capabilities

[Peter Krempa]

On Wed, Jul 27, 2022 at 12:34:56 +0200, Michal Privoznik wrote:
> From: Haibin Huang <haibin.huang at intel.com>
> 
> 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 at intel.com>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---

[...]

> 
> diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst
> index 70f46b972a..bca5389931 100644
> --- a/docs/formatdomaincaps.rst
> +++ b/docs/formatdomaincaps.rst
> @@ -554,6 +554,16 @@ capabilities. All features occur as children of the main ``features`` element.
>           <cbitpos>47</cbitpos>
>           <reduced-phys-bits>1</reduced-phys-bits>
>         </sev>
> +       <sgx supported='yes'>
> +         <flc>no</flc>
> +         <sgx1>yes</sgx1>
> +         <sgx2>no</sgx2>
> +         <section_size>2</section_size>

This element has "unit='KiB'" in the output data in the tests.

> +         <sections>
> +           <section node='0' size='1'/>
> +           <section node='1' size='1'/>

And this one should get a unit too, since the above has it.

> +         </sections>
> +       </sgx>
>       </features>
>     </domainCapabilities>
>  
> @@ -633,3 +643,33 @@ in domain XML <formatdomain.html#launch-security>`__
>  ``maxESGuests``
>     The maximum number of SEV-ES guests that can be launched on the host. This
>     value may be configurable in the firmware for some hosts.
> +
> +SGX capabilities
> +^^^^^^^^^^^^^^^^
> +
> +Intel Software Guard Extensions (Intel SGX) capabilities are exposed under the
> +``sgx`` 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.
> +
> +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 formatdomain.rst
> +Memory devices.

This should be a real RST link to the document/section you are refering
to.

> +
> +``flc``
> +   FLC (Flexible Launch Control), not strictly part of SGX2, but was not part of
> +   original SGX hardware either.
> +
> +``sgx1``
> +   the sgx version 1.
> +
> +``sgx2``
> +   The sgx version 2.
> +
> +``section_size``
> +   The size of the SGX enclave page cache (called EPC).
> +
> +``sections``
> +   The sections of the SGX enclave page cache (called EPC).
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 1d3bba3019..ef41612494 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c

[...]


> @@ -621,6 +622,39 @@ virDomainCapsFeatureSEVFormat(virBuffer *buf,
>      virBufferAddLit(buf, "</sev>\n");
>  }
>  
> +static void
> +virDomainCapsFeatureSGXFormat(virBuffer *buf,
> +                              const virSGXCapability *sgx)
> +{
> +    if (!sgx) {
> +        virBufferAddLit(buf, "<sgx supported='no'/>\n");
> +        return;
> +    }
> +
> +    virBufferAddLit(buf, "<sgx supported='yes'>\n");
> +    virBufferAdjustIndent(buf, 2);
> +    virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
> +    virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", sgx->sgx1 ? "yes" : "no");
> +    virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", sgx->sgx2 ? "yes" : "no");
> +    virBufferAsprintf(buf, "<section_size unit='KiB'>%llu</section_size>\n", sgx->section_size);
> +
> +    if (sgx->nsections > 0) {
> +        size_t i;
> +
> +        virBufferAddLit(buf, "<sections>\n");
> +
> +        for (i = 0; i < sgx->nsections; i++) {
> +            virBufferAdjustIndent(buf, 2);
> +            virBufferAsprintf(buf, "<section node='%d' ", sgx->sections[i].node);
> +            virBufferAsprintf(buf, "size='%llu'/>\n", sgx->sections[i].size);

I think you want to ad a unit field too here.

> +            virBufferAdjustIndent(buf, -2);
> +        }
> +        virBufferAddLit(buf, "</sections>\n");
> +    }
> +
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</sgx>\n");

This looks almost identical to the formatter for the qemu capability
cache. Would it make sense to factor it out and reuse it instead of
reimplementing?

[...]

> diff --git a/tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml
> index 4e6ff06125..5eff1a0ff9 100644
> --- a/tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml
> +++ b/tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml
> @@ -228,5 +228,15 @@
>      <backingStoreInput supported='yes'/>
>      <backup supported='yes'/>
>      <sev supported='no'/>
> +    <sgx supported='yes'>
> +      <flc>no</flc>
> +      <sgx1>yes</sgx1>
> +      <sgx2>no</sgx2>
> +      <section_size unit='KiB'>2</section_size>
> +      <sections>
> +        <section node='0' size='1'/>
> +        <section node='1' size='1'/>

This looks weird without unit.

> +      </sections>
> +    </sgx>
>    </features>
>  </domainCapabilities>