[libvirt] [PATCH v3 1/9] qemu: provide support to query the SEV capability
Brijesh Singh
brijesh.singh at amd.com
Mon Mar 26 13:52:33 UTC 2018
Hi John,
On 03/23/2018 02:18 PM, John Ferlan wrote:
>
>
> 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 at redhat.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>> 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])
I have not tried latest libvirt yet, I will try this today and debug to
see what we are missing. I did the 'make check' before submitting the
patch but at that time QEMU 2.12 was not available and we did not had
updated caps_2.12.0.x86_64.xml and caps_2.12.0.x86_64.replies.
I will investigate a bit more and update with my findings.
thanks
>
>> 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);
>>
More information about the libvir-list
mailing list