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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 12/03/2025 13: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.
Have you considered including "asm/{mmu,mpu}/mm.h" **after** struct 
page_info is declared?
> Introduce FRAMETABLE_NR that is required for 'pdx_group_valid' in
> pdx.c.

Maybe clarify in the commit message that the frametable will be setup at 
a later stage?
> 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);
> +
> +    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)
I guess you copied the value for the MMU code for arm64. But is this 
value still sensible for MPU? What about arm32?
In any case, some documentation would be useful.

> +#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>
> +
> +struct page_info *frame_table;
> +unsigned long __read_mostly frametable_pdx_end;
>   
>   static void __init __maybe_unused build_assertions(void)
>   {
Cheers,

--
Julien Grall