Hi,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Monday, February 6, 2023 5:30 AM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>
> 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.c...@arm.com>
> >
> > 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.c...@arm.com>
> > Signed-off-by: Penny Zheng <penny.zh...@arm.com>
> > ---
> > 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
