Re: [PATCH v5 01/14] KVM: arm64: Combine visitor arguments into a context structure

[Oliver Upton <oliver.upton@linux.dev>]

Hi Gavin,

On Thu, Nov 10, 2022 at 08:23:36AM +0800, Gavin Shan wrote:
> On 11/8/22 5:56 AM, Oliver Upton wrote:
> > Passing new arguments by value to the visitor callbacks is extremely
> > inflexible for stuffing new parameters used by only some of the
> > visitors. Use a context structure instead and pass the pointer through
> > to the visitor callback.
> > 
> > While at it, redefine the 'flags' parameter to the visitor to contain
> > the bit indicating the phase of the walk. Pass the entire set of flags
> > through the context structure such that the walker can communicate
> > additional state to the visitor callback.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >   arch/arm64/include/asm/kvm_pgtable.h  |  15 +-
> >   arch/arm64/kvm/hyp/nvhe/mem_protect.c |  10 +-
> >   arch/arm64/kvm/hyp/nvhe/setup.c       |  16 +-
> >   arch/arm64/kvm/hyp/pgtable.c          | 269 +++++++++++++-------------
> >   4 files changed, 154 insertions(+), 156 deletions(-)
> > 
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> 
> One nit below.
> 
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index 3252eb50ecfe..607f9bb8aab4 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -199,10 +199,17 @@ enum kvm_pgtable_walk_flags {
> >   	KVM_PGTABLE_WALK_TABLE_POST		= BIT(2),
> >   };
> > -typedef int (*kvm_pgtable_visitor_fn_t)(u64 addr, u64 end, u32 level,
> > -					kvm_pte_t *ptep,
> > -					enum kvm_pgtable_walk_flags flag,
> > -					void * const arg);
> > +struct kvm_pgtable_visit_ctx {
> > +	kvm_pte_t				*ptep;
> > +	void					*arg;
> > +	u64					addr;
> > +	u64					end;
> > +	u32					level;
> > +	enum kvm_pgtable_walk_flags		flags;
> > +};
> > +
> > +typedef int (*kvm_pgtable_visitor_fn_t)(const struct kvm_pgtable_visit_ctx *ctx,
> > +					enum kvm_pgtable_walk_flags visit);
> 
> Does it make sense to reorder these fields in the context struct based on
> their properties.

The ordering was a deliberate optimization for space. Your suggestion
has 8 bytes of implicit padding:

>     struct kvm_pgtable_visit_ctx {
>            enum kvm_pgtable_walk_flags     flags;

here

>            u64                             addr;
>            u64                             end;
>            u32                             level;

and here.

>            kvm_pte_t                       *ptep;
>            void                            *arg;
>     };

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