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? > > /* Prepare the page-tables for mapping Xen */ > > ldr x0, =XEN_VIRT_START > > create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3 > > @@ -680,7 +680,7 @@ ENDPROC(create_page_tables) > > * > > * Clobbers x0 - x3 > > */ > > -enable_mmu: > > +ENTRY(enable_mmu) > > PRINT("- Turning on paging -\r\n") > > > > /* > > @@ -714,7 +714,7 @@ ENDPROC(enable_mmu) > > * > > * Clobbers x0 - x1 > > */ > > -remove_identity_mapping: > > +ENTRY(remove_identity_mapping) > > Patch #14 should be before this patch. So you don't have to export > remove_identity_mapping temporarily. > > This will also avoid (transient) naming confusing with my work (see [1]). > Ok, we will do it. > > /* > > * Find the zeroeth slot used. Remove the entry from zeroeth > > * table if the slot is not XEN_ZEROETH_SLOT. > > @@ -775,7 +775,7 @@ ENDPROC(remove_identity_mapping) > > * > > * Clobbers x0 - x3 > > */ > > -setup_fixmap: > > +ENTRY(setup_fixmap) > > #ifdef CONFIG_EARLY_PRINTK > > /* Add UART to the fixmap table */ > > ldr x0, =EARLY_UART_VIRTUAL_ADDRESS > > @@ -871,7 +871,7 @@ ENDPROC(init_uart) > > * x0: Nul-terminated string to print. > > * x23: Early UART base address > > * Clobbers x0-x1 */ > > -puts: > > +ENTRY(puts) > > This name is a bit too generic to be globally exported. It is also now > quite confusing because we have "early_puts" and "puts". > > I would consider to name it asm_puts(). It is still not great but > hopefully it would give a hint that should be call from assembly code. > Yes, I had the same concern. I will rename it in next version. Cheers, Wei Chen > > early_uart_ready x23, 1 > > ldrb w1, [x0], #1 /* Load next char */ > > cbz w1, 1f /* Exit on nul */ > > Cheers, > > [1] https://lore.kernel.org/all/[email protected]/ > > -- > Julien Grall
