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

On Wed, May 23, 2018 at 04:18:26PM -0500, 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. > > Signed-off-by: Brijesh Singh <brijesh.singh(a)amd.com&gt; > --- > src/conf/domain_capabilities.h | 13 ++++ > src/qemu/qemu_capabilities.c | 47 ++++++++++++++ > src/qemu/qemu_capabilities.h | 4 ++ > src/qemu/qemu_capspriv.h | 4 ++ > src/qemu/qemu_monitor.c | 9 +++ > src/qemu/qemu_monitor.h | 3 + > src/qemu/qemu_monitor_json.c | 74 ++++++++++++++++++++++ > src/qemu/qemu_monitor_json.h | 3 + > .../caps_2.12.0.x86_64.replies | 10 +++ > tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 3 +- > 10 files changed, 169 insertions(+), 1 deletion(-) > > diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h > index 9b852e8649bf..c1093234ceb8 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 8a63db5f4f33..49b74f7e12c1 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -489,6 +489,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "screendump_device", > "hda-output", > "blockdev-del", > + "sev-guest", > ); > > > @@ -555,6 +556,8 @@ struct _virQEMUCaps { > size_t ngicCapabilities; > virGICCapability *gicCapabilities; > > + virSEVCapability *sevCapabilities; > + > virQEMUCapsHostCPUData kvmCPU; > virQEMUCapsHostCPUData tcgCPU; > }; > @@ -1121,6 +1124,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { > { "virtual-css-bridge", QEMU_CAPS_CCW }, > { "vfio-ccw", QEMU_CAPS_DEVICE_VFIO_CCW }, > { "hda-output", QEMU_CAPS_HDA_OUTPUT }, > + { "sev-guest", QEMU_CAPS_SEV_GUEST }, > }; > > static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { > @@ -2050,6 +2054,28 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, > } > > > +void > +virQEMUSevCapabilitiesFree(virSEVCapability *cap) Since virSEVCapability will be added to virDomainCaps too, you need to move ^this into domain_capabilities.c so it will become virSEVCapabilityFree, I've got a further comment regarding this in patch 2 as well. NOTE: notice the SEV in the function name, we should stay consistent in naming and since SEV is the name of the feature...

> +{ > + if (!cap) > + return; > + > + VIR_FREE(cap->pdh); > + VIR_FREE(cap->cert_chain); > + VIR_FREE(cap); > +} > + > + > +void > +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, > + virSEVCapability *capabilities) > +{ > + virQEMUSevCapabilitiesFree(qemuCaps->sevCapabilities); virSEVCapabilityFree(qemuCaps->sevCapabilities) > + > + qemuCaps->sevCapabilities = capabilities; > +} > + > + > static int > virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps, > qemuMonitorPtr mon) > @@ -2580,6 +2606,21 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, > } > > > +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, > void *opaque) > @@ -3965,6 +4006,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, > virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW); > } > > + /* Probe for SEV capabilities */ > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { > + if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) > + virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV_GUEST); > + } > + > ret = 0; > cleanup: > return ret; > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index 3e120e64c0b4..8b7eef4359b7 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -473,6 +473,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > QEMU_CAPS_SCREENDUMP_DEVICE, /* screendump command accepts device & head */ > QEMU_CAPS_HDA_OUTPUT, /* -device hda-output */ > QEMU_CAPS_BLOCKDEV_DEL, /* blockdev-del is supported */ > + QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */ > > QEMU_CAPS_LAST /* this must always be the last item */ > } virQEMUCapsFlags; > @@ -599,4 +600,7 @@ bool virQEMUCapsGuestIsNative(virArch host, > bool virQEMUCapsCPUFilterFeatures(const char *name, > void *opaque); > > +void > +virQEMUSevCapabilitiesFree(virSEVCapability *capabilities); > + > #endif /* __QEMU_CAPABILITIES_H__*/ > diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h > index 0199501c931b..20b03876d470 100644 > --- a/src/qemu/qemu_capspriv.h > +++ b/src/qemu/qemu_capspriv.h > @@ -85,6 +85,10 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, > virGICCapability *capabilities, > size_t ncapabilities); > > +void > +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, > + virSEVCapability *capabilities); > + > int > virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps, > qemuMonitorPtr mon, > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 43f1d2f81671..3b034930408c 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -3778,6 +3778,15 @@ qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, > return qemuMonitorJSONGetGICCapabilities(mon, capabilities); > } 1 more blank line here... > > +int > +qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, > + virSEVCapability **capabilities) > +{ > + QEMU_CHECK_MONITOR(mon); > + > + return qemuMonitorJSONGetSEVCapabilities(mon, capabilities); > +} > + > > int > qemuMonitorNBDServerStart(qemuMonitorPtr mon, > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index c28db1a52b8b..b1b7ef09c929 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -710,6 +710,9 @@ int qemuMonitorSetMigrationCapabilities(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 9f5c35879587..24d3a2ff412f 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -6436,6 +6436,80 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, > return ret; > } > Need 1 more blank line here... > +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; We declared ^these as uint in virSEVCapability, so that fact should be reflected here too...

> + > + *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) { GetNumberUInt()...

> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'cbitpos' field is missing")); > + goto cleanup; > + } > + > + if (virJSONValueObjectGetNumberInt(caps, "reduced-phys-bits", > + &reduced_phys_bits) < 0) { GetNumberUInt()...

> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'reduced-phys-bits' field is missing")); we should mention the query command that failed, i.e. "query-sev-capabilities reply was missing 'xyz' field"

> + goto cleanup; > + } > + > + if (!(pdh = virJSONValueObjectGetString(caps, "pdh"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'pdh' field is missing")); same here > + goto cleanup; > + } > + > + if (!(cert_chain = virJSONValueObjectGetString(caps, "cert-chain"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", same here... > + _("'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; > + VIR_STEAL_PTR(*capabilities, capability); > + ret = 0; > + > + cleanup: > + virQEMUSevCapabilitiesFree(capability); > + 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 f4ac8319ac8a..129aab22bf98 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -152,6 +152,9 @@ int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, > int qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, > virGICCapability **capabilities); > > +int qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, > + virSEVCapability **capabilities); > + > int qemuMonitorJSONMigrate(qemuMonitorPtr mon, > unsigned int flags, > const char *uri); > diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies > index c40046beef6b..ace35374ef96 100644 > --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies > +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies > @@ -18995,6 +18995,16 @@ > "id": "libvirt-51" > } > > +{ > + "return" : { > + "reduced-phys-bits": 1, > + "cbitpos": 47, > + "cert-chain": "AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA", > + "pdh": "AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA" > + }, > + "id": "libvirt-52" > +} > + > { > "return": { > }, > diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml > index 3c7dadffcd8a..58a1bf835a73 100644 > --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml > +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml > @@ -204,9 +204,10 @@ > <flag name='screendump_device'/> > <flag name='hda-output'/> > <flag name='blockdev-del'/> > + <flag name='sev-guest'/> > <version>2011090</version> > <kvmVersion>0</kvmVersion> > - <microcodeVersion>390813</microcodeVersion> > + <microcodeVersion>391059</microcodeVersion> > <package>v2.12.0-rc0</package> > <arch>x86_64</arch> > <hostCPU type='kvm' model='base' migratability='yes'> Erik