Re: [PATCH v14 09/15] Convert QMP capabilities to domain capabilities

On Wed, Jul 27, 2022 at 12:34:55 +0200, Michal Privoznik wrote: > From: Haibin Huang <haibin.huang(a)intel.com&gt; > > 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: Haibin Huang <haibin.huang(a)intel.com&gt; > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_capabilities.c | 206 ++++++++++++++++++ > src/qemu/qemu_capabilities.h | 6 + > .../caps_6.2.0.x86_64.replies | 24 +- > .../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 + Preferrably do not mix addition to capability probing with other stuff such as the capabiltiies XML formatter/parser next time. You can always add the formatter/parser first and then do the minimum required to add capability flag and probe it.

> 8 files changed, 321 insertions(+), 12 deletions(-) [...] > @@ -1973,6 +1979,36 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst, > } > > > +static int > +virQEMUCapsSGXInfoCopy(virSGXCapability **dst, > + virSGXCapability *src) > +{ > + g_autoptr(virSGXCapability) tmp = NULL; > + > + if (!src) { > + *dst = NULL; > + return 0; > + } > + > + 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->sections = g_new0(virSection, src->nsections); > + memcpy(tmp->sections, src->sections, > + src->nsections * sizeof(*tmp->sections)); > + tmp->nsections = src->nsections; > + } > + > + *dst = g_steal_pointer(&tmp); > + return 0; > +} > + > + > static void > virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst, > virQEMUCapsAccel *src) > @@ -2054,6 +2090,12 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps) > qemuCaps->sevCapabilities) < 0) > return NULL; > > + > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) && This doesn't seem to be needed ... > + virQEMUCapsSGXInfoCopy(&ret->sgxCapabilities, as this doesn't copy anything if 'src' is NULL. > + qemuCaps->sgxCapabilities) < 0) > + return NULL; > + > return g_steal_pointer(&ret); > } > [...] > @@ -4221,6 +4296,98 @@ virQEMUCapsParseSEVInfo(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt) > } > > > +static int > +virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps, > + xmlXPathContextPtr ctxt) > +{ > + g_autoptr(virSGXCapability) sgx = NULL; > + xmlNodePtr sections = NULL; > + g_autofree char *flc = NULL; > + g_autofree char *sgx1 = NULL; > + g_autofree char *sgx2 = NULL; > + > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC)) > + return 0; Note that this flag

> + > + 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 ((sections = virXPathNode("./sgx/sections", ctxt))) { > + g_autofree xmlNodePtr *sectionNodes = NULL; > + int nsections = 0; > + size_t i; > + VIR_XPATH_NODE_AUTORESTORE(ctxt); > + > + ctxt->node = sections; > + nsections = virXPathNodeSet("./section", ctxt, &sectionNodes); > + > + if (nsections < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("failed to parse SGX sections in QEMU capabilities cache")); > + return -1; > + } > + > + sgx->nsections = nsections; > + sgx->sections = g_new0(virSection, nsections); > + > + for (i = 0; i < nsections; i++) { > + g_autofree char * strNode = NULL; > + g_autofree char * strSize = NULL; > + > + if (!(strNode = virXMLPropString(sectionNodes[i], "node")) || > + virStrToLong_i(strNode, NULL, 10, &sgx->sections[i].node) < 0) { We have helpers such as virXMLPropUInt and similar, removing the need for temporary strings and explicit parsing of the numbers. I'd prefer if you use them instead of this open coding .... in the whole function. > + 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->sections[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 void > +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps, > + virBuffer *buf) > +{ > + virSGXCapability *sgx = virQEMUCapsGetSGXCapabilities(qemuCaps); > + > + virBufferAddLit(buf, "<sgx supported='yes'>\n"); > + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no"); > + virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", sgx->sgx1 ? "yes" : "no"); > + virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", sgx->sgx2 ? "yes" : "no"); > + virBufferAsprintf(buf, "<section_size unit='KiB'>%llu</section_size>\n", sgx->section_size); If the 'section_size' vanishes from qemu, will this field need to be removed? > + > + if (sgx->nsections > 0) { > + size_t i; > + virBufferAddLit(buf, "<sections>\n"); > + > + for (i = 0; i < sgx->nsections; i++) { > + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<section node='%u' ", sgx->sections[i].node); > + virBufferAsprintf(buf, "size='%llu'/>\n", sgx->sections[i].size); > + virBufferAdjustIndent(buf, -2); > + } > + virBufferAddLit(buf, "</sections>\n"); > + } > + > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</sgx>\n"); > +} > + > + > char * > virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) > { > @@ -4789,6 +4990,9 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) > if (qemuCaps->sevCapabilities) > virQEMUCapsFormatSEVInfo(qemuCaps, &buf); > > + if (qemuCaps->sgxCapabilities) As example for my comment about copying the caps, here you don't check the capability. > + virQEMUCapsFormatSGXInfo(qemuCaps, &buf); > + > if (qemuCaps->kvmSupportsNesting) > virBufferAddLit(&buf, "<kvmSupportsNesting/>\n"); > [...] capability->sections[i].size > diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies > index d893d67ea8..c221b9e034 100644 > --- a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies > +++ b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.replies > @@ -34006,6 +34006,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 > + } Next time I'll be re-generating the capabilities this will get overwritten by: + "id": "libvirt-51", + "error": { + "class": "GenericError", + "desc": "SGX is not enabled in KVM" + } as my box does not support it. I'd strongly prefer to use this syntax to avoid changes in my caps bump patch.