Re: [PATCH v2 3/7] xen/arm: Introduce frame_table and virt_to_page

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

On 16/03/2025 20:24, Luca Fancellu wrote:
> 
> 
> Introduce frame_table in order to provide the implementation of
> virt_to_page for MPU system, move the MMU variant in mmu/mm.h.
> 
> Introduce FRAMETABLE_NR that is required for 'pdx_group_valid' in
> pdx.c, but leave the initialisation of the frame table to a later
> stage.
> Define FRAMETABLE_SIZE for MPU to support up to 1TB of ram, as the
> only current implementation of armv8-r aarch64, which is cortex R82,
> can address up to that memory.
When mentioning support statements like this one, it'd be beneficial to provide
a reference to a doc of some sort.

Also, shouldn't this be occasion to clarify SUPPORT statement as for max RAM
supported for ARMv8R-AArch64? ARMv8R support is experimental, so I'm not 100%
sure if we need to provide support statement for it at this stage though. Better
check with others.

> 
> Take the occasion to sort alphabetically the headers following
> the Xen code style and add the emacs footer in mpu/mm.c.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v2 changes:
>  - sorted headers in mm.c
>  - modified commit message
>  - moved virt_to_page to MMU and MPU
>  - removed frametable_pdx_end, used mfn_valid
> ---
>  xen/arch/arm/include/asm/mm.h         | 14 --------------
>  xen/arch/arm/include/asm/mmu/mm.h     | 14 ++++++++++++++
>  xen/arch/arm/include/asm/mpu/layout.h |  3 +++
>  xen/arch/arm/include/asm/mpu/mm.h     | 15 +++++++++++++++
>  xen/arch/arm/mpu/mm.c                 | 14 +++++++++++++-
>  5 files changed, 45 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 444fd03823ec..fbffaccef49b 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -294,20 +294,6 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va, 
> paddr_t *pa,
>  #error "Unknown memory management layout"
>  #endif
> 
> -/* Convert between Xen-heap virtual addresses and page-info structures. */
> -static inline struct page_info *virt_to_page(const void *v)
> -{
> -    unsigned long va = (unsigned long)v;
> -    unsigned long pdx;
> -
> -    ASSERT(va >= XENHEAP_VIRT_START);
> -    ASSERT(va < directmap_virt_end);
> -
> -    pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
> -    pdx += mfn_to_pdx(directmap_mfn_start);
> -    return frame_table + pdx - frametable_base_pdx;
> -}
> -
>  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/mmu/mm.h 
> b/xen/arch/arm/include/asm/mmu/mm.h
> index 6737c3ede783..caba987edc85 100644
> --- a/xen/arch/arm/include/asm/mmu/mm.h
> +++ b/xen/arch/arm/include/asm/mmu/mm.h
> @@ -70,6 +70,20 @@ static inline void *maddr_to_virt(paddr_t ma)
>  }
>  #endif
> 
> +/* Convert between Xen-heap virtual addresses and page-info structures. */
> +static inline struct page_info *virt_to_page(const void *v)
> +{
> +    unsigned long va = (unsigned long)v;
> +    unsigned long pdx;
> +
> +    ASSERT(va >= XENHEAP_VIRT_START);
> +    ASSERT(va < directmap_virt_end);
> +
> +    pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
> +    pdx += mfn_to_pdx(directmap_mfn_start);
> +    return frame_table + pdx - frametable_base_pdx;
> +}
> +
>  /*
>   * Print a walk of a page table or p2m
>   *
> diff --git a/xen/arch/arm/include/asm/mpu/layout.h 
> b/xen/arch/arm/include/asm/mpu/layout.h
> index 248e55f8882d..c331d1feaa84 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(16)
> +#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 6cfd0f5cd2c2..3a0a60dbfa18 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -3,9 +3,13 @@
>  #ifndef __ARM_MPU_MM_H__
>  #define __ARM_MPU_MM_H__
> 
> +#include <xen/bug.h>
>  #include <xen/macros.h>
>  #include <xen/page-size.h>
>  #include <xen/types.h>
> +#include <asm/mm.h>
> +
> +extern struct page_info *frame_table;
> 
>  #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))
> 
> @@ -15,6 +19,17 @@ static inline void *maddr_to_virt(paddr_t ma)
>      return _p(ma);
>  }
> 
> +/* Convert between virtual address to page-info structure. */
> +static inline struct page_info *virt_to_page(const void *v)
> +{
> +    paddr_t paddr = virt_to_maddr(v);
> +    unsigned long pdx = paddr_to_pdx(paddr);
> +
> +    ASSERT(mfn_valid(maddr_to_mfn(paddr)));
> +
> +    return frame_table + pdx - frametable_base_pdx;
> +}
This could be simplified (and number of conversions reduced) by doing sth like:
mfn_t mfn = virt_to_mfn(v);

ASSERT(mfn_valid(mfn));

return mfn_to_page(mfn);

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal