Re: [PATCH v4 02/10] KVM: arm64: Introduce for_each_hyp_page

[Marc Zyngier <maz@kernel.org>]

On Fri, 09 May 2025 14:16:58 +0100,
Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> Add a helper to iterate over the hypervisor vmemmap. This will be
> particularly handy with the introduction of huge mapping support
> for the np-guest stage-2.
> 
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index eb0c2ebd1743..676a0a13c741 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -96,24 +96,24 @@ static inline struct hyp_page *hyp_phys_to_page(phys_addr_t phys)
>  #define hyp_page_to_virt(page)	__hyp_va(hyp_page_to_phys(page))
>  #define hyp_page_to_pool(page)	(((struct hyp_page *)page)->pool)
>  
> -static inline enum pkvm_page_state get_host_state(phys_addr_t phys)
> +static inline enum pkvm_page_state get_host_state(struct hyp_page *p)
>  {
> -	return (enum pkvm_page_state)hyp_phys_to_page(phys)->__host_state;
> +	return (enum pkvm_page_state)p->__host_state;

I'm not quite sure why we have this cast the first place. If we are so
keen on type consistency, why isn't __host_state an enum pkvm_page_state
the first place?

>  }
>  
> -static inline void set_host_state(phys_addr_t phys, enum pkvm_page_state state)
> +static inline void set_host_state(struct hyp_page *p, enum pkvm_page_state state)
>  {
> -	hyp_phys_to_page(phys)->__host_state = state;
> +	p->__host_state = state;
>  }
>  
> -static inline enum pkvm_page_state get_hyp_state(phys_addr_t phys)
> +static inline enum pkvm_page_state get_hyp_state(struct hyp_page *p)
>  {
> -	return hyp_phys_to_page(phys)->__hyp_state_comp ^ PKVM_PAGE_STATE_MASK;
> +	return p->__hyp_state_comp ^ PKVM_PAGE_STATE_MASK;

And we don't seem that bothered here.

>  }
>  
> -static inline void set_hyp_state(phys_addr_t phys, enum pkvm_page_state state)
> +static inline void set_hyp_state(struct hyp_page *p, enum pkvm_page_state state)
>  {
> -	hyp_phys_to_page(phys)->__hyp_state_comp = state ^ PKVM_PAGE_STATE_MASK;
> +	p->__hyp_state_comp = state ^ PKVM_PAGE_STATE_MASK;
>  }
>  
>  /*
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 23544928a637..4d269210dae0 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -60,6 +60,9 @@ static void hyp_unlock_component(void)
>  	hyp_spin_unlock(&pkvm_pgd_lock);
>  }
>  
> +#define for_each_hyp_page(start, size, page)   \
> +	for (page = hyp_phys_to_page(start); page < hyp_phys_to_page((start) + (size)); page++)
> +

Huh. This really should be something like:

#define for_each_hyp_page(__p, __st, __sz)   			\
	for (struct hyp_page *__p = hyp_phys_to_page(__st),	\
	     *__e = __p + ((__sz) >> PAGE_SHIFT);		\
	     __p < __e; __p++)

so that:

- the iterator variable comes first and looks like a normal for-loop
- the iterator has a scope limited to the loop
- we don't evaluate parameters multiple times
- we don't evaluate the end of the loop multiple times
- we try not to reuse common variable names

>  static void *host_s2_zalloc_pages_exact(size_t size)
>  {
>  	void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size));
> @@ -481,7 +484,8 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range)
>  		return -EAGAIN;
>  
>  	if (pte) {
> -		WARN_ON(addr_is_memory(addr) && get_host_state(addr) != PKVM_NOPAGE);
> +		WARN_ON(addr_is_memory(addr) &&
> +			get_host_state(hyp_phys_to_page(addr)) != PKVM_NOPAGE);
>  		return -EPERM;
>  	}
>  
> @@ -507,10 +511,10 @@ int host_stage2_idmap_locked(phys_addr_t addr, u64 size,
>  
>  static void __host_update_page_state(phys_addr_t addr, u64 size, enum pkvm_page_state state)
>  {
> -	phys_addr_t end = addr + size;
> +	struct hyp_page *page;

and then these extra declarations can go.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.