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