RE: [libvirt][PATCH v11 1/4] qemu: provide support to query the SGX capability
-----Original Message-----
From: Peter Krempa <pkrempa(a)redhat.com>
Sent: Thursday, May 12, 2022 12:05 AM
To: Yang, Lin A <lin.a.yang(a)intel.com>
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>
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>
>
> 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>
> ---
> 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"); }