RE: [libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU

There's too much going on in this patch. You are querying qemu for SGX support and filling domain caps. At least the domain caps should go into the next patch.

There's too much going on in this patch. You are querying qemu for SGX support and filling domain caps. At least the domain caps should go into the next patch.

Secondly, you are using SEV functions as an placeholder. I mean, where you see SEV you put corresponding SGX function. There is nothing wrong with that, but either put pick a placement (after/before SEV code) and stick to it. More comments below.

> > diff --git a/src/conf/domain_capabilities.c > b/src/conf/domain_capabilities.c index 22f0963326..d39be55f6a 100644 > --- a/src/conf/domain_capabilities.c > +++ b/src/conf/domain_capabilities.c > @@ -78,6 +78,16 @@ virSEVCapabilitiesFree(virSEVCapability *cap) } > > > +void > +virSGXCapabilitiesFree(virSGXCapability *cap) { > + if (!cap) > + return; > + > + VIR_FREE(cap); > +} > + > + > static void > virDomainCapsDispose(void *obj) > { > diff --git a/src/conf/domain_capabilities.h > b/src/conf/domain_capabilities.h index d44acdcd01..b647ff8107 100644 > --- a/src/conf/domain_capabilities.h > +++ b/src/conf/domain_capabilities.h > @@ -172,6 +172,13 @@ struct _virDomainCapsCPU { > virDomainCapsCPUModels *custom; > }; > > +typedef struct _virSGXCapability virSGXCapability; typedef > +virSGXCapability *virSGXCapabilityPtr; Even in 7.8.0 which you implemented your patches on top of had virXXXPtr typedefs dropped. Please do not introduce new ones.

> +struct _virSGXCapability { > + bool flc; > + unsigned int epc_size; > +}; > + > typedef struct _virSEVCapability virSEVCapability; struct > _virSEVCapability { > char *pdh; > @@ -215,6 +222,7 @@ struct _virDomainCaps { > > virDomainCapsFeatureGIC gic; > virSEVCapability *sev; > + virSGXCapability *sgx; > /* add new domain features here */ > > virTristateBool features[VIR_DOMAIN_CAPS_FEATURE_LAST]; > @@ -262,4 +270,9 @@ char * virDomainCapsFormat(const virDomainCaps > *caps); void virSEVCapabilitiesFree(virSEVCapability *capabilities); > > +void > +virSGXCapabilitiesFree(virSGXCapability *capabilities); > + > G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSEVCapability, > virSEVCapabilitiesFree); > + > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSGXCapability, > +virSGXCapabilitiesFree); > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index > c5d788285e..d90d4ee6e1 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -219,6 +219,7 @@ virDomainCapsEnumSet; virDomainCapsFormat; > virDomainCapsNew; virSEVCapabilitiesFree; > +virSGXCapabilitiesFree; > > > # conf/domain_conf.h > diff --git a/src/qemu/qemu_capabilities.c > b/src/qemu/qemu_capabilities.c index a607f5ea5f..8ce184ce35 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -651,6 +651,7 @@ VIR_ENUM_IMPL(virQEMUCaps, > "chardev.json", /* QEMU_CAPS_CHARDEV_JSON */ > "device.json", /* QEMU_CAPS_DEVICE_JSON */ > "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */ > + "sgx-epc", /* QEMU_CAPS_SGX_EPC */ > ); > > > @@ -731,11 +732,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; > @@ -1367,6 +1371,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { > { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL }, > { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST }, > { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI }, > + { "sgx-epc", QEMU_CAPS_SGX_EPC }, > }; > > > @@ -1918,6 +1923,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; I know you followed the example of virQEMUCapsSEVInfoCopy() but both functions should be void. They never return anything else than 0.

> +} > + > + > static void > virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst, > virQEMUCapsAccel *src) @@ -1997,6 > +2018,11 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps) > qemuCaps->sevCapabilities) < 0) > return NULL; > > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) && > + virQEMUCapsSGXInfoCopy(&ret->sgxCapabilities, > + qemuCaps->sgxCapabilities) < 0) > + return NULL; > + > return g_steal_pointer(&ret); > } > > @@ -2033,6 +2059,7 @@ void virQEMUCapsDispose(void *obj) > g_free(qemuCaps->gicCapabilities); > > virSEVCapabilitiesFree(qemuCaps->sevCapabilities); > + virSGXCapabilitiesFree(qemuCaps->sgxCapabilities); > > virQEMUCapsAccelClear(&qemuCaps->kvm); > virQEMUCapsAccelClear(&qemuCaps->tcg); > @@ -2553,6 +2580,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps > *qemuCaps) } > > > +virSGXCapabilityPtr > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps) { > + return qemuCaps->sgxCapabilities; } > + > + > static int > virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps, > qemuMonitor *mon) @@ -3327,6 +3361,31 @@ > virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps *qemuCaps, } > > > +static int > +virQEMUCapsProbeQMPSGXCapabilities(virQEMUCaps *qemuCaps, > + qemuMonitor *mon) { > + int rc = -1; > + virSGXCapability *caps = NULL; > + > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC)) > + return 0; > + > + if ((rc = qemuMonitorGetSGXCapabilities(mon, &caps)) < 0) > + return -1; > + > + /* SGX isn't actually supported */ > + if (rc == 0) { > + virQEMUCapsClear(qemuCaps, QEMU_CAPS_SGX_EPC); > + return 0; > + } > + > + virSGXCapabilitiesFree(qemuCaps->sgxCapabilities); > + qemuCaps->sgxCapabilities = caps; > + return 0; > +} > + > + > /* > * Filter for features which should never be passed to QEMU. Either because > * QEMU never supported them or they were dropped as they never did > anything @@ -4110,6 +4169,41 @@ virQEMUCapsParseSEVInfo(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt) > return 0; > } > > +static int > +virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps, xmlXPathContextPtr > +ctxt) Here and everywhere else, please one argument per line.

> +{ > + g_autoptr(virSGXCapability) sgx = NULL; > + > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC)) > + return 0; > + > + if (virXPathBoolean("boolean(./sgx)", ctxt) == 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing SGX platform data in QEMU " > + "capabilities cache")); I know you wanted to fit under 80 characters, but error messages are extempt from the rule. In fact, we want error messages to be on one line because it's easier to git-grep them. > + return -1; > + } > + > + sgx = g_new0(virSGXCapability, 1); > + > + if (virXPathBoolean("boolean(./sgx/flc)", ctxt) == 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing SGX platform flc data in QEMU " > + "capabilities cache")); > + return -1; > + } > + > + if (virXPathUInt("string(./sgx/epc_size)", ctxt, &sgx->epc_size) < 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing or malformed SGX platform epc_size information " > + "in QEMU capabilities cache")); > + return -1; > + } > + > + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx); > + return 0; > +} > + > > /* > * Parsing a doc that looks like > @@ -4226,7 +4320,7 @@ virQEMUCapsLoadCache(virArch hostArch, > flag = virQEMUCapsTypeFromString(str); > if (flag < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Unknown qemu capabilities flag %s"), str); > + _("Haibin Unknown qemu capabilities flag > + %s"), str); Huh? Probably some leftover from debugging?

> goto cleanup; > } > VIR_FREE(str); > @@ -4351,6 +4445,9 @@ virQEMUCapsLoadCache(virArch hostArch, > if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0) > goto cleanup; > > + if (virQEMUCapsParseSGXInfo(qemuCaps, ctxt) < 0) > + goto cleanup; > + > virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM); > virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, > VIR_DOMAIN_VIRT_QEMU); > > @@ -4531,6 +4628,19 @@ virQEMUCapsFormatSEVInfo(virQEMUCaps *qemuCaps, virBuffer *buf) > virBufferAddLit(buf, "</sev>\n"); } > > +static void > +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps, virBuffer *buf) { > + virSGXCapabilityPtr sgx = > +virQEMUCapsGetSGXCapabilities(qemuCaps); > + > + virBufferAddLit(buf, "<sgx>\n"); > + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no"); > + virBufferAsprintf(buf, "<epc_size>%u</epc_size>\n", sgx->epc_size); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</sgx>\n"); } > + > > char * > virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) @@ -4605,6 +4715,9 @@ > virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) > if (qemuCaps->sevCapabilities) > virQEMUCapsFormatSEVInfo(qemuCaps, &buf); > > + if (qemuCaps->sgxCapabilities) > + virQEMUCapsFormatSGXInfo(qemuCaps, &buf); > + > if (qemuCaps->kvmSupportsNesting) > virBufferAddLit(&buf, "<kvmSupportsNesting/>\n"); > > @@ -5276,6 +5389,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps, > return -1; > if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) > return -1; > + if (virQEMUCapsProbeQMPSGXCapabilities(qemuCaps, mon) < 0) > + return -1; > > virQEMUCapsInitProcessCaps(qemuCaps); > > @@ -6248,6 +6363,31 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCaps > *qemuCaps, } > > > +/** > + * virQEMUCapsFillDomainFeatureiSGXCaps: > + * @qemuCaps: QEMU capabilities > + * @domCaps: domain capabilities > + * > + * Take the information about SGX capabilities that has been obtained > + * using the 'query-sgx-capabilities' QMP command and stored in > +@qemuCaps > + * and convert it to a form suitable for @domCaps. > + */ > +static void > +virQEMUCapsFillDomainFeatureSGXCaps(virQEMUCaps *qemuCaps, > + virDomainCaps *domCaps) { > + virSGXCapability *cap = qemuCaps->sgxCapabilities; > + > + if (!cap) > + return; > + > + domCaps->sgx = g_new0(virSGXCapability, 1); > + > + domCaps->sgx->flc = cap->flc; > + domCaps->sgx->epc_size = cap->epc_size; } > + > + > /** > * virQEMUCapsFillDomainFeatureSEVCaps: > * @qemuCaps: QEMU capabilities > @@ -6339,6 +6479,7 @@ virQEMUCapsFillDomainCaps(virQEMUCaps *qemuCaps, > virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps); > virQEMUCapsFillDomainFeatureSEVCaps(qemuCaps, domCaps); > virQEMUCapsFillDomainFeatureS390PVCaps(qemuCaps, domCaps); > + virQEMUCapsFillDomainFeatureSGXCaps(qemuCaps, domCaps); > > return 0; > } > diff --git a/src/qemu/qemu_capabilities.h > b/src/qemu/qemu_capabilities.h index bb53d9ae46..e8c052b27d 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -631,6 +631,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > QEMU_CAPS_CHARDEV_JSON, /* -chardev accepts JSON */ > QEMU_CAPS_DEVICE_JSON, /* -device accepts JSON */ > QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */ > + QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */ > > QEMU_CAPS_LAST /* this must always be the last item */ } > virQEMUCapsFlags; @@ -824,5 +825,8 @@ > virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps); bool > virQEMUCapsGetKVMSupportsSecureGuest(virQEMUCaps *qemuCaps) > G_GNUC_NO_INLINE; > > +virSGXCapabilityPtr > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps); > + > virArch virQEMUCapsArchFromString(const char *arch); const char > *virQEMUCapsArchToString(virArch arch); diff --git > a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index > e8accaf2b0..dca3e94ed2 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -3787,6 +3787,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 > 52ff34d316..e08fc1bfe3 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -915,6 +915,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 > 579d986e02..b2c5042a22 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -6723,6 +6723,89 @@ 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; Here, both cmd and reply should be declared with g_autoptr(), like this: g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; That way they don't have to be freed explicitly and whole cleanup label can be dropped.

> + 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) { This boolean is never used. What's its purpose?

> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("query-sgx reply was missing" > + " 'sgx' field")); > + 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/1024; > + *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 c841de0a03..bfb405ed04 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); diff --git > a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies > b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies > index aa7a779a68..1092eb7c31 100644 > --- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies > +++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies > @@ -29479,6 +29479,20 @@ > } > } > > +{ > + "execute": "query-sgx-capabilities", > + "id": "libvirt-49" > +} > + > +{ > + "id": "libvirt-49", > + "return": { > + "sgx": true, > + "section-size": 1024, > + "flc": false > + } I don't have SGX enabled machine, but I believe that 'id' is the last item in the reply. Also, according to QEMU's documentation (qapi/misc-target.json) returns more fields. Have you actually generated this reply using QEMU or was it handcrafted? Nothing wrong with that, but we should get as close to actual reply as possible.

> +} > + Michal