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

-----Original Message----- From: Daniel P. Berrangé <berrange(a)redhat.com&gt; Sent: Tuesday, September 28, 2021 10:12 PM To: Huang, Haibin <haibin.huang(a)intel.com&gt; Cc: libvir-list(a)redhat.com; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Yang, Lin A <lin.a.yang(a)intel.com>; Lu, Lianhao <lianhao.lu(a)intel.com>; pbonzini(a)redhat.com; pkrempa(a)redhat.com; twiederh(a)redhat.com; phrdina(a)redhat.com; mprivozn(a)redhat.com Subject: Re: [libvirt][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.

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