Re: [PATCH v14 08/15] qemu: Get SGX capabilities form QMP

On Thu, Jul 28, 2022 at 16:05:08 +0200, Michal Prívozník wrote: > On 7/28/22 10:15, Peter Krempa wrote: > > On Wed, Jul 27, 2022 at 12:34:54 +0200, Michal Privoznik wrote: > >> From: Haibin Huang <haibin.huang(a)intel.com&gt; > >> > >> Generate the QMP command for query-sgx-capabilities and the command > >> return SGX capabilities from QMP. > >> > >> {"execute":"query-sgx-capabilities"} > >> > >> the right reply: > >> {"return": > >> { > >> "sgx": true, > >> "section-size": 197132288, > >> "flc": true > >> } > >> } > >> > >> the error reply: > >> {"error": > >> {"class": "GenericError", "desc": "SGX is not enabled in KVM"} > >> } > >> > >> Signed-off-by: Haibin Huang <haibin.huang(a)intel.com&gt; > >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > >> --- > >> src/qemu/qemu_monitor.c | 10 ++++ > >> src/qemu/qemu_monitor.h | 3 + > >> src/qemu/qemu_monitor_json.c | 107 +++++++++++++++++++++++++++++++++++ > >> src/qemu/qemu_monitor_json.h | 4 ++ > >> 4 files changed, 124 insertions(+) > > > > [...] > > > >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > >> index 941596563a..b045efa203 100644 > >> --- a/src/qemu/qemu_monitor_json.c > >> +++ b/src/qemu/qemu_monitor_json.c > >> @@ -6395,6 +6395,113 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon, > >> return 1; > >> } > >> > >> + > >> +/** > >> + * qemuMonitorJSONGetSGXCapabilities: > >> + * @mon: qemu monitor object > >> + * @capabilities: pointer to pointer to a SGX capability structure to be filled > >> + * > >> + * This function queries and fills in INTEL's SGX platform-specific data. > >> + * Note that from QEMU's POV both -object sgx-epc and query-sgx-capabilities > >> + * can be present even if SGX is not available, which basically leaves us with > >> + * checking for JSON "GenericError" in order to differentiate between compiled-in > >> + * support and actual SGX support on the platform. > >> + * > >> + * Returns: -1 on error, > >> + * 0 if SGX is not supported, and > >> + * 1 if SGX is supported on the platform. > >> + */ > >> +int > >> +qemuMonitorJSONGetSGXCapabilities(qemuMonitor *mon, > >> + virSGXCapability **capabilities) > >> +{ > >> + g_autoptr(virJSONValue) cmd = NULL; > >> + g_autoptr(virJSONValue) reply = NULL; > >> + g_autoptr(virSGXCapability) capability = NULL; > >> + virJSONValue *sections = NULL; > >> + virJSONValue *caps; > >> + bool flc = false; > >> + bool sgx1 = false; > >> + bool sgx2 = false; > > > > These temporary booleans feel a bit redundant ... > > > >> + unsigned long long section_size = 0; > >> + unsigned long long size; > >> + size_t i; > >> + > >> + *capabilities = NULL; > >> + capability = g_new0(virSGXCapability, 1); > >> + > >> + if (!(cmd = qemuMonitorJSONMakeCommand("query-sgx-capabilities", NULL))) > >> + return -1; > >> + > >> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > >> + return -1; > >> + > >> + /* QEMU has only compiled-in support of SGX */ > >> + if (qemuMonitorJSONHasError(reply, "GenericError")) > >> + return 0; > >> + > >> + if (qemuMonitorJSONCheckError(cmd, reply) < 0) > >> + return -1; > >> + > >> + caps = virJSONValueObjectGetObject(reply, "return"); > >> + > >> + if (virJSONValueObjectGetBoolean(caps, "flc", &flc) < 0) { > >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > >> + _("query-sgx-capabilities reply was missing 'flc' field")); > >> + return -1; > >> + } > >> + capability->flc = flc; > > > > Because you assign the value directly back to the struct. Passing the > > pointer to the field in the struct directly to > > virJSONValueObjectGetBoolean avoids the need. > > > >> + > >> + if (virJSONValueObjectGetBoolean(caps, "sgx1", &sgx1) < 0) { > >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > >> + _("query-sgx-capabilities reply was missing 'sgx1' field")); > >> + return -1; > >> + } > >> + capability->sgx1 = sgx1; > >> + > >> + if (virJSONValueObjectGetBoolean(caps, "sgx2", &sgx2) < 0) { > >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > >> + _("query-sgx-capabilities reply was missing 'sgx2' field")); > >> + return -1; > >> + } > >> + capability->sgx2 = sgx2; > >> + > >> + if (virJSONValueObjectGetNumberUlong(caps, "section-size", &section_size) < 0) { > >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > >> + _("query-sgx-capabilities reply was missing 'section-size' field")); > >> + return -1; > >> + } > >> + capability->section_size = section_size / 1024; > > > > The 'section-size' field is marked as deprecated in the QMP schema. Thus > > we must not report error if it vanishes. > > > > Is there any reason to extract it in the first place? > > > > If yes, the code must be fixed to handle the possibility properly. > > The idea is that this allows us to work with qemu-6.2.0 and qemu-7.0.0; > The former reports section-size only, the latter marked it obsolete and > reports array of 'sections' so that sections per NUMA node can be > reported. Now, section-size is nothing but a sum of individual per NUMA > node sections. So I guess we can do the summation once QEMU stops > reporting it. Either way, we must not report an error if it is not present, because we'd specifically be adding code that will break in the future. > NB, presence of per NUMA node sections (this code below) is then used > when generating cmd line, because qemu-7.0.0 requires slightly different > cmd line (due to those NUMA nodes). > > Alternatively, we may pronounce qemu-6.2.0 not worth supporting and aim > on 7.0.0 only and not deal with deprecated interface at all (i.e. don't > parse/report aggregated sum). I'm definitely for skipping 6.2 if possible rather than have code which is going to work for one release only.