On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang <haibin.huang@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Haibin Huang <haibin.huang@xxxxxxxxx>
> ---
> 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
