RE: [PATCH v7 4/5] Support to query SGX capability

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

 



Modified the comments and merged the latest community patch

> -----Original Message-----
> From: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> Sent: Tuesday, September 28, 2021 10:12 PM
> To: Huang, Haibin <haibin.huang@xxxxxxxxx>
> Cc: libvir-list@xxxxxxxxxx; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Yang,
> Lin A <lin.a.yang@xxxxxxxxx>; Lu, Lianhao <lianhao.lu@xxxxxxxxx>;
> pbonzini@xxxxxxxxxx; pkrempa@xxxxxxxxxx; twiederh@xxxxxxxxxx;
> phrdina@xxxxxxxxxx; mprivozn@xxxxxxxxxx
> Subject: Re: [PATCH v7 4/5] Support to query SGX capability
> 
> On Wed, Sep 08, 2021 at 09:15:57AM +0800, Haibin Huang wrote:
> >  1.Add SGX feature in domain capabilities  2.Get sgx capabilities by
> > query-sgx-capabilities
> 
> I think we'd generally prefer it if the domain capabilities additions are
> separate from the QEMU Driver capabilities probe.
> 
> So I think I'd suggest you move the probing of QEMU capabilities into a
> separate patch - this probing will also need to update the QEMU capabilities
> tests at the same time - we need bisectability such that
> 
>   git rebase -i master -x 'ninja -C build test'
> 
> will succeeed at each step.
[Haibin] ok, now succussed at each step.
> 
> 
> 
> 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index
> > 090ac80691..7ae4d52e3f 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -218,6 +218,7 @@ virDomainCapsEnumSet;  virDomainCapsFormat;
> > virDomainCapsNew;  virSEVCapabilitiesFree;
> > +virSGXCapabilitiesFree;
> >
> >
> >  # conf/domain_conf.h
> > @@ -1843,7 +1844,6 @@ virBitmapToDataBuf;  virBitmapToString;
> > virBitmapUnion;
> >
> > -
> 
> Stay line removal crept in here by mistake
[Haibin] ok, modified it
> 
> >  # util/virbpf.h
> >  virBPFAttachProg;
> >  virBPFCreateMap;
> > diff --git a/src/qemu/qemu_capabilities.c
> > b/src/qemu/qemu_capabilities.c index f6f4ee28fb..ccc0a4392d 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -637,6 +637,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> >                "confidential-guest-support", /*
> QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT */
> >                "query-display-options", /* QEMU_CAPS_QUERY_DISPLAY_OPTIONS
> */
> >                "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
> > +              "sgx-epc", /* QEMU_CAPS_SGX_EPC */
> >      );
> >
> >
> > @@ -717,11 +718,14 @@ struct _virQEMUCaps {
> >
> >      virSEVCapability *sevCapabilities;
> >
> > +    virSGXCapability *sgxCapabilities;
> > +
> >      /* Capabilities which may differ depending on the accelerator. */
> >      virQEMUCapsAccel kvm;
> >      virQEMUCapsAccel tcg;
> >  };
> >
> > +
> >  struct virQEMUCapsSearchData {
> >      virArch arch;
> >      const char *binaryFilter;
> > @@ -1357,6 +1361,7 @@ struct virQEMUCapsStringFlags
> virQEMUCapsObjectTypes[] = {
> >      { "virtio-gpu-gl-pci", QEMU_CAPS_VIRTIO_GPU_GL_PCI },
> >      { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
> >      { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
> > +    { "sgx-epc", QEMU_CAPS_SGX_EPC },
> >  };
> >
> >
> > @@ -1917,6 +1922,22 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
> > }
> >
> >
> > +static int
> > +virQEMUCapsSGXInfoCopy(virSGXCapabilityPtr *dst,
> > +                       virSGXCapabilityPtr src) {
> > +    g_autoptr(virSGXCapability) tmp = NULL;
> > +
> > +    tmp = g_new0(virSGXCapability, 1);
> > +
> > +    tmp->flc = src->flc;
> > +    tmp->epc_size = src->epc_size;
> > +
> > +    *dst = g_steal_pointer(&tmp);
> > +    return 0;
> > +}
> > +
> > +
> >  static void
> >  virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
> >                                   virQEMUCapsAccel *src) @@ -1996,6
> > +2017,11 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps
> *qemuCaps)
> >                                 qemuCaps->sevCapabilities) < 0)
> >          goto error;
> >
> > +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) &&
> > +        virQEMUCapsSGXInfoCopy(&ret->sgxCapabilities,
> > +                               qemuCaps->sgxCapabilities) < 0)
> > +        goto error;
> > +
> >      return ret;
> >
> >   error:
> > @@ -2036,6 +2062,7 @@ void virQEMUCapsDispose(void *obj)
> >      g_free(qemuCaps->gicCapabilities);
> >
> >      virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> > +    virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
> >
> >      virQEMUCapsAccelClear(&qemuCaps->kvm);
> >      virQEMUCapsAccelClear(&qemuCaps->tcg);
> > @@ -2556,6 +2583,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps
> > *qemuCaps)  }
> >
> >
> > +virSGXCapabilityPtr
> > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps) {
> > +    return qemuCaps->sgxCapabilities; }
> > +
> > +
> >  static int
> >  virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
> >                              qemuMonitor *mon) @@ -2582,6 +2616,7 @@
> > virQEMUCapsProbeQMPObjectTypes(virQEMUCaps *qemuCaps,
> >
> >      if (qemuMonitorGetObjectTypes(mon, &values) < 0)
> >          return -1;
> > +
> 
> Another stray whitespace change
[Haibin] ok, modified it
> 
> >      virQEMUCapsProcessStringFlags(qemuCaps,
> >                                    G_N_ELEMENTS(virQEMUCapsObjectTypes),
> >                                    virQEMUCapsObjectTypes,
> 
> 
> > diff --git a/src/qemu/qemu_monitor_json.c
> > b/src/qemu/qemu_monitor_json.c index 8e5af9f79a..213fc05dc9 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -44,6 +44,7 @@
> >  # include "libvirt_qemu_probes.h"
> >  #endif
> >
> > +#define KB 1024
> 
> This feels rather uncessary to me, and it has way too generic a name in any
> case. Just inline the value since its only used once.
> 
[Haibin] ok, delete it.
> >  #define VIR_FROM_THIS VIR_FROM_QEMU
> >
> >  VIR_LOG_INIT("qemu.qemu_monitor_json");
> > @@ -6862,6 +6863,94 @@
> qemuMonitorJSONGetGICCapabilities(qemuMonitor
> > *mon,  }
> >
> >
> > +/**
> > + * 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) {
> > +    int ret = -1;
> > +    virJSONValue *cmd;
> > +    virJSONValue *reply = NULL;
> > +    virJSONValue *caps;
> > +    bool sgx = false;
> > +    bool flc = false;
> > +    unsigned int section_size = 0;
> > +    g_autoptr(virSGXCapability) capability = NULL;
> > +
> > +    *capabilities = NULL;
> > +
> > +    if (!(cmd = qemuMonitorJSONMakeCommand("query-sgx-capabilities",
> NULL)))
> > +        return -1;
> > +
> > +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> > +        goto cleanup;
> > +
> > +    /* QEMU has only compiled-in support of SGX */
> > +    if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > +        ret = 0;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> > +        goto cleanup;
> > +
> > +    caps = virJSONValueObjectGetObject(reply, "return");
> > +
> > +    if (virJSONValueObjectGetBoolean(caps, "sgx", &sgx) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("query-sgx reply was missing"
> > +                         " 'sgx' field"));
> > +        goto cleanup;
> > +    }
> > +    if (!sgx) {
> > +        VIR_WARN("sgx is not support %d\n", sgx);
> 
> Is it really possible that query-sgx-capabilities will return without the 'sgx' field
> set ?  I would have thought that since you already called
> qemuMonitorJSONCheckError, that 'sgx' should thus always be present at this
> point, and this can be a fatal error report, not a warning ?
> 
> Essentially VIR_WARN is almost never something we wnat todo.
> Either its an expected scenario in which case VIR_DEBUG is the most we should
> do, or its a fatal problem in which case virReportError() and return -1.
[Haibin] ok , modified it.
> 
> > +        ret = 0;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (virJSONValueObjectGetBoolean(caps, "flc", &flc) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("query-sgx-capabilities reply was missing"
> > +                         " 'flc' field"));
> > +        goto cleanup;
> > +    }
> > +
> > +    if (virJSONValueObjectGetNumberUint(caps, "section-size", &section_size)
> < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("query-sgx-capabilities reply was missing"
> > +                         " 'section-size' field"));
> > +        goto cleanup;
> > +    }
> > +
> > +    capability = g_new0(virSGXCapability, 1);
> > +
> > +    capability->flc = flc;
> > +
> > +    capability->epc_size = section_size/(KB);
> > +    *capabilities = g_steal_pointer(&capability);
> > +    ret = 1;
> > +
> > + cleanup:
> > +    virJSONValueFree(cmd);
> > +    virJSONValueFree(reply);
> > +
> > +    return ret;
> > +}
> > +
> > +
> >  /**
> >   * qemuMonitorJSONGetSEVCapabilities:
> >   * @mon: qemu monitor object
> > diff --git a/src/qemu/qemu_monitor_json.h
> > b/src/qemu/qemu_monitor_json.h index fbeab2bf6d..057b62833f 100644
> > --- a/src/qemu/qemu_monitor_json.h
> > +++ b/src/qemu/qemu_monitor_json.h
> > @@ -157,6 +157,9 @@ int
> qemuMonitorJSONGetGICCapabilities(qemuMonitor
> > *mon,  int qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon,
> >                                        virSEVCapability
> > **capabilities);
> >
> > +int qemuMonitorJSONGetSGXCapabilities(qemuMonitor *mon,
> > +                                      virSGXCapability
> > +**capabilities);
> > +
> >  int qemuMonitorJSONMigrate(qemuMonitor *mon,
> >                             unsigned int flags,
> >                             const char *uri);
> > --
> > 2.17.1
> >
> 
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|





[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