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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Nice comments, I accepted all comments.

> -----Original Message-----
> From: Michal Prívozník <mprivozn@xxxxxxxxxx>
> Sent: Wednesday, July 20, 2022 7:27 PM
> To: Yang, Lin A <lin.a.yang@xxxxxxxxx>; libvir-list@xxxxxxxxxx; Huang, Haibin
> <haibin.huang@xxxxxxxxx>; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>
> Subject: Re: [PATCH v13 2/6] Get SGX capabilities form QMP
> 
> On 7/1/22 21:14, Lin Yang 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: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > Signed-off-by: Haibin Huang <haibin.huang@xxxxxxxxx>
> > ---
> >  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.
Ok, I will modify it.
> 
> > +
> > +    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.
[Haibin] OK
> 
> > +        *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.

 [Haibin] OK
> > +
> > +    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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux