8:31 p.m.
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(a)redhat.com>
Sent: Wednesday, July 20, 2022 7:27 PM
To: Yang, Lin A <lin.a.yang(a)intel.com>; libvir-list(a)redhat.com; Huang, Haibin
<haibin.huang(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)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(a)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(a)redhat.com>
> Signed-off-by: Haibin Huang <haibin.huang(a)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