RE: [PATCH v13 4/6] conf: expose SGX feature in domain capabilities

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux