On Thu, May 12, 2022 at 01:21:45 +0000, Huang, Haibin wrote:
>
> > -----Original Message-----
> > From: Peter Krempa <pkrempa@xxxxxxxxxx>
> > Sent: Thursday, May 12, 2022 12:05 AM
> > To: Yang, Lin A <lin.a.yang@xxxxxxxxx>
> > Cc: libvir-list@xxxxxxxxxx; Huang, Haibin <haibin.huang@xxxxxxxxx>; Ding,
> > Jian-feng <jian-feng.ding@xxxxxxxxx>; Zhong, Yang <yang.zhong@xxxxxxxxx>
> > Subject: Re: [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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > > 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");
> }
Yes. Alternatively you can use a temporary variable and fill that via an
'if' statement.
Finally you can use a virTristateBool variable type to hold the 'flc'
value and use our internal convertors for it.
