| Message ID | 20221022150422.17707-3-julien@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Don't switch TTBR while the MMU is on | expand |
Hi Julien, On 22/10/2022 17:04, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > At the moment, enable_mmu() will return to an address in the 1:1 mapping > and each path are responsible to switch to the runtime mapping. s/are/is/ > > In a follow-up patch, the behavior to switch to the runtime mapping > will become more complex. So to avoid more code/comment duplication, > move the switch in enable_mmu(). > > Lastly, take the opportunity to replace load from literal pool with > mov_w. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm32/head.S | 51 ++++++++++++++++++++++++--------------- > 1 file changed, 31 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index a558c2a6876e..163bd6596dec 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -167,19 +167,12 @@ past_zImage: > bl check_cpu_mode > bl cpu_init > bl create_page_tables > - bl enable_mmu > > - /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > - ldr r0, =primary_switched > - mov pc, r0 > + /* Address in the runtime mapping to jump to after the MMU is enabled */ > + mov_w lr, primary_switched We seem to still widely use ldr instead of mov_w which is faster. It looks like a prerequisite patch to convert all occurences or something to keep in a backlog. > + b enable_mmu > + > primary_switched: > - /* > - * The 1:1 map may clash with other parts of the Xen virtual memory > - * layout. As it is not used anymore, remove it completely to > - * avoid having to worry about replacing existing mapping > - * afterwards. > - */ > - bl remove_identity_mapping > bl setup_fixmap > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -223,12 +216,10 @@ GLOBAL(init_secondary) > bl check_cpu_mode > bl cpu_init > bl create_page_tables > - bl enable_mmu > - > > - /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > - ldr r0, =secondary_switched > - mov pc, r0 > + /* Address in the runtime mapping to jump to after the MMU is enabled */ > + mov_w lr, secondary_switched > + b enable_mmu > secondary_switched: > /* > * Non-boot CPUs need to move on to the proper pagetables, which were > @@ -523,9 +514,12 @@ virtphys_clash: > ENDPROC(create_page_tables) > > /* > - * Turn on the Data Cache and the MMU. The function will return on the 1:1 > - * mapping. In other word, the caller is responsible to switch to the runtime > - * mapping. > + * Turn on the Data Cache and the MMU. The function will return > + * to the virtual address provided in LR (e.g. the runtime mapping). > + * > + * Inputs: > + * r9 : paddr(start) > + * lr : Virtual address to return to > * > * Clobbers r0 - r3 > */ > @@ -551,7 +545,24 @@ enable_mmu: > dsb /* Flush PTE writes and finish reads */ > mcr CP32(r0, HSCTLR) /* now paging is enabled */ > isb /* Now, flush the icache */ > - mov pc, lr > + > + /* > + * The MMU is turned on and we are in the 1:1 mapping. Switch > + * to the runtime mapping. > + */ > + mov_w r0, 1f > + mov pc, r0 Would it be possible to stop using: mov pc, reg in favor of using: bx reg Some time ago I saw this commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm/include/asm/assembler.h?id=6ebbf2ce437b33022d30badd49dc94d33ecfa498 which states that bx is faster. > +1: > + /* > + * The 1:1 map may clash with other parts of the Xen virtual memory > + * layout. As it is not used anymore, remove it completely to > + * avoid having to worry about replacing existing mapping > + * afterwards. > + * > + * On return this will jump to the virtual address requested by > + * the caller. > + */ > + b remove_identity_mapping > ENDPROC(enable_mmu) > > /* > -- > 2.37.1 > > Apart from that, the change looks ok. ~Michal
Hi, On 25/10/2022 10:45, Michal Orzel wrote: > Hi Julien, > > On 22/10/2022 17:04, Julien Grall wrote: >> >> >> From: Julien Grall <jgrall@amazon.com> >> >> At the moment, enable_mmu() will return to an address in the 1:1 mapping >> and each path are responsible to switch to the runtime mapping. > s/are/is/ > >> >> In a follow-up patch, the behavior to switch to the runtime mapping >> will become more complex. So to avoid more code/comment duplication, >> move the switch in enable_mmu(). >> >> Lastly, take the opportunity to replace load from literal pool with >> mov_w. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> xen/arch/arm/arm32/head.S | 51 ++++++++++++++++++++++++--------------- >> 1 file changed, 31 insertions(+), 20 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index a558c2a6876e..163bd6596dec 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -167,19 +167,12 @@ past_zImage: >> bl check_cpu_mode >> bl cpu_init >> bl create_page_tables >> - bl enable_mmu >> >> - /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ >> - ldr r0, =primary_switched >> - mov pc, r0 >> + /* Address in the runtime mapping to jump to after the MMU is enabled */ >> + mov_w lr, primary_switched > We seem to still widely use ldr instead of mov_w which is faster. This is boot code and personally I don't think the speed will be noticeable here. The reason I am switching to mov_w is because we don't need to use the literal pool. The literal pool add other issues when you think about the 1:1 mapping. > It looks like a prerequisite patch to convert all occurences or something to keep in a backlog I have some plan for that but decided to keep that outside of this series. > >> + b enable_mmu >> + >> primary_switched: >> - /* >> - * The 1:1 map may clash with other parts of the Xen virtual memory >> - * layout. As it is not used anymore, remove it completely to >> - * avoid having to worry about replacing existing mapping >> - * afterwards. >> - */ >> - bl remove_identity_mapping >> bl setup_fixmap >> #ifdef CONFIG_EARLY_PRINTK >> /* Use a virtual address to access the UART. */ >> @@ -223,12 +216,10 @@ GLOBAL(init_secondary) >> bl check_cpu_mode >> bl cpu_init >> bl create_page_tables >> - bl enable_mmu >> - >> >> - /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ >> - ldr r0, =secondary_switched >> - mov pc, r0 >> + /* Address in the runtime mapping to jump to after the MMU is enabled */ >> + mov_w lr, secondary_switched >> + b enable_mmu >> secondary_switched: >> /* >> * Non-boot CPUs need to move on to the proper pagetables, which were >> @@ -523,9 +514,12 @@ virtphys_clash: >> ENDPROC(create_page_tables) >> >> /* >> - * Turn on the Data Cache and the MMU. The function will return on the 1:1 >> - * mapping. In other word, the caller is responsible to switch to the runtime >> - * mapping. >> + * Turn on the Data Cache and the MMU. The function will return >> + * to the virtual address provided in LR (e.g. the runtime mapping). >> + * >> + * Inputs: >> + * r9 : paddr(start) >> + * lr : Virtual address to return to >> * >> * Clobbers r0 - r3 >> */ >> @@ -551,7 +545,24 @@ enable_mmu: >> dsb /* Flush PTE writes and finish reads */ >> mcr CP32(r0, HSCTLR) /* now paging is enabled */ >> isb /* Now, flush the icache */ >> - mov pc, lr >> + >> + /* >> + * The MMU is turned on and we are in the 1:1 mapping. Switch >> + * to the runtime mapping. >> + */ >> + mov_w r0, 1f >> + mov pc, r0 > Would it be possible to stop using: > mov pc, reg > in favor of using: > bx reg > > Some time ago I saw this commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm/include/asm/assembler.h?id=6ebbf2ce437b33022d30badd49dc94d33ecfa498 > which states that bx is faster. See above about my statement regarding fast. I don't see the benefits for early boot code. So I am not keen to switch to 'bx reg' here. Cheers,
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index a558c2a6876e..163bd6596dec 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -167,19 +167,12 @@ past_zImage: bl check_cpu_mode bl cpu_init bl create_page_tables - bl enable_mmu - /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ - ldr r0, =primary_switched - mov pc, r0 + /* Address in the runtime mapping to jump to after the MMU is enabled */ + mov_w lr, primary_switched + b enable_mmu + primary_switched: - /* - * The 1:1 map may clash with other parts of the Xen virtual memory - * layout. As it is not used anymore, remove it completely to - * avoid having to worry about replacing existing mapping - * afterwards. - */ - bl remove_identity_mapping bl setup_fixmap #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ @@ -223,12 +216,10 @@ GLOBAL(init_secondary) bl check_cpu_mode bl cpu_init bl create_page_tables - bl enable_mmu - - /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ - ldr r0, =secondary_switched - mov pc, r0 + /* Address in the runtime mapping to jump to after the MMU is enabled */ + mov_w lr, secondary_switched + b enable_mmu secondary_switched: /* * Non-boot CPUs need to move on to the proper pagetables, which were @@ -523,9 +514,12 @@ virtphys_clash: ENDPROC(create_page_tables) /* - * Turn on the Data Cache and the MMU. The function will return on the 1:1 - * mapping. In other word, the caller is responsible to switch to the runtime - * mapping. + * Turn on the Data Cache and the MMU. The function will return + * to the virtual address provided in LR (e.g. the runtime mapping). + * + * Inputs: + * r9 : paddr(start) + * lr : Virtual address to return to * * Clobbers r0 - r3 */ @@ -551,7 +545,24 @@ enable_mmu: dsb /* Flush PTE writes and finish reads */ mcr CP32(r0, HSCTLR) /* now paging is enabled */ isb /* Now, flush the icache */ - mov pc, lr + + /* + * The MMU is turned on and we are in the 1:1 mapping. Switch + * to the runtime mapping. + */ + mov_w r0, 1f + mov pc, r0 +1: + /* + * The 1:1 map may clash with other parts of the Xen virtual memory + * layout. As it is not used anymore, remove it completely to + * avoid having to worry about replacing existing mapping + * afterwards. + * + * On return this will jump to the virtual address requested by + * the caller. + */ + b remove_identity_mapping ENDPROC(enable_mmu) /*