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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7268ff80d3825fa6d7a50101358b47d5fbe86d5c.camel@intel.com>
Date: Wed, 3 Jul 2024 19:40:35 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Zhao, Yan Y" <yan.y.zhao@...el.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "Huang, Kai" <kai.huang@...el.com>,
	"sagis@...gle.com" <sagis@...gle.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"dmatlack@...gle.com" <dmatlack@...gle.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>
Subject: Re: [PATCH v3 13/17] KVM: x86/tdp_mmu: Support mirror root for TDP
 MMU

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.

>                 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@ls.amr.corp.intel.com/

So just have this small fixup instead?

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;