On Thu, Jan 6, 2022 at 12:27 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Mon, Dec 13, 2021, David Matlack wrote:
> > restore_acc_track_spte is purely an SPTE manipulation, making it a good
> > fit for spte.c. It is also needed in spte.c in a follow-up commit so we
> > can construct child SPTEs during large page splitting.
> >
> > No functional change intended.
> >
> > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> > Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 18 ------------------
> > arch/x86/kvm/mmu/spte.c | 18 ++++++++++++++++++
> > arch/x86/kvm/mmu/spte.h | 1 +
> > 3 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 8b702f2b6a70..3c2cb4dd1f11 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -646,24 +646,6 @@ static u64 mmu_spte_get_lockless(u64 *sptep)
> > return __get_spte_lockless(sptep);
> > }
> >
> > -/* Restore an acc-track PTE back to a regular PTE */
> > -static u64 restore_acc_track_spte(u64 spte)
> > -{
> > - u64 new_spte = spte;
> > - u64 saved_bits = (spte >> SHADOW_ACC_TRACK_SAVED_BITS_SHIFT)
> > - & SHADOW_ACC_TRACK_SAVED_BITS_MASK;
> > -
> > - WARN_ON_ONCE(spte_ad_enabled(spte));
> > - WARN_ON_ONCE(!is_access_track_spte(spte));
> > -
> > - new_spte &= ~shadow_acc_track_mask;
> > - new_spte &= ~(SHADOW_ACC_TRACK_SAVED_BITS_MASK <<
> > - SHADOW_ACC_TRACK_SAVED_BITS_SHIFT);
> > - new_spte |= saved_bits;
> > -
> > - return new_spte;
> > -}
> > -
> > /* Returns the Accessed status of the PTE and resets it at the same time. */
> > static bool mmu_spte_age(u64 *sptep)
> > {
> > diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
> > index 8a7b03207762..fd34ae5d6940 100644
> > --- a/arch/x86/kvm/mmu/spte.c
> > +++ b/arch/x86/kvm/mmu/spte.c
> > @@ -268,6 +268,24 @@ u64 mark_spte_for_access_track(u64 spte)
> > return spte;
> > }
> >
> > +/* Restore an acc-track PTE back to a regular PTE */
> > +u64 restore_acc_track_spte(u64 spte)
> > +{
> > + u64 new_spte = spte;
> > + u64 saved_bits = (spte >> SHADOW_ACC_TRACK_SAVED_BITS_SHIFT)
> > + & SHADOW_ACC_TRACK_SAVED_BITS_MASK;
>
> Obviously not your code, but this could be:
>
> u64 saved_bits = (spte >> SHADOW_ACC_TRACK_SAVED_BITS_SHIFT) &
> SHADOW_ACC_TRACK_SAVED_BITS_MASK;
>
> WARN_ON_ONCE(spte_ad_enabled(spte));
> WARN_ON_ONCE(!is_access_track_spte(spte));
>
> spte &= ~shadow_acc_track_mask;
> spte &= ~(SHADOW_ACC_TRACK_SAVED_BITS_MASK <<
> SHADOW_ACC_TRACK_SAVED_BITS_SHIFT);
> spte |= saved_bits;
>
> return spte;
>
> which is really just an excuse to move the ampersand up a line :-)
>
> And looking at the two callers, the WARNs are rather silly. The spte_ad_enabled()
> WARN is especially pointless because that's also checked by is_access_track_spte().
> I like paranoid WARNs as much as anyone, but I don't see why this code warrants
> extra checking relative to the other SPTE helpers that have more subtle requirements.
>
> At that point, make make this an inline helper?
>
> static inline u64 restore_acc_track_spte(u64 spte)
> {
> u64 saved_bits = (spte >> SHADOW_ACC_TRACK_SAVED_BITS_SHIFT) &
> SHADOW_ACC_TRACK_SAVED_BITS_MASK;
>
> spte &= ~shadow_acc_track_mask;
> spte &= ~(SHADOW_ACC_TRACK_SAVED_BITS_MASK <<
> SHADOW_ACC_TRACK_SAVED_BITS_SHIFT);
> spte |= saved_bits;
>
> return spte;
> }
That all sounds reasonable. I'll include some additional patches in
the next version to include these cleanups.
>
> > + WARN_ON_ONCE(spte_ad_enabled(spte));
> > + WARN_ON_ONCE(!is_access_track_spte(spte));
> > +
> > + new_spte &= ~shadow_acc_track_mask;
> > + new_spte &= ~(SHADOW_ACC_TRACK_SAVED_BITS_MASK <<
> > + SHADOW_ACC_TRACK_SAVED_BITS_SHIFT);
> > + new_spte |= saved_bits;
> > +
> > + return new_spte;
> > +}
> > +
> > void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask)
> > {
> > BUG_ON((u64)(unsigned)access_mask != access_mask);
> > diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> > index a4af2a42695c..9b0c7b27f23f 100644
> > --- a/arch/x86/kvm/mmu/spte.h
> > +++ b/arch/x86/kvm/mmu/spte.h
> > @@ -337,6 +337,7 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
> > u64 make_nonleaf_spte(u64 *child_pt, bool ad_disabled);
> > u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access);
> > u64 mark_spte_for_access_track(u64 spte);
> > +u64 restore_acc_track_spte(u64 spte);
> > u64 kvm_mmu_changed_pte_notifier_make_spte(u64 old_spte, kvm_pfn_t new_pfn);
> >
> > void kvm_mmu_reset_all_pte_masks(void);
> > --
> > 2.34.1.173.g76aa8bc2d0-goog
> >
