On Wed, Jan 5, 2022 at 4:54 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Mon, Dec 13, 2021, David Matlack wrote:
> > Consolidate a bunch of code that was manually re-reading the spte if the
> > cmpxchg fails. There is no extra cost of doing this because we already
> > have the spte value as a result of the cmpxchg (and in fact this
> > eliminates re-reading the spte), and none of the call sites depend on
> > iter->old_spte retaining the stale spte value.
> >
> > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/tdp_mmu.c | 50 ++++++++++++++++----------------------
> > 1 file changed, 21 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index b69e47e68307..656ebf5b20dc 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -492,16 +492,22 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> > * and handle the associated bookkeeping. Do not mark the page dirty
> > * in KVM's dirty bitmaps.
> > *
> > + * If setting the SPTE fails because it has changed, iter->old_spte will be
> > + * updated with the updated value of the spte.
>
> First updated=>refreshed, second updated=>current? More below.
>
> > + *
> > * @kvm: kvm instance
> > * @iter: a tdp_iter instance currently on the SPTE that should be set
> > * @new_spte: The value the SPTE should be set to
> > * Returns: true if the SPTE was set, false if it was not. If false is returned,
> > - * this function will have no side-effects.
> > + * this function will have no side-effects other than updating
>
> s/updating/setting
>
> > + * iter->old_spte to the latest value of spte.
>
> Strictly speaking, "latest" may not be true if yet another thread modifies the
> SPTE. Maybe this?
>
> iter->old_spte to the last known value of the SPTE.
>
> > */
> > static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
> > struct tdp_iter *iter,
> > u64 new_spte)
> > {
> > + u64 old_spte;
> > +
> > lockdep_assert_held_read(&kvm->mmu_lock);
> >
> > /*
> > @@ -515,9 +521,15 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
> > * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
> > * does not hold the mmu_lock.
> > */
> > - if (cmpxchg64(rcu_dereference(iter->sptep), iter->old_spte,
> > - new_spte) != iter->old_spte)
> > + old_spte = cmpxchg64(rcu_dereference(iter->sptep), iter->old_spte, new_spte);
>
> To make this a bit easier to read, and to stay under 80 chars, how about
> opportunistically doing this as well?
>
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 656ebf5b20dc..64f1369f8638 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -506,6 +506,7 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
> struct tdp_iter *iter,
> u64 new_spte)
> {
> + u64 *sptep = rcu_dereference(iter->sptep);
> u64 old_spte;
>
> lockdep_assert_held_read(&kvm->mmu_lock);
> @@ -521,7 +522,7 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
> * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
> * does not hold the mmu_lock.
> */
> - old_spte = cmpxchg64(rcu_dereference(iter->sptep), iter->old_spte, new_spte);
> + old_spte = cmpxchg64(sptep, iter->old_spte, new_spte);
> if (old_spte != iter->old_spte) {
> /*
> * The cmpxchg failed because the spte was updated by another
>
> > + if (old_spte != iter->old_spte) {
> > + /*
> > + * The cmpxchg failed because the spte was updated by another
> > + * thread so record the updated spte in old_spte.
> > + */
>
> Hmm, this is a bit awkward. I think it's the double use of "updated" and the
> somewhat ambiguous reference to "old_spte". I'd also avoid "thread", as this
> requires interference from not only a different task, but a different logical CPU
> since iter->old_spte is refreshed if mmu_lock is dropped and reacquired. And
> "record" is an odd choice of word since it sounds like storing the current value
> is only for logging/debugging.
>
> Something like this?
>
> /*
> * The entry was modified by a different logical CPU, refresh
> * iter->old_spte with the current value so the caller operates
> * on fresh data, e.g. if it retries tdp_mmu_set_spte_atomic().
> */
>
> Nits aside,
>
> Reviewed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
Thanks for the review. I'll incorporate these into v2 (which I'm
holding off on until you have a chance to finish reviewing v1).
>
> > + iter->old_spte = old_spte;
> > return false;
> > + }
> >
> > __handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
> > new_spte, iter->level, true);
