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

[Gavin Shan]

Hi Oliver,

On 11/10/22 8:42 AM, Oliver Upton wrote:
> 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 at 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 at 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:
> 

Right, so how about to rearrange these fields like below? It makes
more sense to have @arg after addr/end/ptep.

    struct kvm_pgtable_visit_ctx {
           enum kvm_pgtable_walk_flags     flags;
           u32                             level;
           u64                             addr;
           u64                             end;
           kvm_pte_t                       *ptep;
           void                            *arg;
    };


[...]

Thanks,
Gavin