Re: [PATCH v3 2/3] KVM: arm64: Introduce KVM_PGTABLE_WALK_SKIP_LEVEL* walk flags

[Marc Zyngier]

On Wed, 08 Jul 2026 14:40:58 +0100,
Leonardo Bras <leo.bras@xxxxxxx> wrote:
> 
> Add the new walking flags that tell kvm_pgtable_walk() to skip lower levels
> when walking the pagetables.

I don't understand what 'lower' means here. There is also no
description of what this patch is trying to do, or why it is trying to
do it. All I see is a wall of code with no rationale, no explanation.

>
> Signed-off-by: Leonardo Bras <leo.bras@xxxxxxx>
> ---
>  arch/arm64/include/asm/kvm_pgtable.h | 13 +++++++++++++
>  arch/arm64/kvm/hyp/pgtable.c         | 19 ++++++++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 41a8687938eb..20c7c12e0e76 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -311,31 +311,44 @@ typedef bool (*kvm_pgtable_force_pte_cb_t)(u64 addr, u64 end,
>   * @KVM_PGTABLE_WALK_SHARED:		Indicates the page-tables may be shared
>   *					with other software walkers.
>   * @KVM_PGTABLE_WALK_IGNORE_EAGAIN:	Don't terminate the walk early if
>   *					the walker returns -EAGAIN.
>   * @KVM_PGTABLE_WALK_SKIP_BBM_TLBI:	Visit and update table entries
>   *					without Break-before-make's
>   *					TLB invalidation.
>   * @KVM_PGTABLE_WALK_SKIP_CMO:		Visit and update table entries
>   *					without Cache maintenance
>   *					operations required.
> + * @KVM_PGTABLE_WALK_SKIP_LEVEL0:	Skip visiting level-0+ entries
> + * @KVM_PGTABLE_WALK_SKIP_LEVEL1:	Skip visiting level-1+ entries
> + * @KVM_PGTABLE_WALK_SKIP_LEVEL2:	Skip visiting level-2+ entries
> + * @KVM_PGTABLE_WALK_SKIP_LEVEL3:	Skip visiting level-3 entries

Skip under which conditions? Always?

>   */
>  enum kvm_pgtable_walk_flags {
>  	KVM_PGTABLE_WALK_LEAF			= BIT(0),
>  	KVM_PGTABLE_WALK_TABLE_PRE		= BIT(1),
>  	KVM_PGTABLE_WALK_TABLE_POST		= BIT(2),
>  	KVM_PGTABLE_WALK_SHARED			= BIT(3),
>  	KVM_PGTABLE_WALK_IGNORE_EAGAIN		= BIT(4),
>  	KVM_PGTABLE_WALK_SKIP_BBM_TLBI		= BIT(5),
>  	KVM_PGTABLE_WALK_SKIP_CMO		= BIT(6),
> +	KVM_PGTABLE_WALK_SKIP_LEVEL0		= BIT(7),
> +	KVM_PGTABLE_WALK_SKIP_LEVEL1		= BIT(8),
> +	KVM_PGTABLE_WALK_SKIP_LEVEL2		= BIT(9),
> +	KVM_PGTABLE_WALK_SKIP_LEVEL3		= BIT(10),

There is a strong assumption that these bits must be contiguous. And
yet that's not captured anywhere. Want to bet what is going to happen
next?

>  };
>  
> +#define KVM_PGTABLE_WALK_SKIP_LEVELS 	(KVM_PGTABLE_WALK_SKIP_LEVEL0 | \
> +					 KVM_PGTABLE_WALK_SKIP_LEVEL1 | \
> +					 KVM_PGTABLE_WALK_SKIP_LEVEL2 | \
> +					 KVM_PGTABLE_WALK_SKIP_LEVEL3 )
> +
>  struct kvm_pgtable_visit_ctx {
>  	kvm_pte_t				*ptep;
>  	kvm_pte_t				old;
>  	void					*arg;
>  	struct kvm_pgtable_mm_ops		*mm_ops;
>  	u64					start;
>  	u64					addr;
>  	u64					end;
>  	s8					level;
>  	enum kvm_pgtable_walk_flags		flags;
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 4be1d51a6ac5..b9a2078efc51 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -137,20 +137,33 @@ static bool kvm_pgtable_walk_continue(const struct kvm_pgtable_walker *walker,
>  	 * Ignore the return code altogether for walkers outside a fault handler
>  	 * (e.g. write protecting a range of memory) and chug along with the
>  	 * page table walk.
>  	 */
>  	if (r == -EAGAIN)
>  		return walker->flags & KVM_PGTABLE_WALK_IGNORE_EAGAIN;
>  
>  	return !r;
>  }
>  
> +static __always_inline bool kvm_pgtable_skip_level(s8 level, enum kvm_pgtable_walk_flags flags)

Why __always_inline? I'm sure the compiler can decide for itself.

> +{
> +	flags &= KVM_PGTABLE_WALK_SKIP_LEVELS;
> +
> +	if (likely(!flags))
> +		return false;
> +
> +	if (level >= (ffs(flags) - ffs(KVM_PGTABLE_WALK_SKIP_LEVELS)))
> +		return true;

This looks awfully complex for something this trivial:

	u32 skip = FIELD_GET(KVM_PGTABLE_WALK_SKIP_LEVELS, flags);

	return skip && level >= ffs(skip);

But also, you seem to assume that there cannot be more than one such
flag set. I have no idea of the outcome in this situation, and the
whole thing is completely undocumented anyway...

> +
> +	return false;
> +}
> +
>  static int __kvm_pgtable_walk(struct kvm_pgtable_walk_data *data,
>  			      struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, s8 level);
>  
>  static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data,
>  				      struct kvm_pgtable_mm_ops *mm_ops,
>  				      kvm_pteref_t pteref, s8 level)
>  {
>  	enum kvm_pgtable_walk_flags flags = data->walker->flags;
>  	kvm_pte_t *ptep = kvm_dereference_pteref(data->walker, pteref);
>  	struct kvm_pgtable_visit_ctx ctx = {
> @@ -185,35 +198,35 @@ static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data,
>  	 * into a newly installed or replaced table.
>  	 */
>  	if (reload) {
>  		ctx.old = READ_ONCE(*ptep);
>  		table = kvm_pte_table(ctx.old, level);
>  	}
>  
>  	if (!kvm_pgtable_walk_continue(data->walker, ret))
>  		return ret;
>  
> -	if (!table) {
> +	if (!table || kvm_pgtable_skip_level(level + 1, ctx.flags)) {
>  		data->addr = ALIGN_DOWN(data->addr, kvm_granule_size(level));
>  		data->addr += kvm_granule_size(level);
>  		goto out;
>  	}
>  
>  	childp = (kvm_pteref_t)kvm_pte_follow(ctx.old, mm_ops);
>  	ret = __kvm_pgtable_walk(data, mm_ops, childp, level + 1);
>  	if (!kvm_pgtable_walk_continue(data->walker, ret))
>  		return ret;
>  
> -	if (ctx.flags & KVM_PGTABLE_WALK_TABLE_POST)
> +out:
> +	if (table && ctx.flags & KVM_PGTABLE_WALK_TABLE_POST)

I don't understand this change. Care to explain?

>  		ret = kvm_pgtable_visitor_cb(data, &ctx, KVM_PGTABLE_WALK_TABLE_POST);
>  
> -out:
>  	if (kvm_pgtable_walk_continue(data->walker, ret))
>  		return 0;
>  
>  	return ret;
>  }
>  
>  static int __kvm_pgtable_walk(struct kvm_pgtable_walk_data *data,
>  			      struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, s8 level)
>  {
>  	u32 idx;

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.