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

[Peter Krempa]

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 at intel.com>
> >>
> >> 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 at intel.com>
> >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >> ---
> >>  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.