RE: [PATCH v2 15/40] xen/arm: move MMU-specific memory management code to mm_mmu.c/mm_mmu.h

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, February 6, 2023 5:30 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 15/40] xen/arm: move MMU-specific memory
> management code to mm_mmu.c/mm_mmu.h
> 
> Hi,
> 
> On 13/01/2023 05:28, Penny Zheng wrote:
> > From: Wei Chen <wei.chen@xxxxxxx>
> >
> > To make the code readable and maintainable, we move MMU-specific
> > memory management code from mm.c to mm_mmu.c and move MMU-
> specific
> > definitions from mm.h to mm_mmu.h.
> > Later we will create mm_mpu.h and mm_mpu.c for MPU-specific memory
> > management code.
> 
> This sentence implies there is no mm_mpu.{c, h} yet and this is not touched
> within this patch. However...
> 
> 
> > This will avoid lots of #ifdef in memory management code and header files.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   xen/arch/arm/Makefile             |    5 +
> >   xen/arch/arm/include/asm/mm.h     |   19 +-
> >   xen/arch/arm/include/asm/mm_mmu.h |   35 +
> >   xen/arch/arm/mm.c                 | 1352 +---------------------------
> >   xen/arch/arm/mm_mmu.c             | 1376
> +++++++++++++++++++++++++++++
> >   xen/arch/arm/mm_mpu.c             |   67 ++
> 
> ... It looks like they already exists and you are modifying them. That
> said, it would be better if this patch only contains code movement (IOW
> no MPU changes).
> 
> >   6 files changed, 1488 insertions(+), 1366 deletions(-)
> >   create mode 100644 xen/arch/arm/include/asm/mm_mmu.h
> >   create mode 100644 xen/arch/arm/mm_mmu.c
> 
> I don't particular like the naming. I think it would make more sense to
> introduce two directories: "mmu" and "mpu" which includes code specific
> to each flavor of Xen.
> 
[...]
> >
> > -
> > -/* Release all __init and __initdata ranges to be reused */
> > -void free_init_memory(void)
> 
> This function doesn't look specific to the MMU.
> 

Functions like, early_fdt_map[1] / setup_frametable_mappings[2] / 
free_init_memory [3] ...
they both share quite the same logic as MMU does in MPU system, the difference 
could only
be address translation regime. Still, in order to avoid putting too much #ifdef 
here and there,
I implement different MMU and MPU version of them.
 
Or I keep them in generic file here, then in future commits when we implement 
MPU version
of them(I list related commits below), I transfer them to MMU file there.

Wdyt?

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00774.html 
[2] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00787.html  
[3] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00786.html 

> > -{
> > -    paddr_t pa = virt_to_maddr(__init_begin);
> > -    unsigned long len = __init_end - __init_begin;
> > -    uint32_t insn;
> > -    unsigned int i, nr = len / sizeof(insn);
> > -    uint32_t *p;
> > -    int rc;
> > -
> > -    rc = modify_xen_mappings((unsigned long)__init_begin,
> > -                             (unsigned long)__init_end, 
> > PAGE_HYPERVISOR_RW);
> > -    if ( rc )
> > -        panic("Unable to map RW the init section (rc = %d)\n", rc);
> > -
> > -    /*
> > -     * From now on, init will not be used for execution anymore,
> > -     * so nuke the instruction cache to remove entries related to init.
> > -     */
> > -    invalidate_icache_local();
> > -
> > -#ifdef CONFIG_ARM_32
> > -    /* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
> > -    insn = 0xe7f000f0;
> > -#else
> > -    insn = AARCH64_BREAK_FAULT;
> > -#endif
> > -    p = (uint32_t *)__init_begin;
> > -    for ( i = 0; i < nr; i++ )
> > -        *(p + i) = insn;
> > -
> > -    rc = destroy_xen_mappings((unsigned long)__init_begin,
> > -                              (unsigned long)__init_end);
> > -    if ( rc )
> > -        panic("Unable to remove the init section (rc = %d)\n", rc);
> > -
> > -    init_domheap_pages(pa, pa + len);
> > -    printk("Freed %ldkB init memory.\n", (long)(__init_end-
> __init_begin)>>10);
> > -}
> > -
> 
> 
> [...]
> 
> > diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
> > index 43e9a1be4d..87a12042cc 100644
> > --- a/xen/arch/arm/mm_mpu.c
> > +++ b/xen/arch/arm/mm_mpu.c
> > @@ -20,8 +20,10 @@
> >    */
> >
> >   #include <xen/init.h>
> > +#include <xen/mm.h>
> >   #include <xen/page-size.h>
> >   #include <asm/arm64/mpu.h>
> > +#include <asm/page.h>
> 
> Regardless of what I wrote above, none of the code you add seems to
> require <asm/page.h>
> 
> >
> >   /* Xen MPU memory region mapping table. */
> >   pr_t __aligned(PAGE_SIZE) __section(".data.page_aligned")
> > @@ -38,6 +40,71 @@ uint64_t __ro_after_init next_transient_region_idx;
> >   /* Maximum number of supported MPU memory regions by the EL2 MPU.
> */
> >   uint64_t __ro_after_init max_xen_mpumap;
> >
> > +/* TODO: Implementation on the first usage */
> 
> It is not clear what you mean given there are some callers.
> 
> > +void dump_hyp_walk(vaddr_t addr)
> > +{
> 
> Please add ASSERT_UNREACHABLE() for any dummy helper you have
> introduced
> any plan to implement later. This will be helpful to track down any
> function you haven't implemented.
> 
> 
> > +}
> > +
> > +void * __init early_fdt_map(paddr_t fdt_paddr)
> > +{
> > +    return NULL;
> > +}
> > +
> > +void __init remove_early_mappings(void)
> > +{
> 
> Ditto
> 
> > +}
> > +
> > +int init_secondary_pagetables(int cpu)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> > +void mmu_init_secondary_cpu(void)
> > +{
> 
> Ditto. The name of the function is also a bit odd given this is an MPU
> specific file. This likely want to be renamed to mm_init_secondary_cpu().
> 
> > +}
> > +
> > +void *ioremap_attr(paddr_t pa, size_t len, unsigned int attributes)
> > +{
> > +    return NULL;
> > +}
> > +
> > +void *ioremap(paddr_t pa, size_t len)
> > +{
> > +    return NULL;
> > +}
> > +
> > +int map_pages_to_xen(unsigned long virt,
> > +                     mfn_t mfn,
> > +                     unsigned long nr_mfns,
> > +                     unsigned int flags)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> > +int destroy_xen_mappings(unsigned long s, unsigned long e)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> > +int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int
> flags)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> > +void free_init_memory(void)
> > +{
> 
> Ditto.
> 
> > +}
> > +
> > +int xenmem_add_to_physmap_one(
> > +    struct domain *d,
> > +    unsigned int space,
> > +    union add_to_physmap_extra extra,
> > +    unsigned long idx,
> > +    gfn_t gfn)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> >   /*
> >    * Local variables:
> >    * mode: C
> 
> --
> Julien Grall