Ok, nice comments
Thank you very much!
> -----Original Message-----
> From: Michal Prívozník <mprivozn@xxxxxxxxxx>
> Sent: Wednesday, July 20, 2022 7:27 PM
> To: Yang, Lin A <lin.a.yang@xxxxxxxxx>; libvir-list@xxxxxxxxxx; Huang, Haibin
> <haibin.huang@xxxxxxxxx>; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>
> Subject: Re: [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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Haibin Huang <haibin.huang@xxxxxxxxx>
> > ---
> > 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
