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", §ion_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 :|