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

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

 



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.




> 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

>  # 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

>      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.

>  #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.

> +        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