| Message ID | 20231101233011.83098-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm32: Improve logging during early boot | expand |
Hi Julien, > On Nov 2, 2023, at 07:30, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the > temporary mapping"), boot_second (used to cover regions like Xen and > the fixmap) will not be mapped if the identity mapping overlap. > > So it is ok to prepare the fixmap table and link it in boot_second > earlier. With that, the fixmap can also be used earlier via the > temporary mapping. > > Therefore split setup_fixmap() in two: > * The table is now linked in create_page_tables() because > the boot page tables needs to be recreated for every CPU. > * The early UART mapping is only added for the boot CPU0 as the > fixmap table is not cleared when secondary CPUs boot. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm32/head.S | 48 ++++++++------------------------------- > 1 file changed, 9 insertions(+), 39 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 33b038e7e0ca..fec2433e568f 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -183,12 +183,16 @@ past_zImage: > bl check_cpu_mode > bl cpu_init > bl create_page_tables > + /* Add the UART mapping if requested */ > +#ifdef CONFIG_EARLY_PRINTK > + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS > + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3 I tried to test this series today and found a corner case in this patch: If we build Xen only with this patch, and CONFIG_EARLY_PRINTK=y, we will get below from compiler: ``` arch/arm/arm32/head.S: Assembler messages: arch/arm/arm32/head.S:189: Error: bad instruction `create_mapping_entry xen_fixmap,r0,r11,type=0xe73' make[3]: *** [Rules.mk:253: arch/arm/arm32/head.o] Error 1 ``` With the second patch applied, the above error will disappear and I confirm arm32 will work fine with both patch applied + either CONFIG_EARLY_PRINTK={y,n}. Kind regards, Henry
Hi Julien, On 02/11/2023 00:30, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the > temporary mapping"), boot_second (used to cover regions like Xen and > the fixmap) will not be mapped if the identity mapping overlap. > > So it is ok to prepare the fixmap table and link it in boot_second > earlier. With that, the fixmap can also be used earlier via the > temporary mapping. > > Therefore split setup_fixmap() in two: > * The table is now linked in create_page_tables() because > the boot page tables needs to be recreated for every CPU. > * The early UART mapping is only added for the boot CPU0 as the > fixmap table is not cleared when secondary CPUs boot. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm32/head.S | 48 ++++++++------------------------------- > 1 file changed, 9 insertions(+), 39 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 33b038e7e0ca..fec2433e568f 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -183,12 +183,16 @@ past_zImage: > bl check_cpu_mode > bl cpu_init > bl create_page_tables > + /* Add the UART mapping if requested */ > +#ifdef CONFIG_EARLY_PRINTK > + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS > + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3 When building this patch, I'm getting a "Bad instruction" build error. Quick look shows that you are trying to use the macro create_mapping_entry before macro definition. Unless, I'm using a different baseline (latest staging) than you, this needs to be fixed. Apart from that: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi, On 02/11/2023 02:29, Henry Wang wrote: > Hi Julien, > >> On Nov 2, 2023, at 07:30, Julien Grall <julien@xen.org> wrote: >> >> From: Julien Grall <jgrall@amazon.com> >> >> Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the >> temporary mapping"), boot_second (used to cover regions like Xen and >> the fixmap) will not be mapped if the identity mapping overlap. >> >> So it is ok to prepare the fixmap table and link it in boot_second >> earlier. With that, the fixmap can also be used earlier via the >> temporary mapping. >> >> Therefore split setup_fixmap() in two: >> * The table is now linked in create_page_tables() because >> the boot page tables needs to be recreated for every CPU. >> * The early UART mapping is only added for the boot CPU0 as the >> fixmap table is not cleared when secondary CPUs boot. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> xen/arch/arm/arm32/head.S | 48 ++++++++------------------------------- >> 1 file changed, 9 insertions(+), 39 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index 33b038e7e0ca..fec2433e568f 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -183,12 +183,16 @@ past_zImage: >> bl check_cpu_mode >> bl cpu_init >> bl create_page_tables >> + /* Add the UART mapping if requested */ >> +#ifdef CONFIG_EARLY_PRINTK >> + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS >> + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3 > > I tried to test this series today and found a corner case in this patch: > > If we build Xen only with this patch, and CONFIG_EARLY_PRINTK=y, we > will get below from compiler: > > ``` > arch/arm/arm32/head.S: Assembler messages: > arch/arm/arm32/head.S:189: Error: bad instruction `create_mapping_entry xen_fixmap,r0,r11,type=0xe73' > make[3]: *** [Rules.mk:253: arch/arm/arm32/head.o] Error 1 > ``` > > With the second patch applied, the above error will disappear and I confirm > arm32 will work fine with both patch applied + either CONFIG_EARLY_PRINTK={y,n}. Thanks for testing. Yes I merged one hunk into the wrong patch. I will re-order the code. Cheers,
Hi Michal, On 02/11/2023 09:29, Michal Orzel wrote: > On 02/11/2023 00:30, Julien Grall wrote: >> >> >> From: Julien Grall <jgrall@amazon.com> >> >> Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the >> temporary mapping"), boot_second (used to cover regions like Xen and >> the fixmap) will not be mapped if the identity mapping overlap. >> >> So it is ok to prepare the fixmap table and link it in boot_second >> earlier. With that, the fixmap can also be used earlier via the >> temporary mapping. >> >> Therefore split setup_fixmap() in two: >> * The table is now linked in create_page_tables() because >> the boot page tables needs to be recreated for every CPU. >> * The early UART mapping is only added for the boot CPU0 as the >> fixmap table is not cleared when secondary CPUs boot. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> xen/arch/arm/arm32/head.S | 48 ++++++++------------------------------- >> 1 file changed, 9 insertions(+), 39 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index 33b038e7e0ca..fec2433e568f 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -183,12 +183,16 @@ past_zImage: >> bl check_cpu_mode >> bl cpu_init >> bl create_page_tables >> + /* Add the UART mapping if requested */ >> +#ifdef CONFIG_EARLY_PRINTK >> + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS >> + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3 > When building this patch, I'm getting a "Bad instruction" build error. > Quick look shows that you are trying to use the macro create_mapping_entry before macro definition. > Unless, I'm using a different baseline (latest staging) than you, this needs to be fixed. You are using the correct baseline. I have just ended up fix the build in patch #2 by mistake. I will move the change here. > > Apart from that: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Thanks! Cheers,
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 33b038e7e0ca..fec2433e568f 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -183,12 +183,16 @@ past_zImage: bl check_cpu_mode bl cpu_init bl create_page_tables + /* Add the UART mapping if requested */ +#ifdef CONFIG_EARLY_PRINTK + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3 +#endif /* Address in the runtime mapping to jump to after the MMU is enabled */ mov_w lr, primary_switched b enable_mmu primary_switched: - bl setup_fixmap #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ mov_w r11, EARLY_UART_VIRTUAL_ADDRESS @@ -456,11 +460,6 @@ ENDPROC(cpu_init) * Rebuild the boot pagetable's first-level entries. The structure * is described in mm.c. * - * After the CPU enables paging it will add the fixmap mapping - * to these page tables, however this may clash with the 1:1 - * mapping. So each CPU must rebuild the page tables here with - * the 1:1 in place. - * * Inputs: * r9 : paddr(start) * r10: phys offset @@ -488,6 +487,10 @@ create_page_tables: add r5, r5, #PAGE_SIZE /* r5 := Next table */ .endr + /* Map the fixmap into boot_second */ + mov_w r0, FIXMAP_ADDR(0) + create_table_entry boot_second, xen_fixmap, r0, 2 + /* * Find the size of Xen in pages and multiply by the size of a * PTE. This will then be compared in the mapping loop below. @@ -718,39 +721,6 @@ remove_temporary_mapping: mov pc, lr ENDPROC(remove_temporary_mapping) -/* - * Map the UART in the fixmap (when earlyprintk is used) and hook the - * fixmap table in the page tables. - * - * The fixmap cannot be mapped in create_page_tables because it may - * clash with the 1:1 mapping. - * - * Inputs: - * r10: Physical offset - * r11: Early UART base physical address - * - * Clobbers r0 - r4 - */ -setup_fixmap: -#if defined(CONFIG_EARLY_PRINTK) - /* Add UART to the fixmap table */ - mov_w r0, EARLY_UART_VIRTUAL_ADDRESS - create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3 -#endif - /* Map fixmap into boot_second */ - mov_w r0, FIXMAP_ADDR(0) - create_table_entry boot_second, xen_fixmap, r0, 2 - /* Ensure any page table updates made above have occurred. */ - dsb nshst - /* - * The fixmap area will be used soon after. So ensure no hardware - * translation happens before the dsb completes. - */ - isb - - mov pc, lr -ENDPROC(setup_fixmap) - /* * Setup the initial stack and jump to the C world *