On Mon, Jun 21, 2021 at 06:04:15AM +0000, Duan, Zhenzhong wrote:
>
>
> > -----Original Message-----
> > From: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > Sent: Friday, June 18, 2021 8:41 PM
> > To: Duan, Zhenzhong <zhenzhong.duan@xxxxxxxxx>
> > Cc: libvir-list@xxxxxxxxxx; Yamahata, Isaku <isaku.yamahata@xxxxxxxxx>;
> > Tian, Jun J <jun.j.tian@xxxxxxxxx>; Qiang, Chenyi <chenyi.qiang@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
> > > ---
> > > 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?
No problem, should have been more explicit about that. I meant the whole
structure and code related to the structure.
> 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.
Right, I missed that part during review. I see that it copies what we do
with AMD SEV where we also have a struct with capabilities.
Currently for Intel TDX there are no extra capabilities and I was not
able to find any patches posted to QEMU devel list which would implement
the QMP command 'query-tdx-capabilities' so right now all of this looks
useless. Unless there is a plan to add some capabilities and introduce
that command we should use boolean value for now.
Pavel
> 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
> > >
Attachment:
signature.asc
Description: PGP signature
