On 7/28/22 10:53, Peter Krempa wrote:
> On Wed, Jul 27, 2022 at 12:34:55 +0200, Michal Privoznik 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: Haibin Huang <haibin.huang@xxxxxxxxx>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>> 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.
Fair enough, let me split 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
Yep, noted :-D
>
>> +
>> + 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, §ionNodes);
>> +
>> + 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.
>
Alright. We have examples in other versions to show the code working.
Michal
