[v1] TDX MMU Part 2

[PATCH 10/21] KVM: TDX: Require TDP MMU and mmio caching for TDX

Posted by Rick Edgecombe 1 year, 3 months ago

From: Isaku Yamahata <isaku.yamahata@intel.com>

Disable TDX support when TDP MMU or mmio caching aren't supported.

As TDP MMU is becoming main stream than the legacy MMU, the legacy MMU
support for TDX isn't implemented.

TDX requires KVM mmio caching. Without mmio caching, KVM will go to MMIO
emulation without installing SPTEs for MMIOs. However, TDX guest is
protected and KVM would meet errors when trying to emulate MMIOs for TDX
guest during instruction decoding. So, TDX guest relies on SPTEs being
installed for MMIOs, which are with no RWX bits and with VE suppress bit
unset, to inject VE to TDX guest. The TDX guest would then issue TDVMCALL
in the VE handler to perform instruction decoding and have host do MMIO
emulation.

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
TDX MMU part 2 v1:
 - Addressed Binbin's comment by massaging Isaku's updated comments and
   adding more explanations about instroducing mmio caching.
 - Addressed Sean's comments of v19 according to Isaku's update but
   kept the warning for MOVDIR64B.
 - Move code change in tdx_hardware_setup() to __tdx_bringup() since the
   former has been removed.
---
 arch/x86/kvm/mmu/mmu.c  | 1 +
 arch/x86/kvm/vmx/main.c | 1 +
 arch/x86/kvm/vmx/tdx.c  | 8 +++-----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 01808cdf8627..d26b235d8f84 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -110,6 +110,7 @@ static bool __ro_after_init tdp_mmu_allowed;
 #ifdef CONFIG_X86_64
 bool __read_mostly tdp_mmu_enabled = true;
 module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0444);
+EXPORT_SYMBOL_GPL(tdp_mmu_enabled);
 #endif
 
 static int max_huge_page_level __read_mostly;
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index c9dfa3aa866c..2cc29d0fc279 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -3,6 +3,7 @@
 
 #include "x86_ops.h"
 #include "vmx.h"
+#include "mmu.h"
 #include "nested.h"
 #include "pmu.h"
 #include "posted_intr.h"
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 25c24901061b..0c08062ef99f 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1474,16 +1474,14 @@ static int __init __tdx_bringup(void)
 	const struct tdx_sys_info_td_conf *td_conf;
 	int r;
 
+	if (!tdp_mmu_enabled || !enable_mmio_caching)
+		return -EOPNOTSUPP;
+
 	if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
 		pr_warn("MOVDIR64B is reqiured for TDX\n");
 		return -EOPNOTSUPP;
 	}
 
-	if (!enable_ept) {
-		pr_err("Cannot enable TDX with EPT disabled.\n");
-		return -EINVAL;
-	}
-
 	/*
 	 * Enabling TDX requires enabling hardware virtualization first,
 	 * as making SEAMCALLs requires CPU being in post-VMXON state.
-- 
2.34.1

Re: [PATCH 10/21] KVM: TDX: Require TDP MMU and mmio caching for TDX

Posted by Huang, Kai 1 year, 3 months ago

On 4/09/2024 3:07 pm, Edgecombe, Rick P wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Disable TDX support when TDP MMU or mmio caching aren't supported.
> 
> As TDP MMU is becoming main stream than the legacy MMU, the legacy MMU
> support for TDX isn't implemented.

Nitpickings:

I suppose we should use imperative mode since this is part of what this 
patch does?

Like:

TDX needs extensive MMU code change to make it work.  As TDP MMU is 
becoming main stream than the legacy MMU, for simplicity only support 
TDX for TDP MMU for now.

> 
> TDX requires KVM mmio caching. Without mmio caching, KVM will go to MMIO
> emulation without installing SPTEs for MMIOs. However, TDX guest is
> protected and KVM would meet errors when trying to emulate MMIOs for TDX
> guest during instruction decoding. So, TDX guest relies on SPTEs being
> installed for MMIOs, which are with no RWX bits and with VE suppress bit
> unset, to inject VE to TDX guest. The TDX guest would then issue TDVMCALL
> in the VE handler to perform instruction decoding and have host do MMIO
> emulation.

AFAICT the above two paragraphs are talking about two different things 
that one thing doens't have hard dependency to the other.

Should we separate this into two patches:  one patch to change 'checking 
enable_ept' to 'checking tdp_mmu_enabled' (which justifies the first 
paragraph), and the other to add MMIO caching checking.

The final code after the two patches could still end up with ...

[...]

> +	if (!tdp_mmu_enabled || !enable_mmio_caching)
> +		return -EOPNOTSUPP;
> +

... this though.

But feel free to ignore (since nitpickings).

Re: [PATCH 10/21] KVM: TDX: Require TDP MMU and mmio caching for TDX

Posted by Paolo Bonzini 1 year, 3 months ago

On 9/4/24 05:07, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Disable TDX support when TDP MMU or mmio caching aren't supported.
> 
> As TDP MMU is becoming main stream than the legacy MMU, the legacy MMU
> support for TDX isn't implemented.
> 
> TDX requires KVM mmio caching. Without mmio caching, KVM will go to MMIO
> emulation without installing SPTEs for MMIOs. However, TDX guest is
> protected and KVM would meet errors when trying to emulate MMIOs for TDX
> guest during instruction decoding. So, TDX guest relies on SPTEs being
> installed for MMIOs, which are with no RWX bits and with VE suppress bit
> unset, to inject VE to TDX guest. The TDX guest would then issue TDVMCALL
> in the VE handler to perform instruction decoding and have host do MMIO
> emulation.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> TDX MMU part 2 v1:
>   - Addressed Binbin's comment by massaging Isaku's updated comments and
>     adding more explanations about instroducing mmio caching.
>   - Addressed Sean's comments of v19 according to Isaku's update but
>     kept the warning for MOVDIR64B.
>   - Move code change in tdx_hardware_setup() to __tdx_bringup() since the
>     former has been removed.
> ---
>   arch/x86/kvm/mmu/mmu.c  | 1 +
>   arch/x86/kvm/vmx/main.c | 1 +
>   arch/x86/kvm/vmx/tdx.c  | 8 +++-----
>   3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 01808cdf8627..d26b235d8f84 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -110,6 +110,7 @@ static bool __ro_after_init tdp_mmu_allowed;
>   #ifdef CONFIG_X86_64
>   bool __read_mostly tdp_mmu_enabled = true;
>   module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0444);
> +EXPORT_SYMBOL_GPL(tdp_mmu_enabled);
>   #endif
>   
>   static int max_huge_page_level __read_mostly;
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index c9dfa3aa866c..2cc29d0fc279 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -3,6 +3,7 @@
>   
>   #include "x86_ops.h"
>   #include "vmx.h"
> +#include "mmu.h"
>   #include "nested.h"
>   #include "pmu.h"
>   #include "posted_intr.h"
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 25c24901061b..0c08062ef99f 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1474,16 +1474,14 @@ static int __init __tdx_bringup(void)
>   	const struct tdx_sys_info_td_conf *td_conf;
>   	int r;
>   
> +	if (!tdp_mmu_enabled || !enable_mmio_caching)
> +		return -EOPNOTSUPP;
> +
>   	if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
>   		pr_warn("MOVDIR64B is reqiured for TDX\n");
>   		return -EOPNOTSUPP;
>   	}
>   
> -	if (!enable_ept) {
> -		pr_err("Cannot enable TDX with EPT disabled.\n");
> -		return -EINVAL;
> -	}
> -
>   	/*
>   	 * Enabling TDX requires enabling hardware virtualization first,
>   	 * as making SEAMCALLs requires CPU being in post-VMXON state.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>