[v19,069/130] KVM: TDX: Require TDP MMU and mmio caching for TDX

Commit Message

Isaku Yamahata Feb. 26, 2024, 8:26 a.m. UTC

From: Isaku Yamahata <[e-mail]>

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.  Disable
TDX support when TDP MMU or mmio caching aren't supported.

Signed-off-by: Isaku Yamahata <[e-mail]>
---
 arch/x86/kvm/mmu/mmu.c  |  1 +
 arch/x86/kvm/vmx/main.c | 13 +++++++++++++
 2 files changed, 14 insertions(+)

Comments

Binbin Wu March 28, 2024, 5:24 a.m. UTC | #1

On 2/26/2024 4:26 PM, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
>
> 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.

Can you add some description about why TDX requires mmio caching in the 
changelog?


>   Disable
> TDX support when TDP MMU or mmio caching aren't supported.
>
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>   arch/x86/kvm/mmu/mmu.c  |  1 +
>   arch/x86/kvm/vmx/main.c | 13 +++++++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 0e0321ad9ca2..b8d6ce02e66d 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -104,6 +104,7 @@ module_param_named(flush_on_reuse, force_flush_and_sync_on_reuse, bool, 0644);
>    * If the hardware supports that we don't need to do shadow paging.
>    */
>   bool tdp_enabled = false;
> +EXPORT_SYMBOL_GPL(tdp_enabled);
>   
>   static bool __ro_after_init tdp_mmu_allowed;
>   
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 076a471d9aea..54df6653193e 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 "tdx.h"
> @@ -36,6 +37,18 @@ static __init int vt_hardware_setup(void)
>   	if (ret)
>   		return ret;
>   
> +	/* TDX requires KVM TDP MMU. */
> +	if (enable_tdx && !tdp_enabled) {
> +		enable_tdx = false;
> +		pr_warn_ratelimited("TDX requires TDP MMU.  Please enable TDP MMU for TDX.\n");
> +	}
> +
> +	/* TDX requires MMIO caching. */
> +	if (enable_tdx && !enable_mmio_caching) {
> +		enable_tdx = false;
> +		pr_warn_ratelimited("TDX requires mmio caching.  Please enable mmio caching for TDX.\n");
> +	}
> +
>   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
>   	if (enable_tdx)
>   		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,

Isaku Yamahata March 28, 2024, 9:03 p.m. UTC | #2

On Thu, Mar 28, 2024 at 01:24:27PM +0800,
Binbin Wu <[e-mail]> wrote:

> 
> 
> On 2/26/2024 4:26 PM, [e-mail] wrote:
> > From: Isaku Yamahata <[e-mail]>
> > 
> > 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.
> 
> Can you add some description about why TDX requires mmio caching in the
> changelog?

Sure, will update the commit log.

As the TDX guest is protected, the guest has to issue TDG.VP.VMCALL<MMIO> on
VE.  The VMM has to setup Shared-EPT entry to inject VE by setting the entry
value with VE suppress bit cleared.

KVM mmio caching is a feature to set the EPT entry to special value for MMIO GFN
instead of the default value with suppress VE bit set.  So TDX KVM wants to
utilize it.

Thanks,

Sean Christopherson April 1, 2024, 5:34 p.m. UTC | #3

On Mon, Feb 26, 2024, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
> 
> 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.  Disable
> TDX support when TDP MMU or mmio caching aren't supported.
> 
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>  arch/x86/kvm/mmu/mmu.c  |  1 +
>  arch/x86/kvm/vmx/main.c | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 0e0321ad9ca2..b8d6ce02e66d 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -104,6 +104,7 @@ module_param_named(flush_on_reuse, force_flush_and_sync_on_reuse, bool, 0644);
>   * If the hardware supports that we don't need to do shadow paging.
>   */
>  bool tdp_enabled = false;
> +EXPORT_SYMBOL_GPL(tdp_enabled);

I haven't looked at the rest of the series, but this should be unnecessary.  Just
use enable_ept.  Ah, the code is wrong.

>  static bool __ro_after_init tdp_mmu_allowed;
>  
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 076a471d9aea..54df6653193e 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 "tdx.h"
> @@ -36,6 +37,18 @@ static __init int vt_hardware_setup(void)
>  	if (ret)
>  		return ret;
>  
> +	/* TDX requires KVM TDP MMU. */

This is a useless comment (assuming the code is correctly written), it's quite
obvious from:

	if (!tdp_mmu_enabled)
		enable_tdx = false;

that the TDP MMU is required.  Explaining *why* could be useful, but I'd probably
just omit a comment.  In the not too distant future, it will likely be common
knowledge that the shadow MMU doesn't support newfangled features, and it _should_
be very easy for someone to get the info from the changelog.

> +	if (enable_tdx && !tdp_enabled) {

tdp_enabled can be true without the TDP MMU, you want tdp_mmu_enabled.

> +		enable_tdx = false;
> +		pr_warn_ratelimited("TDX requires TDP MMU.  Please enable TDP MMU for TDX.\n");

Drop the pr_warn(), TDX will presumably be on by default at some point, I don't
want to get spam every time I test with TDP disabled.

Also, ratelimiting this code is pointless (as is _once()), it should only ever
be called once per module module, and the limiting/once protections are tied to
the module, i.e. effectively get reset when a module is reloaded.

> +	}
> +
> +	/* TDX requires MMIO caching. */
> +	if (enable_tdx && !enable_mmio_caching) {
> +		enable_tdx = false;
> +		pr_warn_ratelimited("TDX requires mmio caching.  Please enable mmio caching for TDX.\n");

Same comments here.

> +	}
> +
>  	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);

All of the above code belongs in tdx_hardware_setup(), especially since you need
tdp_mmu_enabled, not enable_ept.

>  	if (enable_tdx)
>  		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> -- 
> 2.25.1
>

Isaku Yamahata April 2, 2024, 6:03 a.m. UTC | #4

On Mon, Apr 01, 2024 at 10:34:05AM -0700,
Sean Christopherson <[e-mail]> wrote:

> On Mon, Feb 26, 2024, [e-mail] wrote:
> > From: Isaku Yamahata <[e-mail]>
> > 
> > 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.  Disable
> > TDX support when TDP MMU or mmio caching aren't supported.
> > 
> > Signed-off-by: Isaku Yamahata <[e-mail]>
> > ---
> >  arch/x86/kvm/mmu/mmu.c  |  1 +
> >  arch/x86/kvm/vmx/main.c | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 0e0321ad9ca2..b8d6ce02e66d 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -104,6 +104,7 @@ module_param_named(flush_on_reuse, force_flush_and_sync_on_reuse, bool, 0644);
> >   * If the hardware supports that we don't need to do shadow paging.
> >   */
> >  bool tdp_enabled = false;
> > +EXPORT_SYMBOL_GPL(tdp_enabled);
> 
> I haven't looked at the rest of the series, but this should be unnecessary.  Just
> use enable_ept.  Ah, the code is wrong.
> 
> >  static bool __ro_after_init tdp_mmu_allowed;
> >  
> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 076a471d9aea..54df6653193e 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 "tdx.h"
> > @@ -36,6 +37,18 @@ static __init int vt_hardware_setup(void)
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* TDX requires KVM TDP MMU. */
> 
> This is a useless comment (assuming the code is correctly written), it's quite
> obvious from:
> 
> 	if (!tdp_mmu_enabled)
> 		enable_tdx = false;
> 
> that the TDP MMU is required.  Explaining *why* could be useful, but I'd probably
> just omit a comment.  In the not too distant future, it will likely be common
> knowledge that the shadow MMU doesn't support newfangled features, and it _should_
> be very easy for someone to get the info from the changelog.
> 
> > +	if (enable_tdx && !tdp_enabled) {
> 
> tdp_enabled can be true without the TDP MMU, you want tdp_mmu_enabled.
> 
> > +		enable_tdx = false;
> > +		pr_warn_ratelimited("TDX requires TDP MMU.  Please enable TDP MMU for TDX.\n");
> 
> Drop the pr_warn(), TDX will presumably be on by default at some point, I don't
> want to get spam every time I test with TDP disabled.
> 
> Also, ratelimiting this code is pointless (as is _once()), it should only ever
> be called once per module module, and the limiting/once protections are tied to
> the module, i.e. effectively get reset when a module is reloaded.
> 
> > +	}
> > +
> > +	/* TDX requires MMIO caching. */
> > +	if (enable_tdx && !enable_mmio_caching) {
> > +		enable_tdx = false;
> > +		pr_warn_ratelimited("TDX requires mmio caching.  Please enable mmio caching for TDX.\n");
> 
> Same comments here.
> 
> > +	}
> > +
> >  	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> 
> All of the above code belongs in tdx_hardware_setup(), especially since you need
> tdp_mmu_enabled, not enable_ept.

Thanks for review.  With tdp_mmu_enabled, removing warning and comments,
I come up with the followings.

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index aa66b0510062..ba2738cc6e98 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/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 976cf5e37f0f..e6f66f7c04bb 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1253,14 +1253,9 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
 	int r = 0;
 	int i;
 
-	if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
-		pr_warn("MOVDIR64B is reqiured for TDX\n");
+	if (!tdp_mmu_enabled || !enable_mmio_caching ||
+	    !cpu_feature_enabled(X86_FEATURE_MOVDIR64B))
 		return -EOPNOTSUPP;
-	}
-	if (!enable_ept) {
-		pr_warn("Cannot enable TDX with EPT disabled\n");
-		return -EINVAL;
-	}
 
 	max_pkgs = topology_max_packages();
 	tdx_mng_key_config_lock = kcalloc(max_pkgs, sizeof(*tdx_mng_key_config_lock),

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 0e0321ad9ca2..b8d6ce02e66d 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -104,6 +104,7 @@  module_param_named(flush_on_reuse, force_flush_and_sync_on_reuse, bool, 0644);
  * If the hardware supports that we don't need to do shadow paging.
  */
 bool tdp_enabled = false;
+EXPORT_SYMBOL_GPL(tdp_enabled);
 
 static bool __ro_after_init tdp_mmu_allowed;
 
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 076a471d9aea..54df6653193e 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 "tdx.h"
@@ -36,6 +37,18 @@  static __init int vt_hardware_setup(void)
 	if (ret)
 		return ret;
 
+	/* TDX requires KVM TDP MMU. */
+	if (enable_tdx && !tdp_enabled) {
+		enable_tdx = false;
+		pr_warn_ratelimited("TDX requires TDP MMU.  Please enable TDP MMU for TDX.\n");
+	}
+
+	/* TDX requires MMIO caching. */
+	if (enable_tdx && !enable_mmio_caching) {
+		enable_tdx = false;
+		pr_warn_ratelimited("TDX requires mmio caching.  Please enable mmio caching for TDX.\n");
+	}
+
 	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
 	if (enable_tdx)
 		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,