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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux