Re: [PATCH 2/5] arm/mpu: Implement setup_mm for MPU systems

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

On 30/07/2025 10:45, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> Implement `setup_mm` for MPU systems. This variant doesn't need to set
> up the direct map.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Hari Limaye <hari.limaye@xxxxxxx>
> ---
>  xen/arch/arm/mpu/mm.c | 64 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 6a16fa348d..0b05103180 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -8,9 +8,12 @@
>  #include <xen/sizes.h>
>  #include <xen/spinlock.h>
>  #include <xen/types.h>
> +#include <xen/static-memory.h>
> +#include <xen/static-shmem.h>
>  #include <asm/mpu.h>
>  #include <asm/mpu/mm.h>
>  #include <asm/page.h>
> +#include <asm/setup.h>
>  #include <asm/sysregs.h>
>  
>  struct page_info *frame_table;
> @@ -364,9 +367,68 @@ int map_pages_to_xen(unsigned long virt, mfn_t mfn, 
> unsigned long nr_mfns,
>      return xen_mpumap_update(virt, mfn_to_maddr(mfn_add(mfn, nr_mfns)), 
> flags);
>  }
>  
> +/*
> + * Heap must be statically configured in Device Tree through 
> "xen,static-heap"
> + * on MPU systems.
> + */
> +static void __init setup_staticheap_mappings(void)
> +{
> +    const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> +    unsigned int bank = 0;
> +
> +    for ( ; bank < reserved_mem->nr_banks; bank++ )
> +    {
> +        if ( reserved_mem->bank[bank].type == MEMBANK_STATIC_HEAP )
> +        {
> +            paddr_t bank_start = round_pgup(reserved_mem->bank[bank].start);
> +            paddr_t bank_size = round_pgdown(reserved_mem->bank[bank].size);
> +            paddr_t bank_end = bank_start + bank_size;
> +
> +            /* Map static heap with one MPU protection region */
> +            if ( xen_mpumap_update(bank_start, bank_end, PAGE_HYPERVISOR) )
> +                panic("Failed to map static heap\n");
> +
> +            break;
> +        }
> +    }
> +
> +    if ( bank == reserved_mem->nr_banks )
> +        panic("No static heap memory bank found\n");
> +}
> +
>  void __init setup_mm(void)
>  {
> -    BUG_ON("unimplemented");
> +    const struct membanks *mem = bootinfo_get_mem();
> +    paddr_t ram_start = INVALID_PADDR, ram_end = 0, ram_size = 0;
> +
> +    if ( !mem->nr_banks )
> +        panic("No memory bank\n");
> +
> +    init_pdx();
> +
> +    populate_boot_allocator();
> +
> +    total_pages = 0;
There's no other place setting up total_pages between this and += ram_size
so I consider it not needed assignment. And actually, this could be calculated
in init_pdx() next to max_page to avoid requiring each arch (arm32, arm64, mpu)
to set it exactly the same.

> +    for ( unsigned int bank = 0 ; bank < mem->nr_banks; bank++ )
; should be added right after a field, so bank = 0; > +    {
> +        paddr_t bank_start = round_pgup(mem->bank[bank].start);
> +        paddr_t bank_size = round_pgdown(mem->bank[bank].size);
> +        paddr_t bank_end = bank_start + bank_size;
> +
> +        ram_size = ram_size + bank_size;
> +        ram_start = min(ram_start, bank_start);
> +        ram_end = max(ram_end, bank_end);
> +    }
> +
> +    setup_staticheap_mappings();
> +
> +    total_pages += ram_size >> PAGE_SHIFT;
> +    max_page = PFN_DOWN(ram_end);
This is already calculated in init_pdx() after my recent changes

> +
> +    setup_frametable_mappings(ram_start, ram_end);
> +
> +    init_staticmem_pages();
> +    init_sharedmem_pages();
>  }
>  
>  int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)

~Michal