|
On 7/28/22, 7:21 AM, "Peter Krempa" <pkrempa@xxxxxxxxxx> wrote: >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@xxxxxxxxx> >> >> >> >> 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@xxxxxxxxx> >> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> >> --- >> >> 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", §ion_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. I see, let me drop 6.2 support and only aim on 7.0 in v15 patch. @Michal, do you have any updated for v14 patches? If yes, I can rework on top of your changes and submit for review. https://gitlab.com/MichalPrivoznik/libvirt/-/commits/sgx_rework ? Or any way you preferred for this collaboration in this case? Thanks, Lin. |
