RE: [PATCH v2 05/40] xen/arm64: prepare for moving MMU related code from head.S

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2023年1月18日 17:50
> To: Wei Chen <Wei.Chen@xxxxxxx>; Penny Zheng <Penny.Zheng@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 05/40] xen/arm64: prepare for moving MMU related
> code from head.S
> 
> 
> 
> On 18/01/2023 03:09, Wei Chen wrote:
> > Hi Julien,
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: 2023年1月18日 7:37
> >> 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 05/40] xen/arm64: prepare for moving MMU related
> >> code from head.S
> >>
> >> Hi Penny,
> >>
> >> On 13/01/2023 05:28, Penny Zheng wrote:
> >>> From: Wei Chen <wei.chen@xxxxxxx>
> >>>
> >>> We want to reuse head.S for MPU systems, but there are some
> >>> code implemented for MMU systems only. We will move such
> >>> code to another MMU specific file. But before that, we will
> >>> do some preparations in this patch to make them easier
> >>> for reviewing:
> >>
> >> Well, I agree that...
> >>
> >>> 1. Fix the indentations of code comments.
> >>
> >> ... changing the indentation is better here. But...
> >>
> >>> 2. Export some symbols that will be accessed out of file
> >>>      scope.
> >>
> >> ... I have no idea which functions are going to be used in a separate
> >> file. So I think they should belong to the patch moving the code.
> >>
> >
> > Ok, I will move these changes to the moving code patches.
> >
> >>>
> >>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> >>
> >> Your signed-off-by is missing.
> >>
> >>> ---
> >>> v1 -> v2:
> >>> 1. New patch.
> >>> ---
> >>>    xen/arch/arm/arm64/head.S | 40 +++++++++++++++++++-----------------
> ---
> >>>    1 file changed, 20 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> >>> index 93f9b0b9d5..b2214bc5e3 100644
> >>> --- a/xen/arch/arm/arm64/head.S
> >>> +++ b/xen/arch/arm/arm64/head.S
> >>> @@ -136,22 +136,22 @@
> >>>            add \xb, \xb, x20
> >>>    .endm
> >>>
> >>> -        .section .text.header, "ax", %progbits
> >>> -        /*.aarch64*/
> >>> +.section .text.header, "ax", %progbits
> >>> +/*.aarch64*/
> >>
> >> This change is not mentioned.
> >>
> >
> > I will add the description in commit message.
> >
> >>>
> >>> -        /*
> >>> -         * Kernel startup entry point.
> >>> -         * ---------------------------
> >>> -         *
> >>> -         * The requirements are:
> >>> -         *   MMU = off, D-cache = off, I-cache = on or off,
> >>> -         *   x0 = physical address to the FDT blob.
> >>> -         *
> >>> -         * This must be the very first address in the loaded image.
> >>> -         * It should be linked at XEN_VIRT_START, and loaded at any
> >>> -         * 4K-aligned address.  All of text+data+bss must fit in 2MB,
> >>> -         * or the initial pagetable code below will need adjustment.
> >>> -         */
> >>> +/*
> >>> + * Kernel startup entry point.
> >>> + * ---------------------------
> >>> + *
> >>> + * The requirements are:
> >>> + *   MMU = off, D-cache = off, I-cache = on or off,
> >>> + *   x0 = physical address to the FDT blob.
> >>> + *
> >>> + * This must be the very first address in the loaded image.
> >>> + * It should be linked at XEN_VIRT_START, and loaded at any
> >>> + * 4K-aligned address.  All of text+data+bss must fit in 2MB,
> >>> + * or the initial pagetable code below will need adjustment.
> >>> + */
> >>>
> >>>    GLOBAL(start)
> >>>            /*
> >>> @@ -586,7 +586,7 @@ ENDPROC(cpu_init)
> >>>     *
> >>>     * Clobbers x0 - x4
> >>>     */
> >>> -create_page_tables:
> >>> +ENTRY(create_page_tables)
> >>
> >> I am not sure about keeping this name. Now we have create_page_tables()
> >> and arch_setup_page_tables().
> >>
> >> I would conside to name it create_boot_page_tables().
> >>
> >
> > Do you need me to rename it in this patch?
> 
> So looking at the rest of the series, I see you are already renaming the
> helper in patch #11. I think it would be better if the naming is done
> earlier.
> 
> That said, I am not convinced that create_page_tables() should actually
> be called externally.
> 
> In fact, you have something like:
> 
>     bl create_page_tables
>     bl enable_mmu
> 
> Both will need a MMU/MPU specific implementation. So it would be better
> if we provide a wrapper to limit the number of external functions.
>

I agree with you, we will try to wrapper some functions instead of
export them.

Cheers,
Wei Chen
 
> Cheers,
> 
> --
> Julien Grall