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 Thu, Jan 11, 2024 at 03:43:34AM +0000, Duan, Zhenzhong wrote: > > > >-----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. Libvirt has not deprecated i440FX, and neither has QEMU upstream. RHEL, as a downstream, though has deprecated it. > @Yamahata, Isaku, @Li, Xiaoyao, so now we have two choices: > 1. remove this limitation in libvirt Just remove this limitation in libvirt, since RHEL support limitations are not relevant to the code.