Received-SPF: pass (google.com: domain of linux-kernel+bounces-205798-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240530210714.364118-1-rick.p.edgecombe@intel.com> <20240530210714.364118-11-rick.p.edgecombe@intel.com>
In-Reply-To: <20240530210714.364118-11-rick.p.edgecombe@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 7 Jun 2024 12:10:33 +0200
Message-ID: <CABgObfYhKmBkqGP-d12o6W2TfiaqwP-c8pcae9-pnkaYJt6K-w@mail.gmail.com>
Subject: Re: [PATCH v2 10/15] KVM: x86/tdp_mmu: Reflect building mirror page tables
To: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: seanjc@google.com, kvm@vger.kernel.org, kai.huang@intel.com, 
	dmatlack@google.com, erdemaktas@google.com, isaku.yamahata@gmail.com, 
	linux-kernel@vger.kernel.org, sagis@google.com, yan.y.zhao@intel.com, 
	Isaku Yamahata <isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 30, 2024 at 11:07=E2=80=AFPM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
> +       /* Update mirrored mapping with page table link */
> +       int (*reflect_link_spt)(struct kvm *kvm, gfn_t gfn, enum pg_level=
 level,
> +                               void *mirrored_spt);
> +       /* Update the mirrored page table from spte getting set */
> +       int (*reflect_set_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level=
 level,
> +                               kvm_pfn_t pfn);

Possibly link_external_spt and set_external_spte, since you'll have to
s/mirrored/external/ in the comment. But not a hard request.

> +static void *get_mirrored_spt(gfn_t gfn, u64 new_spte, int level)
> +{
> +       if (is_shadow_present_pte(new_spte) && !is_last_spte(new_spte, le=
vel)) {
> +               struct kvm_mmu_page *sp =3D to_shadow_page(pfn_to_hpa(spt=
e_to_pfn(new_spte)));

I think this is spte_to_child_sp(new_spte)?

> +               void *mirrored_spt =3D kvm_mmu_mirrored_spt(sp);
> +
> +               WARN_ON_ONCE(sp->role.level + 1 !=3D level);
> +               WARN_ON_ONCE(sp->gfn !=3D gfn);
> +               return mirrored_spt;

Based on previous reviews this can be just "return sp->external_spt",
removing the not-particularly-interesting kvm_mmu_mirrored_spt()
helper.

> +static int __must_check reflect_set_spte_present(struct kvm *kvm, tdp_pt=
ep_t sptep,

tdp_mmu_set_mirror_spte_atomic?

> +       /*
> +        * For mirrored page table, callbacks are needed to propagate SPT=
E
> +        * change into the mirrored page table. In order to atomically up=
date
> +        * both the SPTE and the mirrored page tables with callbacks, uti=
lize
> +        * freezing SPTE.
> +        * - Freeze the SPTE. Set entry to REMOVED_SPTE.
> +        * - Trigger callbacks for mirrored page tables.
> +        * - Unfreeze the SPTE.  Set the entry to new_spte.
> +        */

/*
 * We need to lock out other updates to the SPTE until the external
 * page table has been modified. Use REMOVED_SPTE similar to
 * the zapping case.
 */

Easy peasy. :) We may want to rename REMOVED_SPTE to FROZEN_SPTE; feel
free to do it at the head of this series, then it can be picked for
6.11.

> -static inline int __tdp_mmu_set_spte_atomic(struct tdp_iter *iter, u64 n=
ew_spte)
> +static inline int __tdp_mmu_set_spte_atomic(struct kvm *kvm, struct tdp_=
iter *iter, u64 new_spte)
>  {
>         u64 *sptep =3D rcu_dereference(iter->sptep);
>
> @@ -571,15 +629,36 @@ 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 l=
ogical
> -        * 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_mirror_sptep(iter->sptep) && !is_removed_spte(new_spte)) {
> +               int ret;
> +
> +               /* Don't support atomic zapping for mirrored roots */

The why is hidden in the commit message to patch 11. I wonder if it
isn't clearer to simply squash together patches 10 and 11 (your call),
and instead split out the addition of the new struct kvm parameters.

Anyway, this comment needs a bit more info:

/*
 * Users of atomic zapping don't operate on mirror roots,
 * so only need to handle present new_spte.
 */

> +               if (KVM_BUG_ON(!is_shadow_present_pte(new_spte), kvm))
> +                       return -EBUSY;
> +               /*
> +                * Populating case.
> +                * - reflect_set_spte_present() implements
> +                *   1) Freeze SPTE
> +                *   2) call hooks to update mirrored page table,
> +                *   3) update SPTE to new_spte
> +                * - handle_changed_spte() only updates stats.
> +                */

Comment not needed (weird I know).


> +               ret =3D reflect_set_spte_present(kvm, iter->sptep, iter->=
gfn,
> +                                              iter->old_spte, new_spte, =
iter->level);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               /*
> +                * Note, fast_pf_fix_direct_spte() can also modify TDP MM=
U 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;
> +       }
>
>         return 0;
>  }
> @@ -610,7 +689,7 @@ static inline int tdp_mmu_set_spte_atomic(struct kvm =
*kvm,
>
>         lockdep_assert_held_read(&kvm->mmu_lock);
>
> -       ret =3D __tdp_mmu_set_spte_atomic(iter, new_spte);
> +       ret =3D __tdp_mmu_set_spte_atomic(kvm, iter, new_spte);
>         if (ret)
>                 return ret;
>
> @@ -636,7 +715,7 @@ static inline int tdp_mmu_zap_spte_atomic(struct kvm =
*kvm,
>          * Delay processing of the zapped SPTE until after TLBs are flush=
ed and
>          * the REMOVED_SPTE is replaced (see below).
>          */
> -       ret =3D __tdp_mmu_set_spte_atomic(iter, REMOVED_SPTE);
> +       ret =3D __tdp_mmu_set_spte_atomic(kvm, iter, REMOVED_SPTE);
>         if (ret)
>                 return ret;
>
> @@ -698,6 +777,11 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_=
id, tdp_ptep_t sptep,
>         role =3D sptep_to_sp(sptep)->role;
>         role.level =3D level;
>         handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, role, fa=
lse);
> +
> +       /* Don't support setting for the non-atomic case */
> +       if (is_mirror_sptep(sptep))
> +               KVM_BUG_ON(is_shadow_present_pte(new_spte), kvm);
> +
>         return old_spte;
>  }
>
> --
> 2.34.1
>