The Qemu QMP provide the command "query-sgx-capabilities"
libvirt call the command to get sgx capabilities
{"execute":"query-sgx-capabilities"}
{"return":
{"sgx": true, "sgx1": true, "sgx2": false, "section-size": 0, \
"flc": false}}
Signed-off-by: Haibin Huang <haibin.huang@intel.com>
---
src/conf/domain_capabilities.c | 10 ++
src/conf/domain_capabilities.h | 13 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_capabilities.c | 143 +++++++++++++++++-
src/qemu/qemu_capabilities.h | 4 +
src/qemu/qemu_monitor.c | 10 ++
src/qemu/qemu_monitor.h | 3 +
src/qemu/qemu_monitor_json.c | 83 ++++++++++
src/qemu/qemu_monitor_json.h | 3 +
.../caps_6.2.0.x86_64.replies | 22 ++-
.../caps_6.2.0.x86_64.xml | 5 +
11 files changed, 292 insertions(+), 5 deletions(-)
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;
+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;
+}
+
+
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)
+{
+ 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"));
+ 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);
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;
+ 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 (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/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
+ }
+}
+
{
"execute": "query-cpu-model-expansion",
"arguments": {
@@ -29487,7 +29501,7 @@
"name": "host"
}
},
- "id": "libvirt-49"
+ "id": "libvirt-50"
}
{
@@ -29820,7 +29834,7 @@
}
}
},
- "id": "libvirt-49"
+ "id": "libvirt-50"
}
{
@@ -29834,7 +29848,7 @@
}
}
},
- "id": "libvirt-50"
+ "id": "libvirt-51"
}
{
@@ -30167,7 +30181,7 @@
}
}
},
- "id": "libvirt-50"
+ "id": "libvirt-51"
}
{
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index 5a46da0a6a..410964d84d 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -241,6 +241,7 @@
<flag name='ich9.acpi-hotplug-bridge'/>
<flag name='device.json'/>
<flag name='query-dirty-rate'/>
+ <flag name='sgx-epc'/>
<version>6001050</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100244</microcodeVersion>
@@ -3711,4 +3712,8 @@
<machine type='tcg' name='pc-q35-2.5' hotplugCpus='yes' maxCpus='255' defaultCPU='qemu64-x86_64-cpu' numaMemSupported='yes' defaultRAMid='pc.ram'/>
<machine type='tcg' name='pc-i440fx-3.0' hotplugCpus='yes' maxCpus='255' defaultCPU='qemu64-x86_64-cpu' numaMemSupported='yes' defaultRAMid='pc.ram'/>
<machine type='tcg' name='pc-q35-2.11' hotplugCpus='yes' maxCpus='288' defaultCPU='qemu64-x86_64-cpu' numaMemSupported='yes' defaultRAMid='pc.ram'/>
+ <sgx>
+ <flc>no</flc>
+ <epc_size>1</epc_size>
+ </sgx>
</qemuCaps>
--
2.17.1
On 12/15/21 04:40, Haibin Huang wrote:
> The Qemu QMP provide the command "query-sgx-capabilities"
> libvirt call the command to get sgx capabilities
>
> {"execute":"query-sgx-capabilities"}
> {"return":
> {"sgx": true, "sgx1": true, "sgx2": false, "section-size": 0, \
> "flc": false}}
>
> Signed-off-by: Haibin Huang <haibin.huang@intel.com>
> ---
> src/conf/domain_capabilities.c | 10 ++
> src/conf/domain_capabilities.h | 13 ++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_capabilities.c | 143 +++++++++++++++++-
> src/qemu/qemu_capabilities.h | 4 +
> src/qemu/qemu_monitor.c | 10 ++
> src/qemu/qemu_monitor.h | 3 +
> src/qemu/qemu_monitor_json.c | 83 ++++++++++
> src/qemu/qemu_monitor_json.h | 3 +
> .../caps_6.2.0.x86_64.replies | 22 ++-
> .../caps_6.2.0.x86_64.xml | 5 +
> 11 files changed, 292 insertions(+), 5 deletions(-)
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", §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/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
Hi Michael,
Thank you for your comments.
> 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.
Ok , we can put domain_capabilities.c in to the next patch, but virSGXCapability is define domain_capabilities.h, qemu_monitor_json.c will use it.
If we also put it to next patch, then this patch will not work.
So, I think we can just put domain_capabilities.c in to the next patch, ok?
> -----Original Message-----
> From: Michal Prívozník <mprivozn@redhat.com>
> Sent: Friday, January 7, 2022 11:06 PM
> To: Huang, Haibin <haibin.huang@intel.com>; libvir-list@redhat.com; Ding,
> Jian-feng <jian-feng.ding@intel.com>; Yang, Lin A <lin.a.yang@intel.com>; Lu,
> Lianhao <lianhao.lu@intel.com>; Zhong, Yang <yang.zhong@intel.com>
> Subject: Re: [libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU
>
> On 12/15/21 04:40, Haibin Huang wrote:
> > The Qemu QMP provide the command "query-sgx-capabilities"
> > libvirt call the command to get sgx capabilities
> >
> > {"execute":"query-sgx-capabilities"}
> > {"return":
> > {"sgx": true, "sgx1": true, "sgx2": false, "section-size": 0, \
> > "flc": false}}
> >
> > Signed-off-by: Haibin Huang <haibin.huang@intel.com>
> > ---
> > src/conf/domain_capabilities.c | 10 ++
> > src/conf/domain_capabilities.h | 13 ++
> > src/libvirt_private.syms | 1 +
> > src/qemu/qemu_capabilities.c | 143 +++++++++++++++++-
> > src/qemu/qemu_capabilities.h | 4 +
> > src/qemu/qemu_monitor.c | 10 ++
> > src/qemu/qemu_monitor.h | 3 +
> > src/qemu/qemu_monitor_json.c | 83 ++++++++++
> > src/qemu/qemu_monitor_json.h | 3 +
> > .../caps_6.2.0.x86_64.replies | 22 ++-
> > .../caps_6.2.0.x86_64.xml | 5 +
> > 11 files changed, 292 insertions(+), 5 deletions(-)
>
>
> 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.
Ok , we can put domain_capabilities.c in to the next patch, but virSGXCapability is define domain_capabilities.h, qemu_monitor_json.c will use it.
If we also put it to next patch, then this patch will not work.
>
> 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.
Ok , I have change it
> >
> > 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.
Ok, I have changed it.
>
> > +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.
Ok, change it.
>
> > +}
> > +
> > +
> > 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.
Ok, changed it
>
> > +{
> > + 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?
Sorry, miss it, change it.
>
> > 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.
>
Ok, I have changed it.
>
> > + 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?
Ok, for if exist sgx, I have deleted it because if epc size is not zero mean sgx exist.
>
> > + 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",
> §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/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.
Yes, this is actually reply, I will adjust the id to end.
>
> > +}
> > +
>
> Michal
On 1/20/22 02:59, Huang, Haibin wrote:
> Hi Michael,
>
> Thank you for your comments.
>
>> 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.
>
> Ok , we can put domain_capabilities.c in to the next patch, but virSGXCapability is define domain_capabilities.h, qemu_monitor_json.c will use it.
> If we also put it to next patch, then this patch will not work.
>
> So, I think we can just put domain_capabilities.c in to the next patch, ok?
Yes, that's one of the options. Firstly, modify
src/qemu/qemu_capabilities.c so that the capability is detected, and
only after that implement domain_capabilities so that the capability is
reported.
Alternatively, you can introduce virSGXCapability machinery in one patch
and then QEMU detection in another.
>
>> -----Original Message-----
>> From: Michal Prívozník <mprivozn@redhat.com>
>> Sent: Friday, January 7, 2022 11:06 PM
>> To: Huang, Haibin <haibin.huang@intel.com>; libvir-list@redhat.com; Ding,
>> Jian-feng <jian-feng.ding@intel.com>; Yang, Lin A <lin.a.yang@intel.com>; Lu,
>> Lianhao <lianhao.lu@intel.com>; Zhong, Yang <yang.zhong@intel.com>
>> Subject: Re: [libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU
>>
>> On 12/15/21 04:40, Haibin Huang wrote:
>>> The Qemu QMP provide the command "query-sgx-capabilities"
>>> libvirt call the command to get sgx capabilities
>>>
>>> {"execute":"query-sgx-capabilities"}
>>> {"return":
>>> {"sgx": true, "sgx1": true, "sgx2": false, "section-size": 0, \
>>> "flc": false}}
>>>
>>> Signed-off-by: Haibin Huang <haibin.huang@intel.com>
>>> ---
>>> src/conf/domain_capabilities.c | 10 ++
>>> src/conf/domain_capabilities.h | 13 ++
>>> src/libvirt_private.syms | 1 +
>>> src/qemu/qemu_capabilities.c | 143 +++++++++++++++++-
>>> src/qemu/qemu_capabilities.h | 4 +
>>> src/qemu/qemu_monitor.c | 10 ++
>>> src/qemu/qemu_monitor.h | 3 +
>>> src/qemu/qemu_monitor_json.c | 83 ++++++++++
>>> src/qemu/qemu_monitor_json.h | 3 +
>>> .../caps_6.2.0.x86_64.replies | 22 ++-
>>> .../caps_6.2.0.x86_64.xml | 5 +
>>> 11 files changed, 292 insertions(+), 5 deletions(-)
>>
>>
>> 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.
>
> Ok , we can put domain_capabilities.c in to the next patch, but virSGXCapability is define domain_capabilities.h, qemu_monitor_json.c will use it.
> If we also put it to next patch, then this patch will not work.
The rule is to break huge change into small semantic chunks. It doesn't
mean that only one file/dir can be changed. If you need to declare a
struct just do it. But detecting SGX capability from QEMU and reporting
it in domain capabilities are two semantically disticnt things, thus
should be in two separate patches.
Michal
© 2016 - 2026 Red Hat, Inc.