On Thu, Aug 15, 2024, Sean Christopherson wrote:
> On Wed, Aug 14, 2024, Paolo Bonzini wrote:
> > On 8/9/24 21:03, Sean Christopherson wrote:
> > > Explicitly query the list of to-be-zapped shadow pages when checking to
> > > see if unprotecting a gfn for retry has succeeded, i.e. if KVM should
> > > retry the faulting instruction.
> > >
> > > Add a comment to explain why the list needs to be checked before zapping,
> > > which is the primary motivation for this change.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > > arch/x86/kvm/mmu/mmu.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > index 300a47801685..50695eb2ee22 100644
> > > --- a/arch/x86/kvm/mmu/mmu.c
> > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > @@ -2731,12 +2731,15 @@ bool __kvm_mmu_unprotect_gfn_and_retry(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> > > goto out;
> > > }
> > > - r = false;
> > > write_lock(&kvm->mmu_lock);
> > > - for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) {
> > > - r = true;
> > > + for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa))
> > > kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
> > > - }
> > > +
> > > + /*
> > > + * Snapshot the result before zapping, as zapping will remove all list
> > > + * entries, i.e. checking the list later would yield a false negative.
> > > + */
> >
> > Hmm, the comment is kinda overkill? Maybe just
> >
> > /* Return whether there were sptes to zap. */
> > r = !list_empty(&invalid_test);
>
> I would strongly prefer to keep the verbose comment. I was "this" close to
> removing the local variable and checking list_empty() after the commit phase.
> If we made that goof, it would only show up at the worst time, i.e. when a guest
> triggers retry and gets stuck. And the logical outcome of fixing such a bug
> would be to add a comment to prevent it from happening again, so I say just add
> the comment straightaway.
>
> > I'm not sure about patch 21 - I like the simple kvm_mmu_unprotect_page()
> > function.
>
> >From a code perspective, I kinda like having a separate helper too. As you
> likely suspect given your below suggestion, KVM should never unprotect a gfn
> without retry protection, i.e. there should never be another caller, and I want
> to enforce that.
Oh, another argument for eliminating the separate helper is that having a separate
helper makes it really hard to write a comment for why reading indirect_shadow_pages
outside of mmu_lock is ok (it reads/looks weird if mmu_lock is taken in a different
helper).
