9:36 p.m.
Ok, nice comments
Thank you very much!
-----Original Message-----
From: Michal Prívozník <mprivozn(a)redhat.com>
Sent: Wednesday, July 20, 2022 7:27 PM
To: Yang, Lin A <lin.a.yang(a)intel.com>; libvir-list(a)redhat.com; Huang, Haibin
<haibin.huang(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>
Subject: Re: [libvirt][PATCH v13 4/6] conf: expose SGX feature in domain
capabilities
On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang <haibin.huang(a)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: Michal Privoznik <mprivozn(a)redhat.com>
> Signed-off-by: Haibin Huang <haibin.huang(a)intel.com>
> ---
> docs/formatdomaincaps.rst | 40 ++++++++++++++++
> src/conf/domain_capabilities.c | 48 +++++++++++++++++++
> src/conf/schemas/domaincaps.rng | 42 ++++++++++++++++
> src/qemu/qemu_capabilities.c | 28 +++++++++++
> tests/domaincapsdata/bhyve_basic.x86_64.xml | 1 +
> tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 1 +
> tests/domaincapsdata/bhyve_uefi.x86_64.xml | 1 +
> tests/domaincapsdata/empty.xml | 1 +
> tests/domaincapsdata/libxl-xenfv.xml | 1 +
> tests/domaincapsdata/libxl-xenpv.xml | 1 +
> .../domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 1 +
> tests/domaincapsdata/qemu_2.11.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 1 +
> .../qemu_2.12.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_2.12.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_2.12.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_3.0.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_3.0.0-tcg.x86_64.xml | 1 +
> tests/domaincapsdata/qemu_3.0.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_3.0.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_3.0.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_3.1.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_3.1.0-tcg.x86_64.xml | 1 +
> tests/domaincapsdata/qemu_3.1.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_3.1.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_4.0.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_4.0.0-tcg.x86_64.xml | 1 +
> .../qemu_4.0.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_4.0.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_4.0.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_4.0.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_4.0.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_4.1.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_4.1.0-tcg.x86_64.xml | 1 +
> tests/domaincapsdata/qemu_4.1.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 1 +
> .../qemu_4.2.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_4.2.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_4.2.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_4.2.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_4.2.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 1 +
> .../qemu_5.0.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_5.0.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_5.0.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_5.0.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_5.1.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml | 1 +
> tests/domaincapsdata/qemu_5.1.0.sparc.xml | 1 +
> tests/domaincapsdata/qemu_5.1.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_5.2.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml | 1 +
> .../qemu_5.2.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_5.2.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_5.2.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 1 +
> .../qemu_6.0.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_6.0.0.s390x.xml | 1 +
> tests/domaincapsdata/qemu_6.0.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_6.1.0-q35.x86_64.xml | 1 +
> .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml | 1 +
> tests/domaincapsdata/qemu_6.1.0.x86_64.xml | 1 +
> .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml | 6 +++
> .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml | 6 +++
> .../qemu_6.2.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_6.2.0.x86_64.xml | 6 +++
> .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 10 ++++
> .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 10 ++++
> .../qemu_7.0.0-virt.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_7.0.0.aarch64.xml | 1 +
> tests/domaincapsdata/qemu_7.0.0.ppc64.xml | 1 +
> tests/domaincapsdata/qemu_7.0.0.x86_64.xml | 10 ++++
> .../domaincapsdata/qemu_7.1.0-q35.x86_64.xml | 10 ++++
> .../domaincapsdata/qemu_7.1.0-tcg.x86_64.xml | 10 ++++
> tests/domaincapsdata/qemu_7.1.0.x86_64.xml | 10 ++++
> 88 files changed, 311 insertions(+)
>
> diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst
> index 933469b2a2..d3bdee28d4 100644
> --- a/docs/formatdomaincaps.rst
> +++ b/docs/formatdomaincaps.rst
> @@ -519,6 +519,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>
> + <sections>
> + <section node='0' size='1'/>
> + <section node='1' size='1'/>
> + </sections>
> + </sgx>
> </features>
> </domainCapabilities>
>
> @@ -598,3 +608,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.
> +
> +``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 27f3fb8d36..fa29f69807 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -98,6 +98,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++) @@ -620,6 +621,52 @@
> virDomainCapsFeatureSEVFormat(virBuffer *buf,
> return;
> }
>
> +static void
> +virDomainCapsFeatureSGXFormat(virBuffer *buf,
> + const virSGXCapability *sgx) {
> + size_t i;
> +
> + if (!sgx) {
> + virBufferAddLit(buf, "<sgx supported='no'/>\n");
> + } else {
> + virBufferAddLit(buf, "<sgx supported='yes'>\n");
> + virBufferAdjustIndent(buf, 2);
> + if (sgx->flc) {
> + virBufferAsprintf(buf, "<flc>%s</flc>\n",
"yes");
> + } else {
> + virBufferAsprintf(buf, "<flc>%s</flc>\n",
"no");
> + }
Same comment here as in the previous patch.
> + if (sgx->sgx1) {
> + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n",
"yes");
> + } else {
> + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n",
"no");
> + }
> + if (sgx->sgx2) {
> + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n",
"yes");
> + } else {
> + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n",
"no");
> + }
> + virBufferAsprintf(buf, "<section_size
> + unit='KiB'>%llu</section_size>\n", sgx->section_size);
> +
> + if (sgx->nSections > 0) {
> + virBufferAddLit(buf, "<sections>\n");
> +
> + for (i = 0; i < sgx->nSections; i++) {
> + virBufferAdjustIndent(buf, 2);
> + virBufferAsprintf(buf, "<section node='%u' ",
sgx-
>pSections[i].node);
> + virBufferAsprintf(buf, "size='%llu'/>\n",
sgx->pSections[i].size);
> + virBufferAdjustIndent(buf, -2);
> + }
> + virBufferAddLit(buf, "</sections>\n");
> + }
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</sgx>\n");
> + }
> +
> + return;
> +}
>
> static void
> virDomainCapsFormatFeatures(const virDomainCaps *caps, @@ -640,6
> +687,7 @@ virDomainCapsFormatFeatures(const virDomainCaps *caps,
> }
>
> virDomainCapsFeatureSEVFormat(&childBuf, caps->sev);
> + virDomainCapsFeatureSGXFormat(&childBuf, caps->sgx);
>
> virXMLFormatElement(buf, "features", NULL, &childBuf); } diff
> --git a/src/conf/schemas/domaincaps.rng
> b/src/conf/schemas/domaincaps.rng index 9cbc2467ab..6975ebacb9
100644
> --- a/src/conf/schemas/domaincaps.rng
> +++ b/src/conf/schemas/domaincaps.rng
> @@ -270,6 +270,9 @@
> <optional>
> <ref name="sev"/>
> </optional>
> + <optional>
> + <ref name="sgx"/>
> + </optional>
> </element>
> </define>
>
> @@ -330,6 +333,45 @@
> </element>
> </define>
>
> + <define name="sgx">
> + <element name="sgx">
> + <ref name="supported"/>
> + <optional>
> + <element name="flc">
> + <data type="string"/>
Or, since we know we are formatting yes/no, we can use virYesNo instead of
too generic string type.
> + </element>
> + <element name="sgx1">
> + <data type="string"/>
> + </element>
> + <element name="sgx2">
> + <data type="string"/>
> + </element>
> + <element name="section_size">
> + <attribute name="unit">
> + <value>KiB</value>
> + </attribute>
> + <data type="unsignedInt"/>
> + </element>
> + <optional>
> + <element name="sections">
> + <interleave>
I think the order is well defined and thus we don't really need <interleave/>
here. The domain capabilities XML is not provided by user, it's generated by
libvirt, in virDomainCapsFormat() where the order is well defined.
> + <zeroOrMore>
> + <element name="section">
> + <attribute name="node">
> + <data type="string"/>
This is an integer ..
> + </attribute>
> + <attribute name="size">
> + <data type="string"/>
.. and so is this.
> + </attribute>
> + </element>
> + </zeroOrMore>
> + </interleave>
> + </element>
> + </optional>
> + </optional>
> + </element>
> + </define>
> +
> <define name="value">
> <zeroOrMore>
> <element name="value">
> diff --git a/src/qemu/qemu_capabilities.c
> b/src/qemu/qemu_capabilities.c index 57b5acb150..598c694738 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -6744,6 +6744,33 @@
virQEMUCapsFillDomainFeatureS390PVCaps(virQEMUCaps *qemuCaps,
> }
> }
>
> +/**
> + * virQEMUCapsFillDomainFeatureiSGXCaps:
> + * @qemuCaps: QEMU capabilities
> + * @domCaps: domain capabilities
> + *
> + * Take the information about SGX capabilities that has been obtained
> + * using the 'query-sgx-capabilities' QMP command and stored in
> +@qemuCaps
> + * and convert it to a form suitable for @domCaps.
> + */
> +static void
> +virQEMUCapsFillDomainFeatureSGXCaps(virQEMUCaps *qemuCaps,
> + virDomainCaps *domCaps) {
> + virSGXCapability *cap = qemuCaps->sgxCapabilities;
> +
> + if (!cap)
> + return;
> +
> + domCaps->sgx = g_new0(virSGXCapability, 1);
> +
> + domCaps->sgx->flc = cap->flc;
> + domCaps->sgx->sgx1 = cap->sgx1;
> + domCaps->sgx->sgx2 = cap->sgx2;
> + domCaps->sgx->section_size = cap->section_size;
> + domCaps->sgx->nSections = cap->nSections;
> + domCaps->sgx->pSections = cap->pSections;
This is a verbatim copy of virQEMUCapsSGXInfoCopy(), any reason for not
just call the function?
> +}
Michal