Re: [PATCH 04/10] arm/mpu: Implement virt/maddr conversion in MPU system

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

On 12/03/2025 14:52, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <Penny.Zheng@xxxxxxx>
> 
> virt_to_maddr and maddr_to_virt are used widely in Xen code. So
> even there is no VMSA in MPU system, we keep the interface in MPU to
> don't change the existing common code.
> 
> In order to do that, move the virt_to_maddr() definition to mmu/mm.h,
> instead for maddr_to_virt() it's more difficult to isolate it under mmu/
> so it will be protected by #ifdef CONFIG_MMU.
I don't understand this rationale. I did a quick test and moving maddr_to_virt
to mmu/mm.h works just fine.

> 
> Finally implement virt_to_maddr() and maddr_to_virt() for MPU systems
> under mpu/mm.h, the MPU version of virt/maddr conversion is simple since
> VA==PA.
> 
> While there, take the occasion to add emacs footer to mpu/mm.c.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>  xen/arch/arm/include/asm/mm.h     | 13 +++++++------
>  xen/arch/arm/include/asm/mmu/mm.h |  7 +++++++
>  xen/arch/arm/include/asm/mpu/mm.h | 27 +++++++++++++++++++++++++++
>  xen/arch/arm/mpu/mm.c             |  9 +++++++++
>  4 files changed, 50 insertions(+), 6 deletions(-)
>  create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index a0d8e5afe977..e7767cdab493 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -16,8 +16,10 @@
> 
>  #if defined(CONFIG_MMU)
>  # include <asm/mmu/mm.h>
> -#elif !defined(CONFIG_MPU)
> -# error "Unknown memory management layout"
> +#elif defined(CONFIG_MPU)
> +# include <asm/mpu/mm.h>
> +#else
> +#error "Unknown memory management layout"
>  #endif
> 
>  /* Align Xen to a 2 MiB boundary. */
> @@ -261,10 +263,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, 
> size_t len)
>  /* Page-align address and convert to frame number format */
>  #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
> 
> -#define virt_to_maddr(va) ({                                        \
> -    vaddr_t va_ = (vaddr_t)(va);                                    \
> -    (paddr_t)((va_to_par(va_) & PADDR_MASK & PAGE_MASK) | (va_ & 
> ~PAGE_MASK)); \
> -})
> +#if defined(CONFIG_MMU)
> 
>  #ifdef CONFIG_ARM_32
>  /**
> @@ -310,6 +309,8 @@ static inline void *maddr_to_virt(paddr_t ma)
>  }
>  #endif
> 
> +#endif /* CONFIG_MMU */
> +
>  /*
>   * Translate a guest virtual address to a machine address.
>   * Return the fault information if the translation has failed else 0.
> diff --git a/xen/arch/arm/include/asm/mmu/mm.h 
> b/xen/arch/arm/include/asm/mmu/mm.h
> index f5a00558c47b..5ff2071133ee 100644
> --- a/xen/arch/arm/include/asm/mmu/mm.h
> +++ b/xen/arch/arm/include/asm/mmu/mm.h
> @@ -2,6 +2,8 @@
>  #ifndef __ARM_MMU_MM_H__
>  #define __ARM_MMU_MM_H__
> 
> +#include <asm/page.h>
> +
>  /* Non-boot CPUs use this to find the correct pagetables. */
>  extern uint64_t init_ttbr;
> 
> @@ -14,6 +16,11 @@ extern unsigned long directmap_base_pdx;
> 
>  #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
> 
> +#define virt_to_maddr(va) ({                                                 
>   \
> +    vaddr_t va_ = (vaddr_t)(va);                                             
>   \
> +    (paddr_t)((va_to_par(va_) & PADDR_MASK & PAGE_MASK) | (va_ & 
> ~PAGE_MASK)); \
> +})
> +
>  /*
>   * Print a walk of a page table or p2m
>   *
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> new file mode 100644
> index 000000000000..57f1e558fd44
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_MPU_MM__
Missing _H? Should be: __ARM_MPU_MM_H__

> +#define __ARM_MPU_MM__
> +
> +#include <xen/macros.h>
I guess you also need xen/types.h

> +
> +#define virt_to_maddr(va) ({  \
> +    (paddr_t)va;              \
> +})
Why multiline? Also, shouldn't we take PA bits into account?
I'd imagine:
((paddr_t)((vaddr_t)(va) & PADDR_MASK))

> +
> +/* On MPU systems there is no translation, ma == va. */
> +static inline void *maddr_to_virt(paddr_t ma)
> +{
> +    return _p(ma);
Why do we need to cast paddr_t to unsigned long before casting to void?
Why not:
return (void *)(ma);

> +}
> +
> +#endif /* __ARM_MPU_MM__ */
__ARM_MPU_MM_H__

> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 0b8748e57598..a11e017d8a96 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -13,3 +13,12 @@ static void __init __maybe_unused build_assertions(void)
>       */
>      BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
>  }
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.34.1
> 

~Michal