On Wed, Aug 15, 2018 at 17:02:07 +0200, Erik Skultety wrote: This should be noted in a comment for the function as for this specific case this function will not report an error but will for any other case. It's also partially weird since the function also reports other errors besides allocation errors which any sane caller has to ignore. ACK if you add a comment block for this function describing this weirdness. You can also come up with a saner detection method e.g. to return a boolean via arguments or 1 via return value which says whether it's supported or not and reserve -1 for real errors.

Since we're not saving the platform-specific data into a cache, we're not going to populate the structure, which in turn will cause a crash upon calling virNodeGetSEVInfo because of a NULL pointer dereference. Ultimately, we should start caching this data along with host-specific capabilities like NUMA and SELinux stuff into a separate cache, but for the time being, this is a semi-proper fix for a potential crash. Backtrace (requires libvirtd restart to load qemu caps from cache): https://bugzilla.redhat.com/show_bug.cgi?id=1612009 Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 100 ++++++++++++++++++++++ tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml | 5 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 6 ++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c17d26801e..9de12f9263 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1570,6 +1570,25 @@ virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData) } +static int +virQEMUCapsSEVInfoCopy(virSEVCapabilityPtr *dst, + virSEVCapabilityPtr src) +{ + VIR_AUTOPTR(virSEVCapability) tmp = NULL; + + if (VIR_ALLOC(tmp) < 0 || + VIR_STRDUP(tmp->pdh, src->pdh) < 0 || + VIR_STRDUP(tmp->cert_chain, src->cert_chain) < 0) + return -1; + + tmp->cbitpos = src->cbitpos; + tmp->reduced_phys_bits = src->reduced_phys_bits; + + VIR_STEAL_PTR(*dst, tmp); + return 0; +} + + virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) { virQEMUCapsPtr ret = virQEMUCapsNew(); @@ -1632,6 +1651,11 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) for (i = 0; i < qemuCaps->ngicCapabilities; i++) ret->gicCapabilities[i] = qemuCaps->gicCapabilities[i]; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && + virQEMUCapsSEVInfoCopy(&ret->sevCapabilities, + qemuCaps->sevCapabilities) < 0) + goto error; + return ret; error: @@ -3342,6 +3366,58 @@ virQEMUCapsCachePrivFree(void *privData) } +static int +virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt) +{ + VIR_AUTOPTR(virSEVCapability) sev = NULL; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) + return 0; + + if (virXPathBoolean("boolean(./sev)", ctxt) == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV platform data in QEMU " + "capabilities cache")); + return -1; + } + + if (VIR_ALLOC(sev) < 0) + return -1; + + if (virXPathUInt("string(./sev/cbitpos)", ctxt, &sev->cbitpos) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or malformed SEV cbitpos information " + "in QEMU capabilities cache")); + return -1; + } + + if (virXPathUInt("string(./sev/reducedPhysBits)", ctxt, + &sev->reduced_phys_bits) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or malformed SEV reducedPhysBits information " + "in QEMU capabilities cache")); + return -1; + } + + if (!(sev->pdh = virXPathString("string(./sev/pdh)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV pdh information " + "in QEMU capabilities cache")); + return -1; + } + + if (!(sev->cert_chain = virXPathString("string(./sev/certChain)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV certChain information " + "in QEMU capabilities cache")); + return -1; + } + + VIR_STEAL_PTR(qemuCaps->sevCapabilities, sev); + return 0; +} + + /* * Parsing a doc that looks like * @@ -3590,6 +3666,9 @@ virQEMUCapsLoadCache(virArch hostArch, } VIR_FREE(nodes); + if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0) + goto cleanup; + virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM); virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU); @@ -3707,6 +3786,24 @@ virQEMUCapsFormatCPUModels(virQEMUCapsPtr qemuCaps, } +static void +virQEMUCapsFormatSEVInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf) +{ + virSEVCapabilityPtr sev = virQEMUCapsGetSEVCapabilities(qemuCaps); + + virBufferAddLit(buf, "<sev>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<cbitpos>%u</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reducedPhysBits>%u</reducedPhysBits>\n", + sev->reduced_phys_bits); + virBufferEscapeString(buf, "<pdh>%s</pdh>\n", sev->pdh); + virBufferEscapeString(buf, "<certChain>%s</certChain>\n", + sev->cert_chain); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</sev>\n"); +} + + char * virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) { @@ -3788,6 +3885,9 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) emulated ? "yes" : "no"); } + if (qemuCaps->sevCapabilities) + virQEMUCapsFormatSEVInfo(qemuCaps, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n"); diff --git a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml index 7a1be4c093..a8d6a4d629 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml @@ -142,6 +142,9 @@ <gic supported='no'/> <vmcoreinfo supported='yes'/> <genid supported='yes'/> - <sev supported='no'/> + <sev supported='yes'> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + </sev> </features> </domainCapabilities> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index d34d762ca8..d134e5632d 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -1253,4 +1253,10 @@ <machine name='pc-0.11' hotplugCpus='yes' maxCpus='255'/> <machine name='pc-0.12' hotplugCpus='yes' maxCpus='255'/> <machine name='pc-0.10' hotplugCpus='yes' maxCpus='255'/> + <sev> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + <pdh>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</pdh> + <certChain>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</certChain> + </sev> </qemuCaps> -- 2.14.4