Re: [PATCH v3 13/17] KVM: x86/tdp_mmu: Support mirror root for TDP MMU

[Yan Zhao]

On Thu, Jul 04, 2024 at 03:40:35AM +0800, Edgecombe, Rick P wrote:
> On Wed, 2024-06-26 at 13:05 +0800, Yan Zhao wrote:
> > But we still need the reload path to have each vcpu to hold a ref count of the
> > mirror root.
> > What about below fix?
> > 
> > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> > index 026e8edfb0bd..4decd13457ec 100644
> > --- a/arch/x86/kvm/mmu.h
> > +++ b/arch/x86/kvm/mmu.h
> > @@ -127,9 +127,28 @@ void kvm_mmu_sync_prev_roots(struct kvm_vcpu *vcpu);
> >  void kvm_mmu_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new,
> >                          int bytes);
> > 
> > +static inline bool kvm_has_mirrored_tdp(const struct kvm *kvm)
> > +{
> > +       return kvm->arch.vm_type == KVM_X86_TDX_VM;
> > +}
> > +
> > +static inline bool kvm_mmu_root_hpa_is_invalid(struct kvm_vcpu *vcpu)
> > +{
> > +       return vcpu->arch.mmu->root.hpa == INVALID_PAGE;
> > +}
> > +
> > +static inline bool kvm_mmu_mirror_root_hpa_is_invalid(struct kvm_vcpu *vcpu)
> > +{
> > +       if (!kvm_has_mirrored_tdp(vcpu->kvm))
> > +               return false;
> > +
> > +       return vcpu->arch.mmu->mirror_root_hpa == INVALID_PAGE;
> > +}
> > +
> >  static inline int kvm_mmu_reload(struct kvm_vcpu *vcpu)
> >  {
> > -       if (likely(vcpu->arch.mmu->root.hpa != INVALID_PAGE))
> > +       if (!kvm_mmu_root_hpa_is_invalid(vcpu) &&
> > +           !kvm_mmu_mirror_root_hpa_is_invalid(vcpu))
> 
> If we go this way, we should keep the likely. But, I'm not convinced.
> 
> >                 return 0;
> > 
> >         return kvm_mmu_load(vcpu);
> > @@ -322,11 +341,6 @@ static inline gpa_t kvm_translate_gpa(struct kvm_vcpu
> > *vcpu,
> >         return translate_nested_gpa(vcpu, gpa, access, exception);
> >  }
> > 
> > -static inline bool kvm_has_mirrored_tdp(const struct kvm *kvm)
> > -{
> > -       return kvm->arch.vm_type == KVM_X86_TDX_VM;
> > -}
> > -
> >  static inline gfn_t kvm_gfn_direct_bits(const struct kvm *kvm)
> >  {
> >         return kvm->arch.gfn_direct_bits;
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index e1299eb03e63..5e7d92074f70 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -3702,9 +3702,11 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu
> > *vcpu)
> >         int r;
> > 
> >         if (tdp_mmu_enabled) {
> > -               if (kvm_has_mirrored_tdp(vcpu->kvm))
> > +               if (kvm_mmu_mirror_root_hpa_is_invalid(vcpu))
> >                         kvm_tdp_mmu_alloc_root(vcpu, true);
> > -               kvm_tdp_mmu_alloc_root(vcpu, false);
> > +
> > +               if (kvm_mmu_root_hpa_is_invalid(vcpu))
> > +                       kvm_tdp_mmu_alloc_root(vcpu, false);
> 
> So we can have either:
> mirror_root_hpa = INVALID_PAGE
> root.hpa = INVALID_PAGE
> 
> or:
> mirror_root_hpa = root
> root.hpa = INVALID_PAGE
> 
> We don't ever have:
> mirror_root_hpa = INVALID_PAGE
> root.hpa = root
> 
> Because mirror_root_hpa is special.
> 
Good point.

> >                 return 0;
> >         }
> > 
> 
> Having the check in kvm_mmu_reload() and here both is really ugly.  Right now we
> have kvm_mmu_reload() which only allocates new roots if needed, then
> kvm_mmu_load() goes and allocates/references them. Now mmu_alloc_direct_roots()
> has the same logic to only reload as needed.
> 
> So could we just leave the kvm_mmu_reload() logic as is, and just have special
> logic in mmu_alloc_direct_roots() to not avoid re-allocating mirror roots? This
> is actually what v19 had, but it was thought to be a historical relic:
>    "Historically we needed it.  We don't need it now. We can drop it."
> https://lore.kernel.org/kvm/20240325200122.GH2357401@xxxxxxxxxxxxxxxxxxxxx/
> 
> So just have this small fixup instead?
Yes.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index ae5d5dee86af..2e062178222e 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3705,7 +3705,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
>         int r;
>  
>         if (tdp_mmu_enabled) {
> -               if (kvm_has_mirrored_tdp(vcpu->kvm))
> +               if (kvm_has_mirrored_tdp(vcpu->kvm) &&
> +                   !VALID_PAGE(mmu->mirror_root_hpa))
>                         kvm_tdp_mmu_alloc_root(vcpu, true);
>                 kvm_tdp_mmu_alloc_root(vcpu, false);
>                 return 0;
> 
Perhaps also a comment in kvm_mmu_reload() to address concerns like why checking
only root.hpa in kvm_mmu_reload() is enough.

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 03737f3aaeeb..aba98c8cc67d 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -129,6 +129,15 @@ void kvm_mmu_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new,
 
 static inline int kvm_mmu_reload(struct kvm_vcpu *vcpu)
 {
+       /*
+        * Checking root.hpa is sufficient even when KVM has mirror root.
+        * We can have either:
+        * (1) mirror_root_hpa = INVALID_PAGE, root.hpa = INVALID_PAGE
+        * (2) mirror_root_hpa = root ,        root.hpa = INVALID_PAGE
+        * (3) mirror_root_hpa = root1,        root.hpa = root2
+        * We don't ever have:
+        *     mirror_root_hpa = INVALID_PAGE, root.hpa = root
+        */
        if (likely(vcpu->arch.mmu->root.hpa != INVALID_PAGE))
                return 0;
 
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index a5f803f1d17e..eee35e958971 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3705,7 +3705,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
        int r;
 
        if (tdp_mmu_enabled) {
-               if (kvm_has_mirrored_tdp(vcpu->kvm))
+               if (kvm_has_mirrored_tdp(vcpu->kvm) &&
+                   !VALID_PAGE(mmu->mirror_root_hpa))
                        kvm_tdp_mmu_alloc_root(vcpu, true);
                kvm_tdp_mmu_alloc_root(vcpu, false);
                return 0;