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
[Haibin] may be "domain capabilities
structs" should be put in "qemu monitor command", because the
virSGXCapability will be used by qemu monitor command.
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.
[Haibin] Is this advice just for
qemu-7.1 or all qemu version?
This is just for unit test, why not add the faking of SGX into that file. If don't add
faking of SGX into that file, the unit can not pass.
[...]
> @@ -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.
> + virBufferAsprintf(buf, "<epc_size>%u</epc_size>\n",
sgx->epc_size);
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</sgx>\n"); }