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

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

On 13/03/2025 12:51, Luca Fancellu wrote:
> 
> 
> Hi Julien,
> 
>> On 13 Mar 2025, at 09:22, Julien Grall <julien@xxxxxxx> wrote:
>>
>> 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?
> 
> I didn’t tried that, but if we do that we solve all the issues (I don’t like 
> #includes in the middle of headers,
> because of that I didn’t try).
> 
> This is what it looks like:
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 9bf5c846c86c..b49ec9c3dd1a 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -14,14 +14,6 @@
>  # error "unknown ARM variant"
>  #endif
> 
> -#if defined(CONFIG_MMU)
> -# include <asm/mmu/mm.h>
> -#elif defined(CONFIG_MPU)
> -# include <asm/mpu/mm.h>
> -#else
> -#error "Unknown memory management layout"
> -#endif
> -
>  /* Align Xen to a 2 MiB boundary. */
>  #define XEN_PADDR_ALIGN (1 << 21)
> 
> @@ -264,53 +256,13 @@ static inline void __iomem *ioremap_wc(paddr_t start, 
> size_t len)
>  #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
> 
>  #if defined(CONFIG_MMU)
> -
> -#ifdef CONFIG_ARM_32
> -/**
> - * Find the virtual address corresponding to a machine address
> - *
> - * Only memory backing the XENHEAP has a corresponding virtual address to
> - * be found. This is so we can save precious virtual space, as it's in
> - * short supply on arm32. This mapping is not subject to PDX compression
> - * because XENHEAP is known to be physically contiguous and can't hence
> - * jump over the PDX hole. This means we can avoid the roundtrips
> - * converting to/from pdx.
> - *
> - * @param ma Machine address
> - * @return Virtual address mapped to `ma`
> - */
> -static inline void *maddr_to_virt(paddr_t ma)
> -{
> -    ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
> -    ma -= mfn_to_maddr(directmap_mfn_start);
> -    return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
> -}
> +# include <asm/mmu/mm.h>
> +#elif defined(CONFIG_MPU)
> +# include <asm/mpu/mm.h>
>  #else
> -/**
> - * Find the virtual address corresponding to a machine address
> - *
> - * The directmap covers all conventional memory accesible by the
> - * hypervisor. This means it's subject to PDX compression.
> - *
> - * Note there's an extra offset applied (directmap_base_pdx) on top of the
> - * regular PDX compression logic. Its purpose is to skip over the initial
> - * range of non-existing memory, should there be one.
> - *
> - * @param ma Machine address
> - * @return Virtual address mapped to `ma`
> - */
> -static inline void *maddr_to_virt(paddr_t ma)
> -{
> -    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - directmap_base_pdx) <
> -           (DIRECTMAP_SIZE >> PAGE_SHIFT));
> -    return (void *)(XENHEAP_VIRT_START -
> -                    (directmap_base_pdx << PAGE_SHIFT) +
> -                    maddr_to_directmapoff(ma));
> -}
> +#error "Unknown memory management layout"
>  #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 5ff2071133ee..9557f632d8e6 100644
> --- a/xen/arch/arm/include/asm/mmu/mm.h
> +++ b/xen/arch/arm/include/asm/mmu/mm.h
> @@ -21,6 +21,50 @@ extern unsigned long directmap_base_pdx;
>      (paddr_t)((va_to_par(va_) & PADDR_MASK & PAGE_MASK) | (va_ & 
> ~PAGE_MASK)); \
>  })
> 
> +#ifdef CONFIG_ARM_32
> +/**
> + * Find the virtual address corresponding to a machine address
> + *
> + * Only memory backing the XENHEAP has a corresponding virtual address to
> + * be found. This is so we can save precious virtual space, as it's in
> + * short supply on arm32. This mapping is not subject to PDX compression
> + * because XENHEAP is known to be physically contiguous and can't hence
> + * jump over the PDX hole. This means we can avoid the roundtrips
> + * converting to/from pdx.
> + *
> + * @param ma Machine address
> + * @return Virtual address mapped to `ma`
> + */
> +static inline void *maddr_to_virt(paddr_t ma)
> +{
> +    ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
> +    ma -= mfn_to_maddr(directmap_mfn_start);
> +    return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
> +}
> +#else
> +/**
> + * Find the virtual address corresponding to a machine address
> + *
> + * The directmap covers all conventional memory accesible by the
> + * hypervisor. This means it's subject to PDX compression.
> + *
> + * Note there's an extra offset applied (directmap_base_pdx) on top of the
> + * regular PDX compression logic. Its purpose is to skip over the initial
> + * range of non-existing memory, should there be one.
> + *
> + * @param ma Machine address
> + * @return Virtual address mapped to `ma`
> + */
> +static inline void *maddr_to_virt(paddr_t ma)
> +{
> +    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - directmap_base_pdx) <
> +           (DIRECTMAP_SIZE >> PAGE_SHIFT));
> +    return (void *)(XENHEAP_VIRT_START -
> +                    (directmap_base_pdx << PAGE_SHIFT) +
> +                    maddr_to_directmapoff(ma));
> +}
> +#endif
> +
>  /*
>   * Print a walk of a page table or p2m
>   *
> 
> 
> 
>>> 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?
> 
> Sure
> 
>>
>>> 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.
> 
> Yes I took the one from arm64, here I probably need some help as there are 
> not too many
> informations about how to size this.
It depends on your estimate about max RAM size you want to support in MPU case.
32GB / 56B (size of page_info on Arm64) - tells you how many page_info structs
you can have. The above value * 4K - tells you amount of RAM supported.

~Michal