Re: [PATCH 05/10] arm/mpu: Introduce frame_table, virt_to_page, maddr_to_virt

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 12/03/2025 14:52, Luca Fancellu wrote:
> 
> 
> Introduce variables and functions used in the common Arm code by
> MPU memory management subsystem, provide struct page_info and
> the MPU implementation for helpers and macros used in the common
> arm code.
> 
> Moving virt_to_page helper to mmu/mpu part is not easy as it needs
> visibility of 'struct page_info', so protect it with CONFIG_MMU
> and provide the MPU variant in the #else branch.
I see we move some helpers to mmu,mpu while others stay in generic mm.h. If it's
not possible to do the movement, then I'd suggest to keep the pairs in generic
mm.h to avoid situation where one helper e.g. MMU is in generic mm.h and for MPU
in mpu/mm.h

> 
> Introduce FRAMETABLE_NR that is required for 'pdx_group_valid' in
> pdx.c.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>  xen/arch/arm/include/asm/mm.h         | 18 ++++++++++++++++++
>  xen/arch/arm/include/asm/mpu/layout.h |  3 +++
>  xen/arch/arm/include/asm/mpu/mm.h     |  3 +++
>  xen/arch/arm/mpu/mm.c                 |  4 ++++
>  4 files changed, 28 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index e7767cdab493..c96d33aceaf0 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -341,6 +341,8 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va, paddr_t 
> *pa,
>  #define virt_to_mfn(va)     __virt_to_mfn(va)
>  #define mfn_to_virt(mfn)    __mfn_to_virt(mfn)
> 
> +#ifdef CONFIG_MMU
> +
>  /* Convert between Xen-heap virtual addresses and page-info structures. */
>  static inline struct page_info *virt_to_page(const void *v)
>  {
> @@ -355,6 +357,22 @@ static inline struct page_info *virt_to_page(const void 
> *v)
>      return frame_table + pdx - frametable_base_pdx;
>  }
> 
> +#else /* !CONFIG_MMU */
> +
> +/* Convert between virtual address to page-info structure. */
> +static inline struct page_info *virt_to_page(const void *v)
> +{
> +    unsigned long pdx;
> +
> +    pdx = paddr_to_pdx(virt_to_maddr(v));
> +    ASSERT(pdx >= frametable_base_pdx);
> +    ASSERT(pdx < frametable_pdx_end);
I struggle to see the usefulness of frametable_pdx_end and we don't generally
add more that start, size. Looking at your tree, the only use of
frametable_pdx_end is in this ASSERT which is a bit pointless considering
release build. You already have start, size, so you could open code it.

> +
> +    return frame_table + pdx - frametable_base_pdx;
> +}
> +
> +#endif /* CONFIG_MMU */
> +
>  static inline void *page_to_virt(const struct page_info *pg)
>  {
>      return mfn_to_virt(mfn_x(page_to_mfn(pg)));
> diff --git a/xen/arch/arm/include/asm/mpu/layout.h 
> b/xen/arch/arm/include/asm/mpu/layout.h
> index 248e55f8882d..c46b634c9c15 100644
> --- a/xen/arch/arm/include/asm/mpu/layout.h
> +++ b/xen/arch/arm/include/asm/mpu/layout.h
> @@ -3,6 +3,9 @@
>  #ifndef __ARM_MPU_LAYOUT_H__
>  #define __ARM_MPU_LAYOUT_H__
> 
> +#define FRAMETABLE_SIZE   GB(32)
> +#define FRAMETABLE_NR     (FRAMETABLE_SIZE / sizeof(*frame_table))
> +
>  #define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
> 
>  /*
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index 57f1e558fd44..2219c9979548 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -5,6 +5,9 @@
> 
>  #include <xen/macros.h>
> 
> +extern struct page_info *frame_table;
> +extern unsigned long frametable_pdx_end;
> +
>  #define virt_to_maddr(va) ({  \
>      (paddr_t)va;              \
>  })
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index a11e017d8a96..4036dd62eeeb 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -3,6 +3,10 @@
>  #include <xen/lib.h>
>  #include <xen/init.h>
>  #include <xen/sizes.h>
> +#include <xen/mm.h>
It's not great for a relatively new header to not have its includes sorted.
Please take the occasion to reorder them.

> +
> +struct page_info *frame_table;
> +unsigned long __read_mostly frametable_pdx_end;
> 
>  static void __init __maybe_unused build_assertions(void)
>  {
> --
> 2.34.1
> 

~Michal