Hi Julien, > -----Original Message----- > From: Julien Grall <[email protected]> > Sent: 2023年1月18日 17:50 > To: Wei Chen <[email protected]>; Penny Zheng <[email protected]>; xen- > [email protected] > Cc: Stefano Stabellini <[email protected]>; Bertrand Marquis > <[email protected]>; Volodymyr Babchuk <[email protected]> > 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 <[email protected]> > >> Sent: 2023年1月18日 7:37 > >> To: Penny Zheng <[email protected]>; [email protected] > >> Cc: Wei Chen <[email protected]>; Stefano Stabellini > >> <[email protected]>; Bertrand Marquis <[email protected]>; > >> Volodymyr Babchuk <[email protected]> > >> 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 <[email protected]> > >>> > >>> 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 <[email protected]> > >> > >> 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
