<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> <div> On 7/28/22, 7:21 AM, "Peter Krempa" <pkrempa@redhat.com> wrote:<o:p></o:p> </div> >On Thu, Jul 28, 2022 at 16:05:08 +0200, Michal Prívozník wrote:<o:p></o:p> >> On 7/28/22 10:15, Peter Krempa wrote:<o:p></o:p> >> > On Wed, Jul 27, 2022 at 12:34:54 +0200, Michal Privoznik wrote:<o:p></o:p> >> >> From: Haibin Huang <haibin.huang@intel.com><o:p></o:p> >> >><o:p></o:p> >> >> Generate the QMP command for query-sgx-capabilities and the command<o:p></o:p> >> >> return SGX capabilities from QMP.<o:p></o:p> >> >><o:p></o:p> >> >> {"execute":"query-sgx-capabilities"}<o:p></o:p> >> >><o:p></o:p> >> >> the right reply:<o:p></o:p> >> >> {"return":<o:p></o:p> >> >> {<o:p></o:p> >> >> "sgx": true,<o:p></o:p> >> >> "section-size": 197132288,<o:p></o:p> >> >> "flc": true<o:p></o:p> >> >> }<o:p></o:p> >> >> }<o:p></o:p> >> >><o:p></o:p> >> >> the error reply:<o:p></o:p> >> >> {"error":<o:p></o:p> >> >> {"class": "GenericError", "desc": "SGX is not enabled in KVM"}<o:p></o:p> >> >> }<o:p></o:p> >> >><o:p></o:p> >> >> Signed-off-by: Haibin Huang <haibin.huang@intel.com><o:p></o:p> >> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com><o:p></o:p> >> >> ---<o:p></o:p> >> >> src/qemu/qemu_monitor.c | 10 ++++<o:p></o:p> >> >> src/qemu/qemu_monitor.h | 3 +<o:p></o:p> >> >> src/qemu/qemu_monitor_json.c | 107 +++++++++++++++++++++++++++++++++++<o:p></o:p> >> >> src/qemu/qemu_monitor_json.h | 4 ++<o:p></o:p> >> >> 4 files changed, 124 insertions(+)<o:p></o:p> >> > <o:p></o:p> >> > [...]<o:p></o:p> >> > <o:p></o:p> >> >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c<o:p></o:p> >> >> index 941596563a..b045efa203 100644<o:p></o:p> >> >> --- a/src/qemu/qemu_monitor_json.c<o:p></o:p> >> >> +++ b/src/qemu/qemu_monitor_json.c<o:p></o:p> >> >> @@ -6395,6 +6395,113 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon,<o:p></o:p> >> >> return 1;<o:p></o:p> >> >> }<o:p></o:p> >> >> <o:p></o:p> >> >> +<o:p></o:p> >> >> +/**<o:p></o:p> >> >> + * qemuMonitorJSONGetSGXCapabilities:<o:p></o:p> >> >> + * @mon: qemu monitor object<o:p></o:p> >> >> + * @capabilities: pointer to pointer to a SGX capability structure to be filled<o:p></o:p> >> >> + *<o:p></o:p> >> >> + * This function queries and fills in INTEL's SGX platform-specific data.<o:p></o:p> >> >> + * Note that from QEMU's POV both -object sgx-epc and query-sgx-capabilities<o:p></o:p> >> >> + * can be present even if SGX is not available, which basically leaves us with<o:p></o:p> >> >> + * checking for JSON "GenericError" in order to differentiate between compiled-in<o:p></o:p> >> >> + * support and actual SGX support on the platform.<o:p></o:p> >> >> + *<o:p></o:p> >> >> + * Returns: -1 on error,<o:p></o:p> >> >> + * 0 if SGX is not supported, and<o:p></o:p> >> >> + * 1 if SGX is supported on the platform.<o:p></o:p> >> >> + */<o:p></o:p> >> >> +int<o:p></o:p> >> >> +qemuMonitorJSONGetSGXCapabilities(qemuMonitor *mon,<o:p></o:p> >> >> + virSGXCapability **capabilities)<o:p></o:p> >> >> +{<o:p></o:p> >> >> + g_autoptr(virJSONValue) cmd = NULL;<o:p></o:p> >> >> + g_autoptr(virJSONValue) reply = NULL;<o:p></o:p> >> >> + g_autoptr(virSGXCapability) capability = NULL;<o:p></o:p> >> >> + virJSONValue *sections = NULL;<o:p></o:p> >> >> + virJSONValue *caps;<o:p></o:p> >> >> + bool flc = false;<o:p></o:p> >> >> + bool sgx1 = false;<o:p></o:p> >> >> + bool sgx2 = false;<o:p></o:p> >> > <o:p></o:p> >> > These temporary booleans feel a bit redundant ...<o:p></o:p> >> > <o:p></o:p> >> >> + unsigned long long section_size = 0;<o:p></o:p> >> >> + unsigned long long size;<o:p></o:p> >> >> + size_t i;<o:p></o:p> >> >> +<o:p></o:p> >> >> + *capabilities = NULL;<o:p></o:p> >> >> + capability = g_new0(virSGXCapability, 1);<o:p></o:p> >> >> +<o:p></o:p> >> >> + if (!(cmd = qemuMonitorJSONMakeCommand("query-sgx-capabilities", NULL)))<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> +<o:p></o:p> >> >> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> +<o:p></o:p> >> >> + /* QEMU has only compiled-in support of SGX */<o:p></o:p> >> >> + if (qemuMonitorJSONHasError(reply, "GenericError"))<o:p></o:p> >> >> + return 0;<o:p></o:p> >> >> +<o:p></o:p> >> >> + if (qemuMonitorJSONCheckError(cmd, reply) < 0)<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> +<o:p></o:p> >> >> + caps = virJSONValueObjectGetObject(reply, "return");<o:p></o:p> >> >> +<o:p></o:p> >> >> + if (virJSONValueObjectGetBoolean(caps, "flc", &flc) < 0) {<o:p></o:p> >> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",<o:p></o:p> >> >> + _("query-sgx-capabilities reply was missing 'flc' field"));<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> + }<o:p></o:p> >> >> + capability->flc = flc;<o:p></o:p> >> > <o:p></o:p> >> > Because you assign the value directly back to the struct. Passing the<o:p></o:p> >> > pointer to the field in the struct directly to<o:p></o:p> >> > virJSONValueObjectGetBoolean avoids the need.<o:p></o:p> >> > <o:p></o:p> >> >> +<o:p></o:p> >> >> + if (virJSONValueObjectGetBoolean(caps, "sgx1", &sgx1) < 0) {<o:p></o:p> >> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",<o:p></o:p> >> >> + _("query-sgx-capabilities reply was missing 'sgx1' field"));<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> + }<o:p></o:p> >> >> + capability->sgx1 = sgx1;<o:p></o:p> >> >> +<o:p></o:p> >> >> + if (virJSONValueObjectGetBoolean(caps, "sgx2", &sgx2) < 0) {<o:p></o:p> >> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",<o:p></o:p> >> >> + _("query-sgx-capabilities reply was missing 'sgx2' field"));<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> + }<o:p></o:p> >> >> + capability->sgx2 = sgx2;<o:p></o:p> >> >> +<o:p></o:p> >> >> + if (virJSONValueObjectGetNumberUlong(caps, "section-size", &section_size) < 0) {<o:p></o:p> >> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",<o:p></o:p> >> >> + _("query-sgx-capabilities reply was missing 'section-size' field"));<o:p></o:p> >> >> + return -1;<o:p></o:p> >> >> + }<o:p></o:p> >> >> + capability->section_size = section_size / 1024;<o:p></o:p> >> > <o:p></o:p> >> > The 'section-size' field is marked as deprecated in the QMP schema. Thus<o:p></o:p> >> > we must not report error if it vanishes.<o:p></o:p> >> > <o:p></o:p> >> > Is there any reason to extract it in the first place?<o:p></o:p> >> > <o:p></o:p> >> > If yes, the code must be fixed to handle the possibility properly.<o:p></o:p> >> <o:p></o:p> >> The idea is that this allows us to work with qemu-6.2.0 and qemu-7.0.0;<o:p></o:p> >> The former reports section-size only, the latter marked it obsolete and<o:p></o:p> >> reports array of 'sections' so that sections per NUMA node can be<o:p></o:p> >> reported. Now, section-size is nothing but a sum of individual per NUMA<o:p></o:p> >> node sections. So I guess we can do the summation once QEMU stops<o:p></o:p> >> reporting it.<o:p></o:p> ><o:p> </o:p> >Either way, we must not report an error if it is not present, because<o:p></o:p> >we'd specifically be adding code that will break in the future.<o:p></o:p> ><o:p> </o:p> >> NB, presence of per NUMA node sections (this code below) is then used<o:p></o:p> >> when generating cmd line, because qemu-7.0.0 requires slightly different<o:p></o:p> >> cmd line (due to those NUMA nodes).<o:p></o:p> >> <o:p></o:p> >> Alternatively, we may pronounce qemu-6.2.0 not worth supporting and aim<o:p></o:p> >> on 7.0.0 only and not deal with deprecated interface at all (i.e. don't<o:p></o:p> >> parse/report aggregated sum).<o:p></o:p> ><o:p> </o:p> >I'm definitely for skipping 6.2 if possible rather than have code which<o:p></o:p> >is going to work for one release only.<o:p></o:p> <o:p> </o:p> I see, let me drop 6.2 support and only aim on 7.0 in v15 patch.<o:p></o:p> <o:p> </o:p> @Michal, do you have any updated for v14 patches? If yes, I can rework on<o:p></o:p> top of your changes and submit for review.<o:p></o:p> <a href="https://gitlab.com/MichalPrivoznik/libvirt/-/commits/sgx_rework">https://gitlab.com/MichalPrivoznik/libvirt/-/commits/sgx_rework</a> ?<o:p></o:p> Or any way you preferred for this collaboration in this case?<o:p></o:p> <o:p> </o:p> Thanks,<o:p></o:p> Lin.<o:p></o:p> <o:p> </o:p> <o:p> </o:p> <o:p> </o:p> <o:p> </o:p> <o:p> </o:p> </div> </body> </html>