[libvirt][PATCH v11 1/4] qemu: provide support to query the SGX capability

[Huang, Haibin]

> -----Original Message-----
> From: Peter Krempa <pkrempa at redhat.com>
> Sent: Thursday, May 12, 2022 12:05 AM
> To: Yang, Lin A <lin.a.yang at intel.com>
> Cc: libvir-list at redhat.com; Huang, Haibin <haibin.huang at intel.com>; Ding,
> Jian-feng <jian-feng.ding at intel.com>; Zhong, Yang <yang.zhong at intel.com>
> Subject: Re: [libvirt][PATCH v11 1/4] qemu: provide support to query the SGX
> capability
> 
> On Tue, May 10, 2022 at 23:11:09 -0700, Lin Yang wrote:
> > From: Haibin Huang <haibin.huang at intel.com>
> >
> > QEMU version >= 6.2.0 provides support for creating enclave on SGX x86
> > platform using Software Guard Extensions (SGX) feature.
> > This patch adds support to query the SGX capability from the qemu.
> >
> > Signed-off-by: Haibin Huang <haibin.huang at intel.com>
> > ---
> >  src/conf/domain_capabilities.c                |  10 ++
> >  src/conf/domain_capabilities.h                |  13 ++
> >  src/libvirt_private.syms                      |   1 +
> >  src/qemu/qemu_capabilities.c                  | 119 ++++++++++++++++++
> >  src/qemu/qemu_capabilities.h                  |   6 +
> >  src/qemu/qemu_capspriv.h                      |   4 +
> >  src/qemu/qemu_monitor.c                       |  10 ++
> >  src/qemu/qemu_monitor.h                       |   3 +
> >  src/qemu/qemu_monitor_json.c                  | 104 +++++++++++++--
> >  src/qemu/qemu_monitor_json.h                  |   9 ++
> >  .../caps_6.2.0.x86_64.replies                 |  22 +++-
> >  .../caps_6.2.0.x86_64.xml                     |   5 +
> >  .../caps_7.0.0.x86_64.replies                 |  22 +++-
> >  .../caps_7.0.0.x86_64.xml                     |   5 +
> >  14 files changed, 318 insertions(+), 15 deletions(-)
> 
> This is not a full review. Couple of points:
> 
> 1) Do not mix other changes with adding QEMU_CAPS* stuff
>     Basically theres waaay too much going on in this patch and it
>     definitely can be separated into smaller chunks. The QEMU_CAPS is
>     just one of them.
>     Separate at least:
>         - qemu monitor command introduction
>         - domain capabilities data structs for sgx
>         - parsing and formatting of the XML
>         - adding of the QEMU_CAPS_ flag
> 2) caps for qemu-7.1 were added very recently
>     You'll need to fix that one too since you added an extra query. Make
>     sure that you _don't_ add the faking of SXG into that file, but
>     rather the error case. My box doesn't support SGX so it will be
>     overwritten in my next refresh anyways.
> 
> [...]
> 
> > @@ -4706,6 +4805,21 @@ virQEMUCapsFormatSEVInfo(virQEMUCaps
> *qemuCaps,
> > virBuffer *buf)  }
> >
> >
> > +static void
> > +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps,
> > +                         virBuffer *buf) {
> > +    virSGXCapabilityPtr sgx =
> > +virQEMUCapsGetSGXCapabilities(qemuCaps);
> > +
> > +    virBufferAddLit(buf, "<sgx>\n");
> > +    virBufferAdjustIndent(buf, 2);
> > +    virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" :
> > + "no");
> 
> Don't use the ternary operator ('?'), use a full if/else branch instead or pick a
> better data structure.
> 
[Haibin]   do you mean change to like below?
   if (sgx->flc) {
        virBufferAsprintf(buf, "<flc>%s</flc>\n", "yes");
    } else {
        virBufferAsprintf(buf, "<flc>%s</flc>\n", "no");
    }
> > +    virBufferAsprintf(buf, "<epc_size>%u</epc_size>\n", sgx->epc_size);
> > +    virBufferAdjustIndent(buf, -2);
> > +    virBufferAddLit(buf, "</sgx>\n"); }