Re: [RFC PATCH 05/17] KVM: arm64: Take an argument to indicate parallel walk

[Oliver Upton <oupton@google.com>]

On Sat, Apr 16, 2022 at 12:30:23PM +0100, Marc Zyngier wrote:
> Hi Oliver,
> 
> On Fri, 15 Apr 2022 22:58:49 +0100,
> Oliver Upton <oupton@google.com> wrote:
> > 
> > It is desirable to reuse the same page walkers for serial and parallel
> > faults. Take an argument to kvm_pgtable_walk() (and throughout) to
> > indicate whether or not a walk might happen in parallel with another.
> >
> > No functional change intended.
> > 
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_pgtable.h  |  5 +-
> >  arch/arm64/kvm/hyp/nvhe/mem_protect.c |  4 +-
> >  arch/arm64/kvm/hyp/nvhe/setup.c       |  4 +-
> >  arch/arm64/kvm/hyp/pgtable.c          | 91 ++++++++++++++-------------
> >  4 files changed, 54 insertions(+), 50 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index ea818a5f7408..74955aba5918 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -194,7 +194,7 @@ enum kvm_pgtable_walk_flags {
> >  typedef int (*kvm_pgtable_visitor_fn_t)(u64 addr, u64 end, u32 level,
> >  					kvm_pte_t *ptep, kvm_pte_t *old,
> >  					enum kvm_pgtable_walk_flags flag,
> > -					void * const arg);
> > +					void * const arg, bool shared);
> 
> Am I the only one who find this really ugly? Sprinkling this all over
> the shop makes the code rather unreadable. It seems to me that having
> some sort of more general context would make more sense.

You certainly are not. This is a bit sloppy, a previous spin of this
needed to know about parallelism in the generic page walker context and
I had picked just poking the bool through instead of hitching it to
kvm_pgtable_walker. I needed to churn either way in that scheme, but
that is no longer the case now.

> For example, I would fully expect the walk context to tell us whether
> this walker is willing to share its walk. Add a predicate to that,
> which would conveniently expand to 'false' for contexts where we don't
> have RCU (such as the pKVM HYP PT management, and you should get
> something that is more manageable.

I think the blast radius is now limited to just the stage2 visitors, so
it can probably get crammed in the callback arg now. Limiting the
changes to stage2 was intentional. The hyp walkers seem to be working
fine and I'd rather not come under fire for breaking it somehow ;)

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm