linux-kernel - Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping 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 for Android: free password hash cracker in your pocket

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

Date: Tue, 28 May 2024 20:54:31 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>
CC: "sagis@...gle.com" <sagis@...gle.com>, "dmatlack@...gle.com"
	<dmatlack@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "Zhao, Yan Y" <yan.y.zhao@...el.com>, "Aktas,
 Erdem" <erdemaktas@...gle.com>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU

On Tue, 2024-05-14 at 17:59 -0700, Rick Edgecombe wrote:
> +static inline int __tdp_mmu_set_spte_atomic(struct kvm *kvm, struct tdp_iter
> *iter, u64 new_spte)
>  {
>         u64 *sptep = rcu_dereference(iter->sptep);
>  
> @@ -542,15 +671,42 @@ static inline int __tdp_mmu_set_spte_atomic(struct
> tdp_iter *iter, u64 new_spte)
>          */
>         WARN_ON_ONCE(iter->yielded || is_removed_spte(iter->old_spte));
>  
> -       /*
> -        * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
> -        * does not hold the mmu_lock.  On failure, i.e. if a different
> logical
> -        * CPU modified the SPTE, try_cmpxchg64() updates iter->old_spte with
> -        * the current value, so the caller operates on fresh data, e.g. if it
> -        * retries tdp_mmu_set_spte_atomic()
> -        */
> -       if (!try_cmpxchg64(sptep, &iter->old_spte, new_spte))
> -               return -EBUSY;
> +       if (is_private_sptep(iter->sptep) && !is_removed_spte(new_spte)) {
> +               int ret;
> +
> +               if (is_shadow_present_pte(new_spte)) {
> +                       /*
> +                        * Populating case.
> +                        * - set_private_spte_present() implements
> +                        *   1) Freeze SPTE
> +                        *   2) call hooks to update private page table,
> +                        *   3) update SPTE to new_spte
> +                        * - handle_changed_spte() only updates stats.
> +                        */
> +                       ret = set_private_spte_present(kvm, iter->sptep, iter-
> >gfn,
> +                                                      iter->old_spte,
> new_spte, iter->level);
> +                       if (ret)
> +                               return ret;
> +               } else {
> +                       /*
> +                        * Zapping case.
> +                        * Zap is only allowed when write lock is held
> +                        */
> +                       if (WARN_ON_ONCE(!is_shadow_present_pte(new_spte)))

This inside an else block for (is_shadow_present_pte(new_spte)), so it will
always be true if it gets here. But it can't because TDX doesn't do any atomic
zapping.

We can remove the conditional, but in regards to the WARN, any recollection of
what was might have been going on here originally?

> +                               return -EBUSY;
> +               }