Re: [PATCH v3 27/31] KVM: arm64: Use atomic refcount helpers for 'struct hyp_page::refcount'

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

On Fri, Apr 19, 2024 at 08:59:37AM +0100, Fuad Tabba wrote:
> From: Will Deacon <will@kernel.org>
> 
> Convert the 'struct hyp_page' refcount manipulation functions over to
> the new atomic refcount helpers. For now, this will make absolutely no
> difference because the 'struct hyp_pool' locking is still serialising
> everything. One step at a time...
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/kvm/hyp/include/nvhe/memory.h | 18 +++++++-----------
>  arch/arm64/kvm/hyp/nvhe/mem_protect.c    |  2 +-
>  arch/arm64/kvm/hyp/nvhe/page_alloc.c     |  5 ++++-
>  3 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index ab205c4d6774..74474c82667b 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -6,6 +6,7 @@
>  #include <asm/page.h>
>  
>  #include <linux/types.h>
> +#include <nvhe/refcount.h>
>  
>  struct hyp_page {
>  	unsigned short refcount;
> @@ -39,37 +40,32 @@ static inline phys_addr_t hyp_virt_to_phys(void *addr)
>  #define hyp_page_to_pool(page)	(((struct hyp_page *)page)->pool)
>  
>  /*
> - * Refcounting for 'struct hyp_page'.
> - * hyp_pool::lock must be held if atomic access to the refcount is required.
> + * Refcounting wrappers for 'struct hyp_page'.
>   */
>  static inline int hyp_page_count(void *addr)
>  {
>  	struct hyp_page *p = hyp_virt_to_page(addr);
>  
> -	return p->refcount;
> +	return hyp_refcount_get(p->refcount);
>  }
>  
>  static inline void hyp_page_ref_inc(struct hyp_page *p)
>  {
> -	BUG_ON(p->refcount == USHRT_MAX);
> -	p->refcount++;
> +	hyp_refcount_inc(p->refcount);

Adding a BUG_ON() for taking a reference on a non-refcounted page (i.e.
p->refcount was 0) would be nice, especially since we're past the point of
serializing everything and you can theoretically have a zero count page
outside of the free list.

Seems like otherwise we'd get actually hit the BUG_ON() in an unrelated
allocation path.

-- 
Thanks,
Oliver