Re: [PATCH v5 10/13] xen/arm: mmu: move MMU-specific setup_mm to mmu/setup.c

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/08/2023 05:25, Henry Wang wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> setup_mm is used for Xen to setup memory management subsystem at boot
> time, like boot allocator, direct-mapping, xenheap initialization,
> frametable and static memory pages.
>
> We could inherit some components seamlessly in later MPU system like
> boot allocator, whilst we need to implement some components differently
> in MPU, like xenheap, etc. There are some components that is specific
> to MMU only, like direct-mapping.
>
> In the commit, we move MMU-specific components into mmu/setup.c, in
> preparation of implementing MPU version of setup_mm later in future
> commit. Also, make init_pdx(), init_staticmem_pages(), setup_mm(), and
> populate_boot_allocator() public for future MPU inplementation.
Typo: s/inplementation/implementation/

> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> v5:
> - No change
> v4:
> - No change
> ---
>   xen/arch/arm/include/asm/setup.h |   5 +
>   xen/arch/arm/mmu/Makefile        |   1 +
>   xen/arch/arm/mmu/setup.c         | 339 +++++++++++++++++++++++++++++++
>   xen/arch/arm/setup.c             | 326 +----------------------------
>   4 files changed, 349 insertions(+), 322 deletions(-)
>   create mode 100644 xen/arch/arm/mmu/setup.c
>
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index f0f64d228c..0922549631 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -156,6 +156,11 @@ struct bootcmdline 
> *boot_cmdline_find_by_kind(bootmodule_kind kind);
>   struct bootcmdline * boot_cmdline_find_by_name(const char *name);
>   const char *boot_module_kind_as_string(bootmodule_kind kind);
>   
> +extern void init_pdx(void);
> +extern void init_staticmem_pages(void);
> +extern void populate_boot_allocator(void);
> +extern void setup_mm(void);
Please avoid the 'extern' for new function declaration.

> +
>   extern uint32_t hyp_traps_vector[];
>   void init_traps(void);
>   
> diff --git a/xen/arch/arm/mmu/Makefile b/xen/arch/arm/mmu/Makefile
> index b18cec4836..4aa1fb466d 100644
> --- a/xen/arch/arm/mmu/Makefile
> +++ b/xen/arch/arm/mmu/Makefile
> @@ -1 +1,2 @@
>   obj-y += mm.o
> +obj-y += setup.o
> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> new file mode 100644
> index 0000000000..e05cca3f86
> --- /dev/null
> +++ b/xen/arch/arm/mmu/setup.c
> @@ -0,0 +1,339 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * xen/arch/arm/mmu/setup.c
> + *
> + * MMU-specific early bringup code for an ARMv7-A with virt extensions.
You modify the comment in arm/setup.c to mention ARMv8 but not here. The 
former feels somewhat unrelated.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/serial.h>
> +#include <xen/libfdt/libfdt-xen.h>
> +#include <xen/mm.h>
> +#include <xen/param.h>
> +#include <xen/pfn.h>
> +#include <asm/fixmap.h>
> +#include <asm/page.h>
> +#include <asm/setup.h>
> +
> +#ifdef CONFIG_ARM_32
AFAICT, mmu/mm.c has nothing common between arm32 and arm64. So can we 
introduce arm{32, 64}/mmu/setup.c and move the respective code there?
[...]

> +void __init setup_mm(void)
> +{
> +    paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
> +    paddr_t static_heap_end = 0, static_heap_size = 0;
> +    unsigned long heap_pages, xenheap_pages, domheap_pages;
> +    unsigned int i;
> +    const uint32_t ctr = READ_CP32(CTR);
> +
> +    if ( !bootinfo.mem.nr_banks )
> +        panic("No memory bank\n");
> +
> +    /* We only supports instruction caches implementing the IVIPT extension. */
> +    if ( ((ctr >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK) == ICACHE_POLICY_AIVIVT )
> +        panic("AIVIVT instruction cache not supported\n");
Thi check is unlikely going to be MMU setup.c.

> +
> +    init_pdx();
> +
> +    ram_start = bootinfo.mem.bank[0].start;
> +    ram_size  = bootinfo.mem.bank[0].size;
> +    ram_end   = ram_start + ram_size;
> +
> +    for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
> +    {
> +        bank_start = bootinfo.mem.bank[i].start;
> +        bank_size = bootinfo.mem.bank[i].size;
> +        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);
> +    }
> +
> +    total_pages = ram_size >> PAGE_SHIFT;
> +
> +    if ( bootinfo.static_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
> +                continue;
> +
> +            bank_start = bootinfo.reserved_mem.bank[i].start;
> +            bank_size = bootinfo.reserved_mem.bank[i].size;
> +            bank_end = bank_start + bank_size;
> +
> +            static_heap_size += bank_size;
> +            static_heap_end = max(static_heap_end, bank_end);
> +        }
> +
> +        heap_pages = static_heap_size >> PAGE_SHIFT;
> +    }
> +    else
> +        heap_pages = total_pages;
> +
> +    /*
> +     * If the user has not requested otherwise via the command line
> +     * then locate the xenheap using these constraints:
> +     *
> +     *  - must be contiguous
> +     *  - must be 32 MiB aligned
> +     *  - must not include Xen itself or the boot modules
> +     *  - must be at most 1GB or 1/32 the total RAM in the system (or static
> +          heap if enabled) if less
While you are moving the code can you add '*'? Could be done as a 
follow-up patch as well.
[...[

> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 44ccea03ca..b3dea41099 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -2,7 +2,7 @@
>   /*
>    * xen/arch/arm/setup.c
>    *
> - * Early bringup code for an ARMv7-A with virt extensions.
> + * Early bringup code for an ARMv7-A/ARM64v8R with virt extensions.
You are not yet supporting ARMv8-R. But we are supporting ARMv8-A which 
is not mentioned here. That said, I don't really the benefits of 
mentioning the major revision of the Arm Arm we support. For instance, 
we are likely going to be able to boot on Armv9.
So I would just remove anything from 'for'.

Cheers,

--
Julien Grall