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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 07/02/2023 03:59, Penny Zheng wrote:
> > -----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] ...
I looked at setup_frametable_mappings() and didn't think it was possible 
to share much code. But I agree for early_fdt_map and free_init_memory().
> 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.
I am not sure why you would need to put #ifdef in the code. Looking at 
it, there is usually only a chunk that is different for the mapping. So 
you could provide an helper that will be implemented in the MMU/MPU code.
>   
> 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.
I am not entirely sure. In one way it help to figure out the commonality 
but on the other way we are mixing code movement and new code.
In this case, the code movement would probably small, so that might be 
better for the review.
Cheers,

--
Julien Grall