RE: [libvirt][PATCH v13 2/6] Get SGX capabilities form QMP

-----Original Message----- From: Michal Prívozník <mprivozn(a)redhat.com&gt; Sent: Wednesday, July 20, 2022 7:27 PM To: Yang, Lin A <lin.a.yang(a)intel.com>; libvir-list(a)redhat.com; Huang, Haibin <haibin.huang(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com&gt; Subject: Re: [libvirt][PATCH v13 2/6] Get SGX capabilities form QMP On 7/1/22 21:14, Lin Yang 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: Michal Privoznik <mprivozn(a)redhat.com&gt; > Signed-off-by: Haibin Huang <haibin.huang(a)intel.com&gt; > --- > src/qemu/qemu_monitor.c | 10 ++++ > src/qemu/qemu_monitor.h | 3 + > src/qemu/qemu_monitor_json.c | 113 +++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor_json.h | 4 ++ > 4 files changed, 130 insertions(+) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index > fda5d2f368..a1b2138921 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -3653,6 +3653,16 @@ qemuMonitorGetSEVCapabilities(qemuMonitor *mon, > } > > > +int > +qemuMonitorGetSGXCapabilities(qemuMonitor *mon, > + virSGXCapability **capabilities) { > + QEMU_CHECK_MONITOR(mon); > + > + return qemuMonitorJSONGetSGXCapabilities(mon, capabilities); } > + > + > int > qemuMonitorNBDServerStart(qemuMonitor *mon, > const virStorageNetHostDef *server, diff > --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index > 95267ec6c7..451ac8eec9 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -864,6 +864,9 @@ int qemuMonitorGetGICCapabilities(qemuMonitor > *mon, int qemuMonitorGetSEVCapabilities(qemuMonitor *mon, > virSEVCapability **capabilities); > > +int qemuMonitorGetSGXCapabilities(qemuMonitor *mon, > + virSGXCapability **capabilities); > + > typedef enum { > QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0, > QEMU_MONITOR_MIGRATE_NON_SHARED_DISK = 1 << 1, /* migration with > non-shared storage with full disk copy */ diff --git > a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index > 3aad2ab212..c900956f82 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -6445,6 +6445,119 @@ 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; > + 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; > + > + 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; Spaces around the operand please.

> + > + if (!(sections = virJSONValueObjectGetArray(caps, "sections"))) { > + capability->nSections = 0; > + capability->pSections = NULL; This is needless. g_new0() used above made sure that the memory is zeroed out upon allocation, so these two lines are NOP. > + VIR_INFO("Sections was not obtained, so QEMU version is > + 6.2.0"); VIR_INFO() is probably too harsh. It's not anything that users should be concerned about. VIR_DEBUG() is much better. But then again, when debug logs are enabled then the QMP command and its reply are seen in the log and thus anybody reading it sees immediately whether a part is missing or not.

> + *capabilities = g_steal_pointer(&capability); > + return 1; > + } > + > + // Got the section, the QEMU version is above 7.0.0 > + capability->nSections = virJSONValueArraySize(sections); > + capability->pSections = g_new0(virSection, capability->nSections > + + 1); This + 1 looks wrong to me. We already have nSections, and pSections is not an array of pointers either. So NULL terminated array is a bit overkill.

> + > + for (i = 0; i < capability->nSections; i++) { > + virJSONValue *elem = virJSONValueArrayGet(sections, i); > + > + if (virJSONValueObjectGetNumberUlong(elem, "size", &size) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("query-sgx-capabilities reply was missing 'size' field")); > + return -1; > + } > + capability->pSections[i].size = size/1024; > + > + if (virJSONValueObjectGetNumberUint(elem, "node", &(capability- >pSections[i].node)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("query-sgx-capabilities reply was missing 'node' field")); > + return -1; > + } > + } > + > + *capabilities = g_steal_pointer(&capability); > + return 1; > +} Michal