Re: [PATCH rfcv3 03/11] conf: expose TDX feature in domain capabilities

>-----Original Message----- >From: Daniel P. Berrangé <berrange(a)redhat.com&gt; >Subject: Re: [PATCH rfcv3 03/11] conf: expose TDX feature in domain >capabilities > >On Mon, Nov 27, 2023 at 04:55:13PM +0800, Zhenzhong Duan wrote: >> Extend qemu TDX capability to domain capabilities. >> >> Signed-off-by: Chenyi Qiang <chenyi.qiang(a)intel.com&gt; >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan(a)intel.com&gt; >> --- >> docs/formatdomaincaps.rst | 1 + >> src/conf/domain_capabilities.c | 1 + >> src/conf/domain_capabilities.h | 1 + >> src/conf/schemas/domaincaps.rng | 9 +++++++++ >> src/qemu/qemu_capabilities.c | 15 +++++++++++++++ >> 5 files changed, 27 insertions(+) >> >> diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst >> index ef752a0f3a..3acc9a12b4 100644 >> --- a/docs/formatdomaincaps.rst >> +++ b/docs/formatdomaincaps.rst >> @@ -669,6 +669,7 @@ capabilities. All features occur as children of the >main ``features`` element. >> <value>vapic</value> >> </enum> >> </hyperv> >> + <tdx supported='yes'/> >> </features> >> </domainCapabilities> >> >> diff --git a/src/conf/domain_capabilities.c >b/src/conf/domain_capabilities.c >> index f6e09dc584..0f9ddb1e48 100644 >> --- a/src/conf/domain_capabilities.c >> +++ b/src/conf/domain_capabilities.c >> @@ -42,6 +42,7 @@ VIR_ENUM_IMPL(virDomainCapsFeature, >> "backup", >> "async-teardown", >> "s390-pv", >> + "tdx", >> ); >> >> static virClass *virDomainCapsClass; >> diff --git a/src/conf/domain_capabilities.h >b/src/conf/domain_capabilities.h >> index 01bcfa2e39..cc44cf2363 100644 >> --- a/src/conf/domain_capabilities.h >> +++ b/src/conf/domain_capabilities.h >> @@ -250,6 +250,7 @@ typedef enum { >> VIR_DOMAIN_CAPS_FEATURE_BACKUP, >> VIR_DOMAIN_CAPS_FEATURE_ASYNC_TEARDOWN, >> VIR_DOMAIN_CAPS_FEATURE_S390_PV, >> + VIR_DOMAIN_CAPS_FEATURE_TDX, >> >> VIR_DOMAIN_CAPS_FEATURE_LAST >> } virDomainCapsFeature; >> diff --git a/src/conf/schemas/domaincaps.rng >b/src/conf/schemas/domaincaps.rng >> index e7aa4a1066..a5522b1e67 100644 >> --- a/src/conf/schemas/domaincaps.rng >> +++ b/src/conf/schemas/domaincaps.rng >> @@ -308,6 +308,9 @@ >> <optional> >> <ref name="s390-pv"/> >> </optional> >> + <optional> >> + <ref name="tdx"/> >> + </optional> >> <optional> >> <ref name="sev"/> >> </optional> >> @@ -363,6 +366,12 @@ >> </element> >> </define> >> >> + <define name="tdx"> >> + <element name="tdx"> >> + <ref name="supported"/> >> + </element> >> + </define> >> + >> <define name="sev"> >> <element name="sev"> >> <ref name="supported"/> >> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c >> index 8764df5e9d..0b4988256f 100644 >> --- a/src/qemu/qemu_capabilities.c >> +++ b/src/qemu/qemu_capabilities.c >> @@ -6657,6 +6657,20 @@ >virQEMUCapsFillDomainFeatureHypervCaps(virQEMUCaps *qemuCaps, >> } >> >> >> +static void >> +virQEMUCapsFillDomainFeatureTDXCaps(virQEMUCaps *qemuCaps, >> + virDomainCaps *domCaps) >> +{ >> + if (domCaps->arch == VIR_ARCH_X86_64 && >> + domCaps->virttype == VIR_DOMAIN_VIRT_KVM && >> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST) && >> + virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps) && >> + virQEMUCapsGet(qemuCaps, >QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) && > >Checking QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT is >overkill, as we >can assume that is implied to exist by virtue of QEMU_CAPS_TDX_GUEST >existing. Good catch, will remove it. > >> + (STREQ(domCaps->machine, "q35") || STRPREFIX(domCaps- >>machine, "pc-q35-"))) > >If QEMU has limited its support for TDX to just q35, then it would be >much better if QEMU patches for TDX provided a way to detect this via >QMP, so we don't need to do these string comparisons. IIUC, QEMU has no limitation on using i440FX with TDX. We had this check in libvirt as we thought i440FX is deprecated.

@Yamahata, Isaku, @Li, Xiaoyao, so now we have two choices: 1. remove this limitation in libvirt

2. add QMP on QEMU side to report this limitation to libvirt Which choice do you like?