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

> -----Original Message----- > From: Peter Krempa <pkrempa(a)redhat.com&gt; > Sent: Thursday, May 12, 2022 12:05 AM > To: Yang, Lin A <lin.a.yang(a)intel.com&gt; > Cc: libvir-list(a)redhat.com; Huang, Haibin <haibin.huang(a)intel.com>; Ding, > Jian-feng <jian-feng.ding(a)intel.com>; Zhong, Yang <yang.zhong(a)intel.com&gt; > 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(a)intel.com&gt; > > > > 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(a)intel.com&gt; > > --- > > 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"); }