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

On Mon, Feb 26, 2018 at 11:53:33 -0600, 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; > --- > > QEMU SEV v9 patch does not have implementation of query-sev-capabilities command > and I am will be adding this command in next QEMU patch round. Command result > will look like this: > > { "execute": "query-sev-capabilities" } > { "return": { "sev": 1, "pdh": "....", "cert-chain": "...", > "cbitpos": 47, "reduced-phys-bits": 5}} This patch fails both 'make check' and 'make syntax-check' please make sure that for the next posting you send a series where every single patch passes both test suites.

> > src/conf/domain_capabilities.h | 14 +++++++ > src/qemu/qemu_capabilities.c | 28 +++++++++++++ > src/qemu/qemu_capspriv.h | 4 ++ > src/qemu/qemu_monitor.c | 9 +++++ > src/qemu/qemu_monitor.h | 3 ++ > src/qemu/qemu_monitor_json.c | 92 ++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor_json.h | 3 ++ > 7 files changed, 153 insertions(+) > > diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h > index fa4c1e442f57..e13a7fd6ba1b 100644 > --- a/src/conf/domain_capabilities.h > +++ b/src/conf/domain_capabilities.h > @@ -137,6 +137,20 @@ struct _virDomainCapsCPU { > virDomainCapsCPUModelsPtr custom; > }; > > +/* > + * SEV capabilities > + */ > +typedef struct _virSEVCapability virSEVCapability; > +typedef virSEVCapability *virSEVCapabilityPtr; > +struct _virSEVCapability { > + bool sev; > + char *pdh; > + char *cert_chain; > + int cbitpos; > + int reduced_phys_bits; > +}; > + > + > struct _virDomainCaps { > virObjectLockable parent; > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index b5eb8cf46a52..2c680528deb8 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -525,6 +525,8 @@ struct _virQEMUCaps { > size_t ngicCapabilities; > virGICCapability *gicCapabilities; > > + virSEVCapability *sevCapabilities; > + > virQEMUCapsHostCPUData kvmCPU; > virQEMUCapsHostCPUData tcgCPU; > }; > @@ -2811,6 +2813,14 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, > qemuCaps->ngicCapabilities = ncapabilities; > } > > +void > +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, > + virSEVCapability *capabilities) > +{ > + VIR_FREE(qemuCaps->sevCapabilities); This structure contains also some pointers which would be leaked.

> + > + qemuCaps->sevCapabilities = capabilities; > +} > > static int > virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps, > @@ -3318,6 +3328,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, > @@ -4951,6 +4974,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, > virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) > virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_CACHE); > > + /* SEV capabilities */ > + if (ARCH_IS_X86(qemuCaps->arch)) { > + virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon); > + } I think you can add a capability bit for the presence of the query-sev-capabilities command and call this function based on this fact.

> + > ret = 0; > cleanup: > return ret; > 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 ad5c572aeefb..195248c88ae1 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 954ae88e4f64..1b2513650c58 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -755,6 +755,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 a09e93e464b3..4424abfa7148 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -6362,6 +6362,98 @@ 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; > + bool sev; > + 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 the 'query-sev-capabilities' QMP command was not available > + * we simply successfully return zero capabilities. > + * This is the case for QEMU <2.12 */ > + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { > + ret = 0; > + goto cleanup; Making this whole function depend on the presence of the new command will greatly simplify your pain with the qemucapabilitiestest which this will inflict. If you make sure that only new qemus call this command you won't have to fix all older test suites. Also note that you'll need to add a test case for the new qemu with the command supported so we can test this capability detection.

> + } > + > + if (qemuMonitorJSONCheckError(cmd, reply) < 0) > + goto cleanup; > + > + caps = virJSONValueObjectGetObject(reply, "return"); > + > + if (virJSONValueObjectGetBoolean(caps, "sev", &sev) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("'sev' field is missing")); > + goto cleanup; > + } > + > + if (!sev) { > + goto cleanup; > + } > + > + 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_N(capability, 1) < 0) 'VIR_ALLOC' should be enough

> + goto cleanup; > + > + if (VIR_STRDUP(capability->pdh, pdh) < 0) > + goto cleanup; > + > + if (VIR_STRDUP(capability->cert_chain, cert_chain) < 0) > + goto cleanup; > + > + capability->sev = true; This field should not be necessary. The presence of the structure itself should be good enough in this case.

> + 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); > -- > 2.14.3 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list