Re: [libvirt] [PATCH v3 1/9] qemu: provide support to query the SEV capability

On 03/14/2018 11:44 AM, Brijesh Singh wrote: > QEMU version >= 2.12 provides support for launching an encrypted VMs on > AMD x86 platform using Secure Encrypted Virtualization (SEV) feature. > This patch adds support to query the SEV capability from the qemu. > > Reviewed-by: "Daniel P. Berrangé" <berrange(a)redhat.com&gt; > Signed-off-by: Brijesh Singh <brijesh.singh(a)amd.com&gt; > --- > src/conf/domain_capabilities.h | 13 ++++++++ > src/qemu/qemu_capabilities.c | 38 ++++++++++++++++++++++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_capspriv.h | 4 +++ > src/qemu/qemu_monitor.c | 9 ++++++ > src/qemu/qemu_monitor.h | 3 ++ > src/qemu/qemu_monitor_json.c | 73 ++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor_json.h | 3 ++ > 8 files changed, 144 insertions(+) > Now that the 2.12 capabilities were added, I started looking through each of the upstream patch series that would make use of it - this is just "next" on my list. I tried applying just this patch, but kept getting: 29) caps_2.12.0(x86_64) ... libvirt: QEMU Driver error : internal error: query-cpu-definitions reply data was not an array a small bit of debugging found that qemuMonitorJSONGetCPUDefinitions was returning NULL for @data after/when the "query-sev-capabilities". I narrowed it down into the virQEMUCapsInitQMPMonitor when run during qemucapabilitiestest (see [1])

> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h > index fa4c1e442f57..72e9daf9120f 100644 > --- a/src/conf/domain_capabilities.h > +++ b/src/conf/domain_capabilities.h > @@ -137,6 +137,19 @@ struct _virDomainCapsCPU { > virDomainCapsCPUModelsPtr custom; > }; > > +/* > + * SEV capabilities > + */ > +typedef struct _virSEVCapability virSEVCapability; > +typedef virSEVCapability *virSEVCapabilityPtr; > +struct _virSEVCapability { > + char *pdh; > + char *cert_chain; > + unsigned int cbitpos; > + unsigned int reduced_phys_bits; > +}; > + > + > struct _virDomainCaps { > virObjectLockable parent; > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 3eb5ed6d1a60..6da7cf7477c7 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "pl011", > "machine.pseries.max-cpu-compat", > "dump-completed", > + "sev", This should also be "sev-guest" to be consistent with other virQEMUCapsObjectTypes entries naming. BTW: You can/should pull out this entry, the one in virQEMUCapsObjectTypes and the qemu_capabilities.h change to add the entry to virQEMUCapsFlags, then build and run: VIR_TEST_REGENERATE_OUTPUT=1 tests/qemucapabilitiestest in order to get/see the changed tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml file. > ); > > > @@ -525,6 +526,8 @@ struct _virQEMUCaps { > size_t ngicCapabilities; > virGICCapability *gicCapabilities; > > + virSEVCapability *sevCapabilities; > + > virQEMUCapsHostCPUData kvmCPU; > virQEMUCapsHostCPUData tcgCPU; > }; > @@ -1694,6 +1697,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { > { "sclplmconsole", QEMU_CAPS_DEVICE_SCLPLMCONSOLE }, > { "isa-serial", QEMU_CAPS_DEVICE_ISA_SERIAL }, > { "pl011", QEMU_CAPS_DEVICE_PL011 }, > + { "sev-guest", QEMU_CAPS_SEV }, > }; > > static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { > @@ -2770,6 +2774,21 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, > qemuCaps->ngicCapabilities = ncapabilities; > } > > +void > +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, > + virSEVCapability *capabilities) > +{ > + virSEVCapability *cap = qemuCaps->sevCapabilities; > + > + if (cap) { > + VIR_FREE(cap->pdh); > + VIR_FREE(cap->cert_chain); > + } > + > + VIR_FREE(qemuCaps->sevCapabilities); > + > + qemuCaps->sevCapabilities = capabilities; > +} > > static int > virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps, > @@ -3273,6 +3292,19 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, > return 0; > } > > +static int > +virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps, > + qemuMonitorPtr mon) > +{ > + virSEVCapability *caps = NULL; > + > + if (qemuMonitorGetSEVCapabilities(mon, &caps) < 0) > + return -1; > + > + virQEMUCapsSetSEVCapabilities(qemuCaps, caps); > + > + return 0; > +} > > bool > virQEMUCapsCPUFilterFeatures(const char *name, > @@ -4906,6 +4938,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, > virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) > virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_CACHE); > > + /* Probe for SEV capabilities */ > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV)) { > + if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) > + virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV); [1] If I change the Clear to be a goto cleanup, then a slightly different error appears: libvirt: QEMU Driver error : internal error: 'cbitpos' field is missing Checking the output of a LIBVIRT_DEBUG=1 VIR_TEST_DEBUG=1 VIR_TEST_RANGE=29 make -C tests check TESTS=qemucapabilitiestest gives a bit of a hint in the test-suite.log output: 2018-03-23 19:08:52.819+0000: 11727: debug : qemuMonitorTestAddResponse:108 : Adding response to monitor command: '{ "return": { }, "id": "libvirt-1" } ... So that says to me that we perhaps need some mocked output. Although I'm not 100% sure - it doesn't seem previous adjustments needed those, but there's lots that changes in this capabilities space and I'm not always up to date on the mocking code. BTW: resetting the last error in this case didn't fix the problem - it just moved the cheese back to the original error. So there's some mocked buffer somewhere that's being read - I'm hoping someone with a fresher memory on how this works will be able to debug faster than I. > + } > + > ret = 0; > cleanup: > return ret; > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index c2ec2be19311..02acae491ab5 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -444,6 +444,7 @@ typedef enum { > QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */ > QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine pseries,max-cpu-compat= */ > QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */ > + QEMU_CAPS_SEV, /* -object sev-guest,... */ I think this should be QEMU_CAPS_SEV_GUEST (it's a consistency in naming thing) I'll defer to Dan and Peter on the rest... John > > QEMU_CAPS_LAST /* this must always be the last item */ > } virQEMUCapsFlags; > diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h > index 222f3368e3b6..1fa85cc14f07 100644 > --- a/src/qemu/qemu_capspriv.h > +++ b/src/qemu/qemu_capspriv.h > @@ -86,6 +86,10 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, > virGICCapability *capabilities, > size_t ncapabilities); > > +void > +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, > + virSEVCapability *capabilities); > + > int > virQEMUCapsParseHelpStr(const char *qemu, > const char *str, > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 1d67a97789e7..2820714b5c55 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -4007,6 +4007,15 @@ qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, > return qemuMonitorJSONGetGICCapabilities(mon, capabilities); > } > > +int > +qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, > + virSEVCapability **capabilities) > +{ > + QEMU_CHECK_MONITOR_JSON(mon); > + > + return qemuMonitorJSONGetSEVCapabilities(mon, capabilities); > +} > + > > int > qemuMonitorNBDServerStart(qemuMonitorPtr mon, > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index adfa87aba91b..aaa14f66fdfb 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -767,6 +767,9 @@ int qemuMonitorSetMigrationCapability(qemuMonitorPtr mon, > int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, > virGICCapability **capabilities); > > +int qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, > + virSEVCapability **capabilities); > + > typedef enum { > QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0, > QEMU_MONITOR_MIGRATE_NON_SHARED_DISK = 1 << 1, /* migration with non-shared storage with full disk copy */ > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 08dfffdf6435..c51b98d2bda7 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -6398,6 +6398,79 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, > return ret; > } > > +int > +qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, > + virSEVCapability **capabilities) > +{ > + int ret = -1; > + virJSONValuePtr cmd; > + virJSONValuePtr reply = NULL; > + virJSONValuePtr caps; > + virSEVCapability *capability = NULL; > + const char *pdh = NULL, *cert_chain = NULL; > + int cbitpos, reduced_phys_bits; > + > + *capabilities = NULL; > + > + if (!(cmd = qemuMonitorJSONMakeCommand("query-sev-capabilities", > + NULL))) > + return -1; > + > + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > + goto cleanup; > + > + > + if (qemuMonitorJSONCheckError(cmd, reply) < 0) > + goto cleanup; > + > + caps = virJSONValueObjectGetObject(reply, "return"); > + > + if (virJSONValueObjectGetNumberInt(caps, "cbitpos", &cbitpos) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'cbitpos' field is missing")); > + goto cleanup; > + } > + > + if (virJSONValueObjectGetNumberInt(caps, "reduced-phys-bits", > + &reduced_phys_bits) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'reduced-phys-bits' field is missing")); > + goto cleanup; > + } > + > + if (!(pdh = virJSONValueObjectGetString(caps, "pdh"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'pdh' field is missing")); > + goto cleanup; > + } > + > + if (!(cert_chain = virJSONValueObjectGetString(caps, "cert-chain"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'cert-chain' field is missing")); > + goto cleanup; > + } > + > + if (VIR_ALLOC(capability) < 0) > + goto cleanup; > + > + if (VIR_STRDUP(capability->pdh, pdh) < 0) > + goto cleanup; > + > + if (VIR_STRDUP(capability->cert_chain, cert_chain) < 0) > + goto cleanup; > + > + capability->cbitpos = cbitpos; > + capability->reduced_phys_bits = reduced_phys_bits; > + *capabilities = capability; > + ret = 0; > + > + cleanup: > + virJSONValueFree(cmd); > + virJSONValueFree(reply); > + > + return ret; > +} > + > static virJSONValuePtr > qemuMonitorJSONBuildInetSocketAddress(const char *host, > const char *port) > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index ec243becc4ae..305f789902e9 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -152,6 +152,9 @@ int qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon, > int qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, > virGICCapability **capabilities); > > +int qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, > + virSEVCapability **capabilities); > + > int qemuMonitorJSONMigrate(qemuMonitorPtr mon, > unsigned int flags, > const char *uri); >