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@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@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);