From: Haibin Huang <haibin.huang@intel.com>
the QMP capabilities:
{"return":
{
"sgx": true,
"section-size": 1024,
"flc": true
}
}
the domain capabilities:
<sgx>
<flc>yes</flc>
<epc_size>1</epc_size>
</sgx>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Haibin Huang <haibin.huang@intel.com>
---
src/qemu/qemu_capabilities.c | 230 ++++++++++++++++++
src/qemu/qemu_capabilities.h | 4 +
.../caps_6.2.0.x86_64.replies | 30 ++-
.../caps_6.2.0.x86_64.xml | 7 +
.../caps_7.0.0.x86_64.replies | 34 ++-
.../caps_7.0.0.x86_64.xml | 11 +
.../caps_7.1.0.x86_64.replies | 34 ++-
.../caps_7.1.0.x86_64.xml | 11 +
8 files changed, 346 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2c3be3ecec..57b5acb150 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
"display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
"iothread.thread-pool-max", /* QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
+ "sgx-epc", /* QEMU_CAPS_SGX_EPC */
);
@@ -752,6 +753,8 @@ struct _virQEMUCaps {
virSEVCapability *sevCapabilities;
+ virSGXCapability *sgxCapabilities;
+
/* Capabilities which may differ depending on the accelerator. */
virQEMUCapsAccel kvm;
virQEMUCapsAccel hvf;
@@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
{ "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
{ "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
+ { "sgx-epc", QEMU_CAPS_SGX_EPC },
};
@@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
}
+static int
+virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
+ virSGXCapability *src)
+{
+ g_autoptr(virSGXCapability) tmp = NULL;
+
+ tmp = g_new0(virSGXCapability, 1);
+
+ tmp->flc = src->flc;
+ tmp->sgx1 = src->sgx1;
+ tmp->sgx2 = src->sgx2;
+ tmp->section_size = src->section_size;
+
+ if (src->nSections == 0) {
+ tmp->nSections = 0;
+ tmp->pSections = NULL;
+ } else {
+ tmp->nSections = src->nSections;
+ tmp->pSections = src->pSections;
+ }
+
+ *dst = g_steal_pointer(&tmp);
+ return 0;
+}
+
+
static void
virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
virQEMUCapsAccel *src)
@@ -2053,6 +2083,12 @@ 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);
}
@@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
virCPUDataFree(qemuCaps->cpuData);
virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
+ virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
virQEMUCapsAccelClear(&qemuCaps->kvm);
virQEMUCapsAccelClear(&qemuCaps->hvf);
@@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps)
}
+virSGXCapabilityPtr
+virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps)
+{
+ return qemuCaps->sgxCapabilities;
+}
+
+
static int
virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
qemuMonitor *mon)
@@ -3442,6 +3486,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
@@ -4220,6 +4289,116 @@ virQEMUCapsParseSEVInfo(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt)
}
+static int
+virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps,
+ xmlXPathContextPtr ctxt)
+{
+ g_autoptr(virSGXCapability) sgx = NULL;
+ xmlNodePtr node;
+
+ g_autofree xmlNodePtr *nodes = NULL;
+ g_autofree xmlNodePtr *sectionNodes = NULL;
+ g_autofree char *flc = NULL;
+ g_autofree char *sgx1 = NULL;
+ g_autofree char *sgx2 = NULL;
+
+ int n = 0;
+ int nsections = 0;
+
+ 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 ((!(flc = virXPathString("string(./sgx/flc)", ctxt))) ||
+ virStringParseYesNo(flc, &sgx->flc) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing or invalid SGX platform flc in QEMU capabilities cache"));
+ return -1;
+ }
+
+ if ((!(sgx1 = virXPathString("string(./sgx/sgx1)", ctxt))) ||
+ virStringParseYesNo(sgx1, &sgx->sgx1) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing or invalid SGX platform sgx1 in QEMU capabilities cache"));
+ return -1;
+ }
+
+ if ((!(sgx2 = virXPathString("string(./sgx/sgx2)", ctxt))) ||
+ virStringParseYesNo(sgx2, &sgx->sgx2) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing or invalid SGX platform sgx2 in QEMU capabilities cache"));
+ return -1;
+ }
+
+ if (virXPathULongLong("string(./sgx/section_size)", ctxt, &sgx->section_size) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing or malformed SGX platform section_size in QEMU capabilities cache"));
+ return -1;
+ }
+
+ if ((n = virXPathNodeSet("./sgx/sections", ctxt, &nodes)) < 0) {
+ sgx->nSections = 0;
+ sgx->pSections = NULL;
+ VIR_INFO("Sections was not obtained, so QEMU version is 6.2.0");
+ qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
+ return 0;
+ }
+
+ if (n == 0) {
+ qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
+ return 0;
+ }
+
+ // Got the section, the QEMU version is above 7.0.0
+ node = ctxt->node;
+ ctxt->node = nodes[0];
+ nsections = virXPathNodeSet("./section", ctxt, §ionNodes);
+ ctxt->node = node;
+
+ if (nsections < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to parse CPU blockers in QEMU capabilities"));
+ return -1;
+ }
+
+ if (nsections > 0) {
+ size_t i;
+ g_autofree char * strNode = NULL;
+ g_autofree char * strSize = NULL;
+ sgx->nSections = nsections;
+ sgx->pSections = g_new0(virSection, nsections + 1);
+
+ for (i = 0; i < nsections; i++) {
+ if ((strNode = virXMLPropString(sectionNodes[i], "node")) &&
+ (virStrToLong_ui(strNode, NULL, 10, &(sgx->pSections[i].node)) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing node name in QEMU "
+ "capabilities cache"));
+ return -1;
+ }
+
+ if ((strSize = virXMLPropString(sectionNodes[i], "size")) &&
+ (virStrToLong_ull(strSize, NULL, 10, &(sgx->pSections[i].size)) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing size name in QEMU "
+ "capabilities cache"));
+ return -1;
+ }
+ }
+ }
+
+ qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
+ return 0;
+}
+
+
static int
virQEMUCapsParseFlags(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt)
{
@@ -4522,6 +4701,9 @@ virQEMUCapsLoadCache(virArch hostArch,
if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
return -1;
+ if (virQEMUCapsParseSGXInfo(qemuCaps, ctxt) < 0)
+ return -1;
+
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
@@ -4707,6 +4889,49 @@ virQEMUCapsFormatSEVInfo(virQEMUCaps *qemuCaps, virBuffer *buf)
}
+static void
+virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps,
+ virBuffer *buf)
+{
+ virSGXCapabilityPtr sgx = virQEMUCapsGetSGXCapabilities(qemuCaps);
+ size_t i;
+
+ virBufferAddLit(buf, "<sgx supported='yes'>\n");
+ virBufferAdjustIndent(buf, 2);
+ if (sgx->flc) {
+ virBufferAsprintf(buf, "<flc>%s</flc>\n", "yes");
+ } else {
+ virBufferAsprintf(buf, "<flc>%s</flc>\n", "no");
+ }
+ if (sgx->sgx1) {
+ virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "yes");
+ } else {
+ virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "no");
+ }
+ if (sgx->sgx2) {
+ virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "yes");
+ } else {
+ virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "no");
+ }
+ virBufferAsprintf(buf, "<section_size unit='KiB'>%llu</section_size>\n", sgx->section_size);
+
+ if (sgx->nSections > 0) {
+ virBufferAddLit(buf, "<sections>\n");
+
+ for (i = 0; i < sgx->nSections; i++) {
+ virBufferAdjustIndent(buf, 2);
+ virBufferAsprintf(buf, "<section node='%u' ", sgx->pSections[i].node);
+ virBufferAsprintf(buf, "size='%llu'/>\n", sgx->pSections[i].size);
+ virBufferAdjustIndent(buf, -2);
+ }
+ virBufferAddLit(buf, "</sections>\n");
+ }
+
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</sgx>\n");
+}
+
+
char *
virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
{
@@ -4788,6 +5013,9 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
if (qemuCaps->sevCapabilities)
virQEMUCapsFormatSEVInfo(qemuCaps, &buf);
+ if (qemuCaps->sgxCapabilities)
+ virQEMUCapsFormatSGXInfo(qemuCaps, &buf);
+
if (qemuCaps->kvmSupportsNesting)
virBufferAddLit(&buf, "<kvmSupportsNesting/>\n");
@@ -5455,6 +5683,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps,
return -1;
if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
return -1;
+ if (virQEMUCapsProbeQMPSGXCapabilities(qemuCaps, mon) < 0)
+ return -1;
virQEMUCapsInitProcessCaps(qemuCaps);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 6f35ba1485..fc8c0fde1b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -650,6 +650,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent */
QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */
+ QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
@@ -843,6 +844,9 @@ virQEMUCapsCPUFeatureFromQEMU(virQEMUCaps *qemuCaps,
virSEVCapability *
virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps);
+virSGXCapabilityPtr
+virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps);
+
bool
virQEMUCapsGetKVMSupportsSecureGuest(virQEMUCaps *qemuCaps) G_GNUC_NO_INLINE;
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
index e235532d62..0151ab07fa 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
@@ -7459,15 +7459,15 @@
"type": "bool"
},
{
- "name": "sgx1",
+ "name": "flc",
"type": "bool"
},
{
- "name": "sgx2",
+ "name": "sgx1",
"type": "bool"
},
{
- "name": "flc",
+ "name": "sgx2",
"type": "bool"
},
{
@@ -32707,6 +32707,22 @@
}
}
+{
+ "execute": "query-sgx-capabilities",
+ "id": "libvirt-51"
+}
+
+{
+ "return": {
+ "sgx": true,
+ "flc": false,
+ "sgx1": true,
+ "sgx2": false,
+ "section-size": 2048
+ },
+ "id": "libvirt-51"
+}
+
{
"execute": "query-cpu-model-expansion",
"arguments": {
@@ -32715,7 +32731,7 @@
"name": "host"
}
},
- "id": "libvirt-51"
+ "id": "libvirt-52"
}
{
@@ -33048,7 +33064,7 @@
}
}
},
- "id": "libvirt-51"
+ "id": "libvirt-52"
}
{
@@ -33062,7 +33078,7 @@
}
}
},
- "id": "libvirt-52"
+ "id": "libvirt-53"
}
{
@@ -33395,7 +33411,7 @@
}
}
},
- "id": "libvirt-52"
+ "id": "libvirt-53"
}
{
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index 19605d93ae..e1f177281f 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -238,6 +238,7 @@
<flag name='virtio-iommu-pci'/>
<flag name='virtio-net.rss'/>
<flag name='chardev.qemu-vdagent'/>
+ <flag name='sgx-epc'/>
<version>6002000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100244</microcodeVersion>
@@ -3706,4 +3707,10 @@
<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 supported='yes'>
+ <flc>no</flc>
+ <sgx1>yes</sgx1>
+ <sgx2>no</sgx2>
+ <section_size unit='KiB'>2</section_size>
+ </sgx>
</qemuCaps>
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
index 620442704a..9f806412f7 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
@@ -33317,6 +33317,32 @@
}
}
+{
+ "execute": "query-sgx-capabilities",
+ "id": "libvirt-51"
+}
+
+{
+ "return": {
+ "sgx": true,
+ "flc": false,
+ "sgx1": true,
+ "sgx2": false,
+ "section-size": 2048,
+ "sections": [
+ {
+ "node": 0,
+ "size": 1024
+ },
+ {
+ "node": 1,
+ "size": 1024
+ }
+ ]
+ },
+ "id": "libvirt-51"
+}
+
{
"execute": "query-cpu-model-expansion",
"arguments": {
@@ -33325,7 +33351,7 @@
"name": "host"
}
},
- "id": "libvirt-51"
+ "id": "libvirt-52"
}
{
@@ -33662,7 +33688,7 @@
}
}
},
- "id": "libvirt-51"
+ "id": "libvirt-52"
}
{
@@ -33676,7 +33702,7 @@
}
}
},
- "id": "libvirt-52"
+ "id": "libvirt-53"
}
{
@@ -34013,7 +34039,7 @@
}
}
},
- "id": "libvirt-52"
+ "id": "libvirt-53"
}
{
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
index 05f844fd5b..7cad1fd7d8 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
@@ -243,6 +243,7 @@
<flag name='virtio-net.rss'/>
<flag name='chardev.qemu-vdagent'/>
<flag name='display-dbus'/>
+ <flag name='sgx-epc'/>
<version>7000000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100243</microcodeVersion>
@@ -3771,4 +3772,14 @@
<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 supported='yes'>
+ <flc>no</flc>
+ <sgx1>yes</sgx1>
+ <sgx2>no</sgx2>
+ <section_size unit='KiB'>2</section_size>
+ <sections>
+ <section node='0' size='1'/>
+ <section node='1' size='1'/>
+ </sections>
+ </sgx>
</qemuCaps>
diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies
index 025d2db895..b9f9201ac7 100644
--- a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies
@@ -33866,6 +33866,32 @@
}
}
+{
+ "execute": "query-sgx-capabilities",
+ "id": "libvirt-51"
+}
+
+{
+ "return": {
+ "sgx": true,
+ "flc": false,
+ "sgx1": true,
+ "sgx2": false,
+ "section-size": 2048,
+ "sections": [
+ {
+ "node": 0,
+ "size": 1024
+ },
+ {
+ "node": 1,
+ "size": 1024
+ }
+ ]
+ },
+ "id": "libvirt-51"
+}
+
{
"execute": "query-cpu-model-expansion",
"arguments": {
@@ -33874,7 +33900,7 @@
"name": "host"
}
},
- "id": "libvirt-51"
+ "id": "libvirt-52"
}
{
@@ -34212,7 +34238,7 @@
}
}
},
- "id": "libvirt-51"
+ "id": "libvirt-52"
}
{
@@ -34226,7 +34252,7 @@
}
}
},
- "id": "libvirt-52"
+ "id": "libvirt-53"
}
{
@@ -34564,7 +34590,7 @@
}
}
},
- "id": "libvirt-52"
+ "id": "libvirt-53"
}
{
diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
index 3707d9b7c9..21b5e361b1 100644
--- a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
@@ -244,6 +244,7 @@
<flag name='chardev.qemu-vdagent'/>
<flag name='display-dbus'/>
<flag name='iothread.thread-pool-max'/>
+ <flag name='sgx-epc'/>
<version>7000050</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100244</microcodeVersion>
@@ -3569,4 +3570,14 @@
<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 supported='yes'>
+ <flc>no</flc>
+ <sgx1>yes</sgx1>
+ <sgx2>no</sgx2>
+ <section_size unit='KiB'>2</section_size>
+ <sections>
+ <section node='0' size='1'/>
+ <section node='1' size='1'/>
+ </sections>
+ </sgx>
</qemuCaps>
--
2.25.1On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang <haibin.huang@intel.com>
>
> the QMP capabilities:
> {"return":
> {
> "sgx": true,
> "section-size": 1024,
> "flc": true
> }
> }
>
> the domain capabilities:
> <sgx>
> <flc>yes</flc>
> <epc_size>1</epc_size>
> </sgx>
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> Signed-off-by: Haibin Huang <haibin.huang@intel.com>
> ---
> src/qemu/qemu_capabilities.c | 230 ++++++++++++++++++
> src/qemu/qemu_capabilities.h | 4 +
> .../caps_6.2.0.x86_64.replies | 30 ++-
> .../caps_6.2.0.x86_64.xml | 7 +
> .../caps_7.0.0.x86_64.replies | 34 ++-
> .../caps_7.0.0.x86_64.xml | 11 +
> .../caps_7.1.0.x86_64.replies | 34 ++-
> .../caps_7.1.0.x86_64.xml | 11 +
> 8 files changed, 346 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2c3be3ecec..57b5acb150 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> "chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
> "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
> "iothread.thread-pool-max", /* QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
> + "sgx-epc", /* QEMU_CAPS_SGX_EPC */
> );
>
>
> @@ -752,6 +753,8 @@ struct _virQEMUCaps {
>
> virSEVCapability *sevCapabilities;
>
> + virSGXCapability *sgxCapabilities;
> +
> /* Capabilities which may differ depending on the accelerator. */
> virQEMUCapsAccel kvm;
> virQEMUCapsAccel hvf;
> @@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
> { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
> { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
> { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
> + { "sgx-epc", QEMU_CAPS_SGX_EPC },
> };
>
>
> @@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
> }
>
>
> +static int
> +virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
> + virSGXCapability *src)
> +{
> + g_autoptr(virSGXCapability) tmp = NULL;
> +
For a convenience of caller, we can have a src == NULL check here and
return early.
> + tmp = g_new0(virSGXCapability, 1);
> +
> + tmp->flc = src->flc;
> + tmp->sgx1 = src->sgx1;
> + tmp->sgx2 = src->sgx2;
> + tmp->section_size = src->section_size;
> +
> + if (src->nSections == 0) {
> + tmp->nSections = 0;
> + tmp->pSections = NULL;
> + } else {
> + tmp->nSections = src->nSections;
> + tmp->pSections = src->pSections;
Ouch, this will definitely lead to a double free. If I run
qemucapabilitiestest after this patch I can see it clearly. I don't
quite understand why you didn't though. Fortunately, we can use plain
memcpy() since virSection struct does not contain any pointer, just a
pair of integers.
> + }
> +
> + *dst = g_steal_pointer(&tmp);
> + return 0;
> +}
> +
> +
> static void
> virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
> virQEMUCapsAccel *src)
> @@ -2053,6 +2083,12 @@ 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);
> }
>
> @@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
> virCPUDataFree(qemuCaps->cpuData);
>
> virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> + virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
>
> virQEMUCapsAccelClear(&qemuCaps->kvm);
> virQEMUCapsAccelClear(&qemuCaps->hvf);
> @@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps)
> }
>
>
> +virSGXCapabilityPtr
> +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps)
> +{
> + return qemuCaps->sgxCapabilities;
> +}
> +
> +
> static int
> virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
> qemuMonitor *mon)
> @@ -3442,6 +3486,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
> @@ -4220,6 +4289,116 @@ virQEMUCapsParseSEVInfo(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt)
> }
>
>
> +static int
> +virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps,
> + xmlXPathContextPtr ctxt)
> +{
> + g_autoptr(virSGXCapability) sgx = NULL;
> + xmlNodePtr node;
> +
> + g_autofree xmlNodePtr *nodes = NULL;
> + g_autofree xmlNodePtr *sectionNodes = NULL;
> + g_autofree char *flc = NULL;
> + g_autofree char *sgx1 = NULL;
> + g_autofree char *sgx2 = NULL;
> +
> + int n = 0;
> + int nsections = 0;
No need for extra empty lines. It's okay if this is one block.
> +
> + 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 ((!(flc = virXPathString("string(./sgx/flc)", ctxt))) ||
> + virStringParseYesNo(flc, &sgx->flc) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing or invalid SGX platform flc in QEMU capabilities cache"));
> + return -1;
> + }
> +
> + if ((!(sgx1 = virXPathString("string(./sgx/sgx1)", ctxt))) ||
> + virStringParseYesNo(sgx1, &sgx->sgx1) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing or invalid SGX platform sgx1 in QEMU capabilities cache"));
> + return -1;
> + }
> +
> + if ((!(sgx2 = virXPathString("string(./sgx/sgx2)", ctxt))) ||
> + virStringParseYesNo(sgx2, &sgx->sgx2) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing or invalid SGX platform sgx2 in QEMU capabilities cache"));
> + return -1;
> + }
> +
> + if (virXPathULongLong("string(./sgx/section_size)", ctxt, &sgx->section_size) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing or malformed SGX platform section_size in QEMU capabilities cache"));
> + return -1;
> + }
> +
> + if ((n = virXPathNodeSet("./sgx/sections", ctxt, &nodes)) < 0) {
Here, were intrested in a single occurrance, thus virXPathNode() could
be used.
> + sgx->nSections = 0;
> + sgx->pSections = NULL;
> + VIR_INFO("Sections was not obtained, so QEMU version is 6.2.0");
Again, no need for extra NOPs, VIR_INFO()...
> + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> + return 0;
> + }
> +
> + if (n == 0) {
> + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> + return 0;
> + }
> +
> + // Got the section, the QEMU version is above 7.0.0
We like c89 style of comments.
> + node = ctxt->node;
> + ctxt->node = nodes[0];
> + nsections = virXPathNodeSet("./section", ctxt, §ionNodes);
> + ctxt->node = node;
> +
> + if (nsections < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse CPU blockers in QEMU capabilities"));
> + return -1;
> + }
> +
> + if (nsections > 0) {
> + size_t i;
> + g_autofree char * strNode = NULL;
> + g_autofree char * strSize = NULL;
> + sgx->nSections = nsections;
> + sgx->pSections = g_new0(virSection, nsections + 1);
> +
> + for (i = 0; i < nsections; i++) {
> + if ((strNode = virXMLPropString(sectionNodes[i], "node")) &&
> + (virStrToLong_ui(strNode, NULL, 10, &(sgx->pSections[i].node)) < 0)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing node name in QEMU "
> + "capabilities cache"));
Error messages are extempt from 80 chars line rule. The reason is:
simpler git-grep. I, as a developer, can take whatever portion of error
message and 'git grep' it and find it easily. But if the message is
broken into multiple lines it's more tricky to do.
> + return -1;
> + }
> +
> + if ((strSize = virXMLPropString(sectionNodes[i], "size")) &&
> + (virStrToLong_ull(strSize, NULL, 10, &(sgx->pSections[i].size)) < 0)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing size name in QEMU "
> + "capabilities cache"));
> + return -1;
> + }
> + }
> + }
> +
> + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> + return 0;
> +}
> +
> +
> static int
> virQEMUCapsParseFlags(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt)
> {
> @@ -4522,6 +4701,9 @@ virQEMUCapsLoadCache(virArch hostArch,
> if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
> return -1;
>
> + if (virQEMUCapsParseSGXInfo(qemuCaps, ctxt) < 0)
> + return -1;
> +
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
> virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
> @@ -4707,6 +4889,49 @@ virQEMUCapsFormatSEVInfo(virQEMUCaps *qemuCaps, virBuffer *buf)
> }
>
>
> +static void
> +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps,
> + virBuffer *buf)
> +{
> + virSGXCapabilityPtr sgx = virQEMUCapsGetSGXCapabilities(qemuCaps);
> + size_t i;
> +
> + virBufferAddLit(buf, "<sgx supported='yes'>\n");
> + virBufferAdjustIndent(buf, 2);
> + if (sgx->flc) {
> + virBufferAsprintf(buf, "<flc>%s</flc>\n", "yes");
> + } else {
> + virBufferAsprintf(buf, "<flc>%s</flc>\n", "no");
> + }
Well, this works. But how about:
if (sgx->flc) {
virBufferAddLit(buf, "<flc>yes</flc>\n");
} else {
virBufferAddLit(buf, "<flc>no</flc>\n");
}
which saves and extra call to printf() for a static string? Or even better:
virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
> + if (sgx->sgx1) {
> + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "yes");
> + } else {
> + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "no");
> + }
> + if (sgx->sgx2) {
> + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "yes");
> + } else {
> + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "no");
> + }
> + virBufferAsprintf(buf, "<section_size unit='KiB'>%llu</section_size>\n", sgx->section_size);
> +
> + if (sgx->nSections > 0) {
> + virBufferAddLit(buf, "<sections>\n");
> +
> + for (i = 0; i < sgx->nSections; i++) {
> + virBufferAdjustIndent(buf, 2);
> + virBufferAsprintf(buf, "<section node='%u' ", sgx->pSections[i].node);
> + virBufferAsprintf(buf, "size='%llu'/>\n", sgx->pSections[i].size);
> + virBufferAdjustIndent(buf, -2);
> + }
> + virBufferAddLit(buf, "</sections>\n");
> + }
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</sgx>\n");
> +}
> +
> +
> char *
> virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
> {
> @@ -4788,6 +5013,9 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
> if (qemuCaps->sevCapabilities)
> virQEMUCapsFormatSEVInfo(qemuCaps, &buf);
>
> + if (qemuCaps->sgxCapabilities)
> + virQEMUCapsFormatSGXInfo(qemuCaps, &buf);
> +
> if (qemuCaps->kvmSupportsNesting)
> virBufferAddLit(&buf, "<kvmSupportsNesting/>\n");
>
> @@ -5455,6 +5683,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps,
> return -1;
> if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
> return -1;
> + if (virQEMUCapsProbeQMPSGXCapabilities(qemuCaps, mon) < 0)
> + return -1;
>
> virQEMUCapsInitProcessCaps(qemuCaps);
>
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 6f35ba1485..fc8c0fde1b 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -650,6 +650,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
> QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent */
> QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
> QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */
> + QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
> @@ -843,6 +844,9 @@ virQEMUCapsCPUFeatureFromQEMU(virQEMUCaps *qemuCaps,
> virSEVCapability *
> virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps);
>
> +virSGXCapabilityPtr
> +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps);
> +
> bool
> virQEMUCapsGetKVMSupportsSecureGuest(virQEMUCaps *qemuCaps) G_GNUC_NO_INLINE;
>
> diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> index e235532d62..0151ab07fa 100644
> --- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> +++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> @@ -7459,15 +7459,15 @@
> "type": "bool"
> },
> {
> - "name": "sgx1",
> + "name": "flc",
> "type": "bool"
> },
> {
> - "name": "sgx2",
> + "name": "sgx1",
> "type": "bool"
> },
> {
> - "name": "flc",
> + "name": "sgx2",
> "type": "bool"
> },
> {
This move does not seem to be warranted. When Peter generated the file
I'm quite certain that this was genuine order of attributes in which
QEMU replied. Furthermore, nothing in our code relies on ordering of
these particular attributes. Why is this necessary?
In fact, the QEMU I've built from git recently (v7.0.0-2668-gf9d9fff72e)
replied in this order:
{"return": {"sgx": true, "sgx2": false, "sgx1": true, "sections":
[{"size": 98041856, "node": 0}], "section-size": 98041856, "flc":
false}, "id": "libvirt-50"}
Michal
Hi Michal Peter,
Thank you for your comments.
Way 1:
virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
Way 2:
if (sgx->flc) {
virBufferAsprintf(buf, "<flc>yes</flc>\n");
} else {
virBufferAsprintf(buf, "<flc>yes</flc>\n");
}
For this section, both ways of writing work.
Peter Krempa said: "Don't use the ternary operator ('?'), use a full if/else branch instead or pick a better data structure."
You mean to be more concise use the ternary operator ('?').
I feel like it all makes sense.
> -----Original Message-----
> From: Michal Prívozník <mprivozn@redhat.com>
> Sent: Wednesday, July 20, 2022 7:27 PM
> To: Yang, Lin A <lin.a.yang@intel.com>; libvir-list@redhat.com; Huang, Haibin
> <haibin.huang@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>
> Subject: Re: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain
> capabilities
>
> On 7/1/22 21:14, Lin Yang wrote:
> > From: Haibin Huang <haibin.huang@intel.com>
> >
> > the QMP capabilities:
> > {"return":
> > {
> > "sgx": true,
> > "section-size": 1024,
> > "flc": true
> > }
> > }
> >
> > the domain capabilities:
> > <sgx>
> > <flc>yes</flc>
> > <epc_size>1</epc_size>
> > </sgx>
> >
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > Signed-off-by: Haibin Huang <haibin.huang@intel.com>
> > ---
> > src/qemu/qemu_capabilities.c | 230 ++++++++++++++++++
> > src/qemu/qemu_capabilities.h | 4 +
> > .../caps_6.2.0.x86_64.replies | 30 ++-
> > .../caps_6.2.0.x86_64.xml | 7 +
> > .../caps_7.0.0.x86_64.replies | 34 ++-
> > .../caps_7.0.0.x86_64.xml | 11 +
> > .../caps_7.1.0.x86_64.replies | 34 ++-
> > .../caps_7.1.0.x86_64.xml | 11 +
> > 8 files changed, 346 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/qemu/qemu_capabilities.c
> > b/src/qemu/qemu_capabilities.c index 2c3be3ecec..57b5acb150 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> > "chardev.qemu-vdagent", /*
> QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
> > "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
> > "iothread.thread-pool-max", /*
> > QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
> > + "sgx-epc", /* QEMU_CAPS_SGX_EPC */
> > );
> >
> >
> > @@ -752,6 +753,8 @@ struct _virQEMUCaps {
> >
> > virSEVCapability *sevCapabilities;
> >
> > + virSGXCapability *sgxCapabilities;
> > +
> > /* Capabilities which may differ depending on the accelerator. */
> > virQEMUCapsAccel kvm;
> > virQEMUCapsAccel hvf;
> > @@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags
> virQEMUCapsObjectTypes[] = {
> > { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
> > { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
> > { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
> > + { "sgx-epc", QEMU_CAPS_SGX_EPC },
> > };
> >
> >
> > @@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability
> **dst,
> > }
> >
> >
> > +static int
> > +virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
> > + virSGXCapability *src) {
> > + g_autoptr(virSGXCapability) tmp = NULL;
> > +
>
> For a convenience of caller, we can have a src == NULL check here and return
> early.
>
> > + tmp = g_new0(virSGXCapability, 1);
> > +
> > + tmp->flc = src->flc;
> > + tmp->sgx1 = src->sgx1;
> > + tmp->sgx2 = src->sgx2;
> > + tmp->section_size = src->section_size;
> > +
> > + if (src->nSections == 0) {
> > + tmp->nSections = 0;
> > + tmp->pSections = NULL;
> > + } else {
> > + tmp->nSections = src->nSections;
> > + tmp->pSections = src->pSections;
>
> Ouch, this will definitely lead to a double free. If I run qemucapabilitiestest
> after this patch I can see it clearly. I don't quite understand why you didn't
> though. Fortunately, we can use plain
> memcpy() since virSection struct does not contain any pointer, just a pair of
> integers.
>
> > + }
> > +
> > + *dst = g_steal_pointer(&tmp);
> > + return 0;
> > +}
> > +
> > +
> > static void
> > virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
> > virQEMUCapsAccel *src) @@ -2053,6
> > +2083,12 @@ 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);
> > }
> >
> > @@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
> > virCPUDataFree(qemuCaps->cpuData);
> >
> > virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> > + virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
> >
> > virQEMUCapsAccelClear(&qemuCaps->kvm);
> > virQEMUCapsAccelClear(&qemuCaps->hvf);
> > @@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps
> > *qemuCaps) }
> >
> >
> > +virSGXCapabilityPtr
> > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps) {
> > + return qemuCaps->sgxCapabilities; }
> > +
> > +
> > static int
> > virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
> > qemuMonitor *mon) @@ -3442,6 +3486,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 @@ -4220,6 +4289,116 @@
> virQEMUCapsParseSEVInfo(virQEMUCaps
> > *qemuCaps, xmlXPathContextPtr ctxt) }
> >
> >
> > +static int
> > +virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps,
> > + xmlXPathContextPtr ctxt) {
> > + g_autoptr(virSGXCapability) sgx = NULL;
> > + xmlNodePtr node;
> > +
> > + g_autofree xmlNodePtr *nodes = NULL;
> > + g_autofree xmlNodePtr *sectionNodes = NULL;
> > + g_autofree char *flc = NULL;
> > + g_autofree char *sgx1 = NULL;
> > + g_autofree char *sgx2 = NULL;
> > +
> > + int n = 0;
> > + int nsections = 0;
>
> No need for extra empty lines. It's okay if this is one block.
>
> > +
> > + 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 ((!(flc = virXPathString("string(./sgx/flc)", ctxt))) ||
> > + virStringParseYesNo(flc, &sgx->flc) < 0) {
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("missing or invalid SGX platform flc in QEMU capabilities
> cache"));
> > + return -1;
> > + }
> > +
> > + if ((!(sgx1 = virXPathString("string(./sgx/sgx1)", ctxt))) ||
> > + virStringParseYesNo(sgx1, &sgx->sgx1) < 0) {
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("missing or invalid SGX platform sgx1 in QEMU capabilities
> cache"));
> > + return -1;
> > + }
> > +
> > + if ((!(sgx2 = virXPathString("string(./sgx/sgx2)", ctxt))) ||
> > + virStringParseYesNo(sgx2, &sgx->sgx2) < 0) {
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("missing or invalid SGX platform sgx2 in QEMU capabilities
> cache"));
> > + return -1;
> > + }
> > +
> > + if (virXPathULongLong("string(./sgx/section_size)", ctxt, &sgx-
> >section_size) < 0) {
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("missing or malformed SGX platform section_size in QEMU
> capabilities cache"));
> > + return -1;
> > + }
> > +
> > + if ((n = virXPathNodeSet("./sgx/sections", ctxt, &nodes)) < 0) {
>
> Here, were intrested in a single occurrance, thus virXPathNode() could be
> used.
>
> > + sgx->nSections = 0;
> > + sgx->pSections = NULL;
> > + VIR_INFO("Sections was not obtained, so QEMU version is
> > + 6.2.0");
>
> Again, no need for extra NOPs, VIR_INFO()...
>
> > + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> > + return 0;
> > + }
> > +
> > + if (n == 0) {
> > + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> > + return 0;
> > + }
> > +
> > + // Got the section, the QEMU version is above 7.0.0
>
> We like c89 style of comments.
>
> > + node = ctxt->node;
> > + ctxt->node = nodes[0];
> > + nsections = virXPathNodeSet("./section", ctxt, §ionNodes);
> > + ctxt->node = node;
> > +
> > + if (nsections < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("failed to parse CPU blockers in QEMU capabilities"));
> > + return -1;
> > + }
> > +
> > + if (nsections > 0) {
> > + size_t i;
> > + g_autofree char * strNode = NULL;
> > + g_autofree char * strSize = NULL;
> > + sgx->nSections = nsections;
> > + sgx->pSections = g_new0(virSection, nsections + 1);
> > +
> > + for (i = 0; i < nsections; i++) {
> > + if ((strNode = virXMLPropString(sectionNodes[i], "node")) &&
> > + (virStrToLong_ui(strNode, NULL, 10, &(sgx->pSections[i].node)) <
> 0)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("missing node name in QEMU "
> > + "capabilities cache"));
>
> Error messages are extempt from 80 chars line rule. The reason is:
> simpler git-grep. I, as a developer, can take whatever portion of error
> message and 'git grep' it and find it easily. But if the message is broken into
> multiple lines it's more tricky to do.
>
> > + return -1;
> > + }
> > +
> > + if ((strSize = virXMLPropString(sectionNodes[i], "size")) &&
> > + (virStrToLong_ull(strSize, NULL, 10, &(sgx->pSections[i].size)) < 0))
> {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("missing size name in QEMU "
> > + "capabilities cache"));
> > + return -1;
> > + }
> > + }
> > + }
> > +
> > + qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> > + return 0;
> > +}
> > +
> > +
> > static int
> > virQEMUCapsParseFlags(virQEMUCaps *qemuCaps, xmlXPathContextPtr
> ctxt)
> > { @@ -4522,6 +4701,9 @@ virQEMUCapsLoadCache(virArch hostArch,
> > if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
> > return -1;
> >
> > + if (virQEMUCapsParseSGXInfo(qemuCaps, ctxt) < 0)
> > + return -1;
> > +
> > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
> > virQEMUCapsInitHostCPUModel(qemuCaps, hostArch,
> VIR_DOMAIN_VIRT_KVM);
> > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF)) @@ -4707,6
> +4889,49
> > @@ virQEMUCapsFormatSEVInfo(virQEMUCaps *qemuCaps, virBuffer
> *buf) }
> >
> >
> > +static void
> > +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps,
> > + virBuffer *buf) {
> > + virSGXCapabilityPtr sgx = virQEMUCapsGetSGXCapabilities(qemuCaps);
> > + size_t i;
> > +
> > + virBufferAddLit(buf, "<sgx supported='yes'>\n");
> > + virBufferAdjustIndent(buf, 2);
> > + if (sgx->flc) {
> > + virBufferAsprintf(buf, "<flc>%s</flc>\n", "yes");
> > + } else {
> > + virBufferAsprintf(buf, "<flc>%s</flc>\n", "no");
> > + }
>
> Well, this works. But how about:
>
> if (sgx->flc) {
> virBufferAddLit(buf, "<flc>yes</flc>\n"); } else {
> virBufferAddLit(buf, "<flc>no</flc>\n"); }
>
> which saves and extra call to printf() for a static string? Or even better:
>
> virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
>
> > + if (sgx->sgx1) {
> > + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "yes");
> > + } else {
> > + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "no");
> > + }
> > + if (sgx->sgx2) {
> > + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "yes");
> > + } else {
> > + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "no");
> > + }
> > + virBufferAsprintf(buf, "<section_size
> > + unit='KiB'>%llu</section_size>\n", sgx->section_size);
> > +
> > + if (sgx->nSections > 0) {
> > + virBufferAddLit(buf, "<sections>\n");
> > +
> > + for (i = 0; i < sgx->nSections; i++) {
> > + virBufferAdjustIndent(buf, 2);
> > + virBufferAsprintf(buf, "<section node='%u' ", sgx-
> >pSections[i].node);
> > + virBufferAsprintf(buf, "size='%llu'/>\n", sgx->pSections[i].size);
> > + virBufferAdjustIndent(buf, -2);
> > + }
> > + virBufferAddLit(buf, "</sections>\n");
> > + }
> > +
> > + virBufferAdjustIndent(buf, -2);
> > + virBufferAddLit(buf, "</sgx>\n"); }
> > +
> > +
> > char *
> > virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) { @@ -4788,6
> +5013,9
> > @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
> > if (qemuCaps->sevCapabilities)
> > virQEMUCapsFormatSEVInfo(qemuCaps, &buf);
> >
> > + if (qemuCaps->sgxCapabilities)
> > + virQEMUCapsFormatSGXInfo(qemuCaps, &buf);
> > +
> > if (qemuCaps->kvmSupportsNesting)
> > virBufferAddLit(&buf, "<kvmSupportsNesting/>\n");
> >
> > @@ -5455,6 +5683,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps
> *qemuCaps,
> > return -1;
> > if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
> > return -1;
> > + if (virQEMUCapsProbeQMPSGXCapabilities(qemuCaps, mon) < 0)
> > + return -1;
> >
> > virQEMUCapsInitProcessCaps(qemuCaps);
> >
> > diff --git a/src/qemu/qemu_capabilities.h
> > b/src/qemu/qemu_capabilities.h index 6f35ba1485..fc8c0fde1b 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -650,6 +650,7 @@ typedef enum { /* virQEMUCapsFlags grouping
> marker for syntax-check */
> > QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent
> */
> > QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
> > QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object
> > iothread.thread-pool-max */
> > + QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */
> >
> > QEMU_CAPS_LAST /* this must always be the last item */ }
> > virQEMUCapsFlags; @@ -843,6 +844,9 @@
> > virQEMUCapsCPUFeatureFromQEMU(virQEMUCaps *qemuCaps,
> virSEVCapability
> > * virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps);
> >
> > +virSGXCapabilityPtr
> > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps);
> > +
> > bool
> > virQEMUCapsGetKVMSupportsSecureGuest(virQEMUCaps *qemuCaps)
> > G_GNUC_NO_INLINE;
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > index e235532d62..0151ab07fa 100644
> > --- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > +++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > @@ -7459,15 +7459,15 @@
> > "type": "bool"
> > },
> > {
> > - "name": "sgx1",
> > + "name": "flc",
> > "type": "bool"
> > },
> > {
> > - "name": "sgx2",
> > + "name": "sgx1",
> > "type": "bool"
> > },
> > {
> > - "name": "flc",
> > + "name": "sgx2",
> > "type": "bool"
> > },
> > {
>
> This move does not seem to be warranted. When Peter generated the file
> I'm quite certain that this was genuine order of attributes in which QEMU
> replied. Furthermore, nothing in our code relies on ordering of these
> particular attributes. Why is this necessary?
>
> In fact, the QEMU I've built from git recently (v7.0.0-2668-gf9d9fff72e)
> replied in this order:
>
> {"return": {"sgx": true, "sgx2": false, "sgx1": true, "sections":
> [{"size": 98041856, "node": 0}], "section-size": 98041856, "flc":
> false}, "id": "libvirt-50"}
>
> Michal
On 7/25/22 03:31, Huang, Haibin wrote:
> Hi Michal Peter,
>
> Thank you for your comments.
>
> Way 1:
> virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
>
> Way 2:
> if (sgx->flc) {
> virBufferAsprintf(buf, "<flc>yes</flc>\n");
> } else {
> virBufferAsprintf(buf, "<flc>yes</flc>\n");
> }
>
> For this section, both ways of writing work.
>
> Peter Krempa said: "Don't use the ternary operator ('?'), use a full if/else branch instead or pick a better data structure."
> You mean to be more concise use the ternary operator ('?').
Yeah, this one should be exempt from our ternary operator rule. The idea
behin the rule is to avoid complicated (or even composed) expressions
with a ternary operator. However, this is not the case and in fact,
ternary operator makes the code more readable. And we already use this
pattern for formatting other booleans, indeed.
We may switch to virTristateBool but one can argue that FLC is not
really tristate, it's either present or not; there's no _ABSENT state.
On the other hand, we may assign _ABSENT special meaning - the structure
was just allocated and does not hold decisive answer for FLC presence.
So let me switch to virTristateBool.
BTW: I've discussed with Yang that I'll fix up these patches and resend.
Is that agreement still on?
Michal
> BTW: I've discussed with Yang that I'll fix up these patches and resend. > Is that agreement still on? Yes, it's still on. Please go ahead. I just didn't get chance to sync with Haibin after we made agreement last week. Thanks, Lin.
© 2016 - 2026 Red Hat, Inc.