Re: [RFC PATCH 1/7] qemu: provide support to query the TDX capabilities

> -----Original Message----- > From: Pavel Hrdina <phrdina(a)redhat.com&gt; > Sent: Friday, June 18, 2021 8:41 PM > To: Duan, Zhenzhong <zhenzhong.duan(a)intel.com&gt; > Cc: libvir-list(a)redhat.com; Yamahata, Isaku <isaku.yamahata(a)intel.com>; > Tian, Jun J <jun.j.tian(a)intel.com>; Qiang, Chenyi <chenyi.qiang(a)intel.com&gt; > Subject: Re: [RFC PATCH 1/7] qemu: provide support to query the TDX > capabilities > > On Fri, Jun 18, 2021 at 04:50:46PM +0800, Zhenzhong Duan wrote: > > QEMU provides support for launching an encrypted VMs on Intel x86 > > platform using Trust Domain Extension (TDX) feature. This patch adds > > support to query the TDX capabilities from the QEMU. > > > > Currently there is no elements in TDX capabilities except a placeholder. > > > > Signed-off-by: Chenyi Qiang <chenyi.qiang(a)intel.com&gt; > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan(a)intel.com&gt; > > --- > > src/conf/domain_capabilities.c | 8 +++++ > > src/conf/domain_capabilities.h | 10 +++++++ > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_capabilities.c | 30 +++++++++++++++++++ > > src/qemu/qemu_capabilities.h | 1 + > > src/qemu/qemu_monitor.c | 8 +++++ > > src/qemu/qemu_monitor.h | 3 ++ > > src/qemu/qemu_monitor_json.c | 53 > ++++++++++++++++++++++++++++++++++ > > src/qemu/qemu_monitor_json.h | 3 ++ > > 9 files changed, 117 insertions(+) > > > > diff --git a/src/conf/domain_capabilities.c > > b/src/conf/domain_capabilities.c index cb90ae0176..31577095e9 100644 > > --- a/src/conf/domain_capabilities.c > > +++ b/src/conf/domain_capabilities.c > > @@ -76,6 +76,14 @@ virSEVCapabilitiesFree(virSEVCapability *cap) > > g_free(cap); > > } > > > > +void > > +virTDXCapabilitiesFree(virTDXCapability *cap) { > > + if (!cap) > > + return; > > + > > + VIR_FREE(cap); > > +} > > > > static void > > virDomainCapsDispose(void *obj) > > diff --git a/src/conf/domain_capabilities.h > > b/src/conf/domain_capabilities.h index b6433b20c9..e099788da9 100644 > > --- a/src/conf/domain_capabilities.h > > +++ b/src/conf/domain_capabilities.h > > @@ -173,6 +173,12 @@ struct _virSEVCapability { > > unsigned int reduced_phys_bits; > > }; > > > > +typedef struct _virTDXCapability virTDXCapability; struct > > +_virTDXCapability { > > + /* no elements for Intel TDX for now, just put a placeholder */ > > + uint64_t placeholder; > > +}; > > + > > There is no need to add unused code into libvirt especially if the impact is > only internal and it is not exposed to users. It can be added in the future if > needed. Because this patch series doesn't add any TDX specific capability into > this structure please drop all related code to this placeholder. Sorry, I didn't fully understand which part to preserve. Do you mean removing placeholder from struct virTDXCapability or removing virTDXCapability as it's empty?

virDomainCapsFeatureTDXFormat(virTDXCapability *tdx) shows '<tdx supported='yes'/>' based on the value of tdx. If virTDXCapability is empty, tdx is always NULL and '<tdx supported='no'/>' showed.

Thanks Zhenzhong > > Pavel > > > typedef enum { > > VIR_DOMAIN_CAPS_FEATURE_IOTHREADS = 0, > > VIR_DOMAIN_CAPS_FEATURE_VMCOREINFO, > > @@ -254,3 +260,7 @@ void > > virSEVCapabilitiesFree(virSEVCapability *capabilities); > > > > G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSEVCapability, > > virSEVCapabilitiesFree); > > + > > +void virTDXCapabilitiesFree(virTDXCapability *capabilities); > > + > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTDXCapability, > > +virTDXCapabilitiesFree); > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index > > 2efa787664..8cbb60b577 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -218,6 +218,7 @@ virDomainCapsEnumSet; virDomainCapsFormat; > > virDomainCapsNew; virSEVCapabilitiesFree; > > +virTDXCapabilitiesFree; > > > > > > # conf/domain_conf.h > > diff --git a/src/qemu/qemu_capabilities.c > > b/src/qemu/qemu_capabilities.c index 059d6badf2..a143e453f4 100644 > > --- a/src/qemu/qemu_capabilities.c > > +++ b/src/qemu/qemu_capabilities.c > > @@ -636,6 +636,7 @@ VIR_ENUM_IMPL(virQEMUCaps, > > /* 405 */ > > "confidential-guest-support", > > "query-display-options", > > + "tdx-guest", > > ); > > > > > > @@ -716,6 +717,8 @@ struct _virQEMUCaps { > > > > virSEVCapability *sevCapabilities; > > > > + virTDXCapability *tdxCapabilities; > > + > > /* Capabilities which may differ depending on the accelerator. */ > > virQEMUCapsAccel kvm; > > virQEMUCapsAccel tcg; > > @@ -1354,6 +1357,7 @@ struct virQEMUCapsStringFlags > virQEMUCapsObjectTypes[] = { > > { "input-linux", QEMU_CAPS_INPUT_LINUX }, > > { "virtio-gpu-gl-pci", QEMU_CAPS_VIRTIO_GPU_GL_PCI }, > > { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL }, > > + { "tdx-guest", QEMU_CAPS_TDX_GUEST}, > > }; > > > > > > @@ -2027,6 +2031,7 @@ void virQEMUCapsDispose(void *obj) > > g_free(qemuCaps->gicCapabilities); > > > > virSEVCapabilitiesFree(qemuCaps->sevCapabilities); > > + virTDXCapabilitiesFree(qemuCaps->tdxCapabilities); > > > > virQEMUCapsAccelClear(&qemuCaps->kvm); > > virQEMUCapsAccelClear(&qemuCaps->tcg); > > @@ -3354,6 +3359,29 @@ > virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps *qemuCaps, > > return 0; > > } > > > > +static int > > +virQEMUCapsProbeQMPTDXCapabilities(virQEMUCaps *qemuCaps, > > + qemuMonitor *mon) { > > + int rc = -1; > > + virTDXCapability *caps = NULL; > > + > > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST)) > > + return 0; > > + > > + if ((rc = qemuMonitorGetTDXCapabilities(mon, &caps)) < 0) > > + return -1; > > + > > + /* TDX isn't actually supported */ > > + if (rc == 0) { > > + virQEMUCapsClear(qemuCaps, QEMU_CAPS_TDX_GUEST); > > + return 0; > > + } > > + > > + virTDXCapabilitiesFree(qemuCaps->tdxCapabilities); > > + qemuCaps->tdxCapabilities = caps; > > + return 0; > > +} > > > > /* > > * Filter for features which should never be passed to QEMU. Either > > because @@ -5316,6 +5344,8 @@ > virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps, > > return -1; > > if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) > > return -1; > > + if (virQEMUCapsProbeQMPTDXCapabilities(qemuCaps, mon) < 0) > > + return -1; > > > > virQEMUCapsInitProcessCaps(qemuCaps); > > > > diff --git a/src/qemu/qemu_capabilities.h > > b/src/qemu/qemu_capabilities.h index b2878312ac..a51bd9a256 100644 > > --- a/src/qemu/qemu_capabilities.h > > +++ b/src/qemu/qemu_capabilities.h > > @@ -616,6 +616,7 @@ typedef enum { /* virQEMUCapsFlags grouping > marker for syntax-check */ > > /* 405 */ > > QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT, /* -machine > confidential-guest-support */ > > QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp > > command present */ > > + QEMU_CAPS_TDX_GUEST, /* -object tdx-guest,... */ > > > > QEMU_CAPS_LAST /* this must always be the last item */ } > > virQEMUCapsFlags; diff --git a/src/qemu/qemu_monitor.c > > b/src/qemu/qemu_monitor.c index 8f35b4240f..f2a3badeec 100644 > > --- a/src/qemu/qemu_monitor.c > > +++ b/src/qemu/qemu_monitor.c > > @@ -3946,6 +3946,14 @@ qemuMonitorNBDServerStart(qemuMonitor > *mon, > > return qemuMonitorJSONNBDServerStart(mon, server, tls_alias); } > > > > +int > > +qemuMonitorGetTDXCapabilities(qemuMonitor *mon, > > + virTDXCapability **capabilities) { > > + QEMU_CHECK_MONITOR(mon); > > + > > + return qemuMonitorJSONGetTDXCapabilities(mon, capabilities); } > > > > int > > qemuMonitorNBDServerAdd(qemuMonitor *mon, diff --git > > a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index > > 6a25def78b..48c18c5220 100644 > > --- a/src/qemu/qemu_monitor.h > > +++ b/src/qemu/qemu_monitor.h > > @@ -859,6 +859,9 @@ int qemuMonitorGetGICCapabilities(qemuMonitor > > *mon, int qemuMonitorGetSEVCapabilities(qemuMonitor *mon, > > virSEVCapability **capabilities); > > > > +int qemuMonitorGetTDXCapabilities(qemuMonitor *mon, > > + virTDXCapability **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 > > 223777739d..c58152e86f 100644 > > --- a/src/qemu/qemu_monitor_json.c > > +++ b/src/qemu/qemu_monitor_json.c > > @@ -7028,6 +7028,59 @@ > qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon, > > return ret; > > } > > > > +/** > > + * qemuMonitorJSONGetTDXCapabilities: > > + * @mon: qemu monitor object > > + * @capabilities: pointer to pointer to a TDX capability structure to > > +be filled > > + * > > + * Returns -1 on error, 0 if TDX is not supported, and 1 if TDX is > > +supported on > > + * the platform. > > + */ > > +int > > +qemuMonitorJSONGetTDXCapabilities(qemuMonitor *mon, > > + virTDXCapability **capabilities) { > > + int ret = -1; > > + virJSONValue *cmd; > > + virJSONValue *reply = NULL; > > + g_autoptr(virTDXCapability) capability = NULL; > > + > > + *capabilities = NULL; > > + > > + if (!(cmd = qemuMonitorJSONMakeCommand("query-tdx-capabilities", > > + NULL))) > > + return -1; > > + > > + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > > + goto cleanup; > > + > > + /* QEMU has only compiled-in support of TDX */ > > + if (qemuMonitorJSONHasError(reply, "GenericError")) { > > + ret = 0; > > + goto cleanup; > > + } > > + > > + if (qemuMonitorJSONCheckError(cmd, reply) < 0) > > + goto cleanup; > > + > > + if (!virJSONValueObjectGetObject(reply, "return")) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > + _("query-tdx-capabilities reply was missing return")); > > + return -1; > > + } > > + > > + capability = g_new0(virTDXCapability, 1); > > + > > + *capabilities = g_steal_pointer(&capability); > > + > > + ret = 1; > > + cleanup: > > + virJSONValueFree(cmd); > > + virJSONValueFree(reply); > > + > > + return ret; > > +} > > + > > static virJSONValue * > > qemuMonitorJSONBuildInetSocketAddress(const char *host, > > const char *port) diff --git > > a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index > > 01a3ba25f1..3fe6a34c4c 100644 > > --- a/src/qemu/qemu_monitor_json.h > > +++ b/src/qemu/qemu_monitor_json.h > > @@ -157,6 +157,9 @@ int > qemuMonitorJSONGetGICCapabilities(qemuMonitor > > *mon, int qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon, > > virSEVCapability > > **capabilities); > > > > +int qemuMonitorJSONGetTDXCapabilities(qemuMonitor *mon, > > + virTDXCapability > > +**capabilities); > > + > > int qemuMonitorJSONMigrate(qemuMonitor *mon, > > unsigned int flags, > > const char *uri); > > -- > > 2.25.1 > >