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

Michal Prívozník mprivozn at redhat.com
Thu Jul 28 14:05:08 UTC 2022


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 at 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: Haibin Huang <haibin.huang at intel.com>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  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, &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.
> 

Alright. We have examples in other versions to show the code working.

Michal



More information about the libvir-list mailing list