| Message ID | 20230127195508.2786-6-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 27 Jan 2023, at 20:55, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > Switching TTBR while the MMU is on is not safe. Now that the identity > mapping will not clash with the rest of the memory layout, we can avoid > creating temporary page-tables every time a CPU is brought up. > > The arm32 code will use a different approach. So this issue is for now > only resolved on arm64. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Tested-by: Luca Fancellu <luca.fancellu@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com <mailto:bertrand.marquis@arm.com>> Cheers Bertrand > > --- > Changes in v5: > - Add Luca's reviewed-by and tested-by tags. > > Changes in v4: > - Somehow I forgot to send it in v3. So re-include it. > > Changes in v2: > - Remove arm32 code > --- > xen/arch/arm/arm32/smpboot.c | 4 ++++ > xen/arch/arm/arm64/head.S | 29 +++++++++-------------------- > xen/arch/arm/arm64/smpboot.c | 15 ++++++++++++++- > xen/arch/arm/include/asm/smp.h | 1 + > xen/arch/arm/smpboot.c | 1 + > 5 files changed, 29 insertions(+), 21 deletions(-) > > diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c > index e7368665d50d..518e9f9c7e70 100644 > --- a/xen/arch/arm/arm32/smpboot.c > +++ b/xen/arch/arm/arm32/smpboot.c > @@ -21,6 +21,10 @@ int arch_cpu_up(int cpu) > return platform_cpu_up(cpu); > } > > +void arch_cpu_up_finish(void) > +{ > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 5efd442b24af..a61b4d3c2738 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -308,6 +308,7 @@ real_start_efi: > bl check_cpu_mode > bl cpu_init > bl create_page_tables > + load_paddr x0, boot_pgtable > bl enable_mmu > > /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > @@ -365,29 +366,14 @@ GLOBAL(init_secondary) > #endif > bl check_cpu_mode > bl cpu_init > - bl create_page_tables > + load_paddr x0, init_ttbr > + ldr x0, [x0] > bl enable_mmu > > /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > ldr x0, =secondary_switched > br x0 > secondary_switched: > - /* > - * Non-boot CPUs need to move on to the proper pagetables, which were > - * setup in init_secondary_pagetables. > - * > - * XXX: This is not compliant with the Arm Arm. > - */ > - ldr x4, =init_ttbr /* VA of TTBR0_EL2 stashed by CPU 0 */ > - ldr x4, [x4] /* Actual value */ > - dsb sy > - msr TTBR0_EL2, x4 > - dsb sy > - isb > - tlbi alle2 > - dsb sy /* Ensure completion of TLB flush */ > - isb > - > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > @@ -672,9 +658,13 @@ ENDPROC(create_page_tables) > * mapping. In other word, the caller is responsible to switch to the runtime > * mapping. > * > - * Clobbers x0 - x3 > + * Inputs: > + * x0 : Physical address of the page tables. > + * > + * Clobbers x0 - x4 > */ > enable_mmu: > + mov x4, x0 > PRINT("- Turning on paging -\r\n") > > /* > @@ -685,8 +675,7 @@ enable_mmu: > dsb nsh > > /* Write Xen's PT's paddr into TTBR0_EL2 */ > - load_paddr x0, boot_pgtable > - msr TTBR0_EL2, x0 > + msr TTBR0_EL2, x4 > isb > > mrs x0, SCTLR_EL2 > diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c > index 694fbf67e62a..9637f424699e 100644 > --- a/xen/arch/arm/arm64/smpboot.c > +++ b/xen/arch/arm/arm64/smpboot.c > @@ -106,10 +106,23 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn) > > int arch_cpu_up(int cpu) > { > + int rc; > + > if ( !smp_enable_ops[cpu].prepare_cpu ) > return -ENODEV; > > - return smp_enable_ops[cpu].prepare_cpu(cpu); > + update_identity_mapping(true); > + > + rc = smp_enable_ops[cpu].prepare_cpu(cpu); > + if ( rc ) > + update_identity_mapping(false); > + > + return rc; > +} > + > +void arch_cpu_up_finish(void) > +{ > + update_identity_mapping(false); > } > > /* > diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h > index 8133d5c29572..a37ca55bff2c 100644 > --- a/xen/arch/arm/include/asm/smp.h > +++ b/xen/arch/arm/include/asm/smp.h > @@ -25,6 +25,7 @@ extern void noreturn stop_cpu(void); > extern int arch_smp_init(void); > extern int arch_cpu_init(int cpu, struct dt_device_node *dn); > extern int arch_cpu_up(int cpu); > +extern void arch_cpu_up_finish(void); > > int cpu_up_send_sgi(int cpu); > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 412ae2286906..4a89b3a8345b 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -500,6 +500,7 @@ int __cpu_up(unsigned int cpu) > init_data.cpuid = ~0; > smp_up_cpu = MPIDR_INVALID; > clean_dcache(smp_up_cpu); > + arch_cpu_up_finish(); > > if ( !cpu_online(cpu) ) > { > -- > 2.38.1 > >
> On 23 Feb 2023, at 15:06, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Julien, > >> On 27 Jan 2023, at 20:55, Julien Grall <julien@xen.org> wrote: >> >> From: Julien Grall <jgrall@amazon.com> >> >> Switching TTBR while the MMU is on is not safe. Now that the identity >> mapping will not clash with the rest of the memory layout, we can avoid >> creating temporary page-tables every time a CPU is brought up. >> >> The arm32 code will use a different approach. So this issue is for now >> only resolved on arm64. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> >> Tested-by: Luca Fancellu <luca.fancellu@arm.com> > > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com <mailto:bertrand.marquis@arm.com>> Sorry for that. Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > > Cheers > Bertrand > >> >> --- >> Changes in v5: >> - Add Luca's reviewed-by and tested-by tags. >> >> Changes in v4: >> - Somehow I forgot to send it in v3. So re-include it. >> >> Changes in v2: >> - Remove arm32 code >> --- >> xen/arch/arm/arm32/smpboot.c | 4 ++++ >> xen/arch/arm/arm64/head.S | 29 +++++++++-------------------- >> xen/arch/arm/arm64/smpboot.c | 15 ++++++++++++++- >> xen/arch/arm/include/asm/smp.h | 1 + >> xen/arch/arm/smpboot.c | 1 + >> 5 files changed, 29 insertions(+), 21 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c >> index e7368665d50d..518e9f9c7e70 100644 >> --- a/xen/arch/arm/arm32/smpboot.c >> +++ b/xen/arch/arm/arm32/smpboot.c >> @@ -21,6 +21,10 @@ int arch_cpu_up(int cpu) >> return platform_cpu_up(cpu); >> } >> >> +void arch_cpu_up_finish(void) >> +{ >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 5efd442b24af..a61b4d3c2738 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -308,6 +308,7 @@ real_start_efi: >> bl check_cpu_mode >> bl cpu_init >> bl create_page_tables >> + load_paddr x0, boot_pgtable >> bl enable_mmu >> >> /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ >> @@ -365,29 +366,14 @@ GLOBAL(init_secondary) >> #endif >> bl check_cpu_mode >> bl cpu_init >> - bl create_page_tables >> + load_paddr x0, init_ttbr >> + ldr x0, [x0] >> bl enable_mmu >> >> /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ >> ldr x0, =secondary_switched >> br x0 >> secondary_switched: >> - /* >> - * Non-boot CPUs need to move on to the proper pagetables, which were >> - * setup in init_secondary_pagetables. >> - * >> - * XXX: This is not compliant with the Arm Arm. >> - */ >> - ldr x4, =init_ttbr /* VA of TTBR0_EL2 stashed by CPU 0 */ >> - ldr x4, [x4] /* Actual value */ >> - dsb sy >> - msr TTBR0_EL2, x4 >> - dsb sy >> - isb >> - tlbi alle2 >> - dsb sy /* Ensure completion of TLB flush */ >> - isb >> - >> #ifdef CONFIG_EARLY_PRINTK >> /* Use a virtual address to access the UART. */ >> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >> @@ -672,9 +658,13 @@ ENDPROC(create_page_tables) >> * mapping. In other word, the caller is responsible to switch to the runtime >> * mapping. >> * >> - * Clobbers x0 - x3 >> + * Inputs: >> + * x0 : Physical address of the page tables. >> + * >> + * Clobbers x0 - x4 >> */ >> enable_mmu: >> + mov x4, x0 >> PRINT("- Turning on paging -\r\n") >> >> /* >> @@ -685,8 +675,7 @@ enable_mmu: >> dsb nsh >> >> /* Write Xen's PT's paddr into TTBR0_EL2 */ >> - load_paddr x0, boot_pgtable >> - msr TTBR0_EL2, x0 >> + msr TTBR0_EL2, x4 >> isb >> >> mrs x0, SCTLR_EL2 >> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c >> index 694fbf67e62a..9637f424699e 100644 >> --- a/xen/arch/arm/arm64/smpboot.c >> +++ b/xen/arch/arm/arm64/smpboot.c >> @@ -106,10 +106,23 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn) >> >> int arch_cpu_up(int cpu) >> { >> + int rc; >> + >> if ( !smp_enable_ops[cpu].prepare_cpu ) >> return -ENODEV; >> >> - return smp_enable_ops[cpu].prepare_cpu(cpu); >> + update_identity_mapping(true); >> + >> + rc = smp_enable_ops[cpu].prepare_cpu(cpu); >> + if ( rc ) >> + update_identity_mapping(false); >> + >> + return rc; >> +} >> + >> +void arch_cpu_up_finish(void) >> +{ >> + update_identity_mapping(false); >> } >> >> /* >> diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h >> index 8133d5c29572..a37ca55bff2c 100644 >> --- a/xen/arch/arm/include/asm/smp.h >> +++ b/xen/arch/arm/include/asm/smp.h >> @@ -25,6 +25,7 @@ extern void noreturn stop_cpu(void); >> extern int arch_smp_init(void); >> extern int arch_cpu_init(int cpu, struct dt_device_node *dn); >> extern int arch_cpu_up(int cpu); >> +extern void arch_cpu_up_finish(void); >> >> int cpu_up_send_sgi(int cpu); >> >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c >> index 412ae2286906..4a89b3a8345b 100644 >> --- a/xen/arch/arm/smpboot.c >> +++ b/xen/arch/arm/smpboot.c >> @@ -500,6 +500,7 @@ int __cpu_up(unsigned int cpu) >> init_data.cpuid = ~0; >> smp_up_cpu = MPIDR_INVALID; >> clean_dcache(smp_up_cpu); >> + arch_cpu_up_finish(); >> >> if ( !cpu_online(cpu) ) >> { >> -- >> 2.38.1 >> >> > >
diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c index e7368665d50d..518e9f9c7e70 100644 --- a/xen/arch/arm/arm32/smpboot.c +++ b/xen/arch/arm/arm32/smpboot.c @@ -21,6 +21,10 @@ int arch_cpu_up(int cpu) return platform_cpu_up(cpu); } +void arch_cpu_up_finish(void) +{ +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 5efd442b24af..a61b4d3c2738 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -308,6 +308,7 @@ real_start_efi: bl check_cpu_mode bl cpu_init bl create_page_tables + load_paddr x0, boot_pgtable bl enable_mmu /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ @@ -365,29 +366,14 @@ GLOBAL(init_secondary) #endif bl check_cpu_mode bl cpu_init - bl create_page_tables + load_paddr x0, init_ttbr + ldr x0, [x0] bl enable_mmu /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ ldr x0, =secondary_switched br x0 secondary_switched: - /* - * Non-boot CPUs need to move on to the proper pagetables, which were - * setup in init_secondary_pagetables. - * - * XXX: This is not compliant with the Arm Arm. - */ - ldr x4, =init_ttbr /* VA of TTBR0_EL2 stashed by CPU 0 */ - ldr x4, [x4] /* Actual value */ - dsb sy - msr TTBR0_EL2, x4 - dsb sy - isb - tlbi alle2 - dsb sy /* Ensure completion of TLB flush */ - isb - #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ ldr x23, =EARLY_UART_VIRTUAL_ADDRESS @@ -672,9 +658,13 @@ ENDPROC(create_page_tables) * mapping. In other word, the caller is responsible to switch to the runtime * mapping. * - * Clobbers x0 - x3 + * Inputs: + * x0 : Physical address of the page tables. + * + * Clobbers x0 - x4 */ enable_mmu: + mov x4, x0 PRINT("- Turning on paging -\r\n") /* @@ -685,8 +675,7 @@ enable_mmu: dsb nsh /* Write Xen's PT's paddr into TTBR0_EL2 */ - load_paddr x0, boot_pgtable - msr TTBR0_EL2, x0 + msr TTBR0_EL2, x4 isb mrs x0, SCTLR_EL2 diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c index 694fbf67e62a..9637f424699e 100644 --- a/xen/arch/arm/arm64/smpboot.c +++ b/xen/arch/arm/arm64/smpboot.c @@ -106,10 +106,23 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn) int arch_cpu_up(int cpu) { + int rc; + if ( !smp_enable_ops[cpu].prepare_cpu ) return -ENODEV; - return smp_enable_ops[cpu].prepare_cpu(cpu); + update_identity_mapping(true); + + rc = smp_enable_ops[cpu].prepare_cpu(cpu); + if ( rc ) + update_identity_mapping(false); + + return rc; +} + +void arch_cpu_up_finish(void) +{ + update_identity_mapping(false); } /* diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h index 8133d5c29572..a37ca55bff2c 100644 --- a/xen/arch/arm/include/asm/smp.h +++ b/xen/arch/arm/include/asm/smp.h @@ -25,6 +25,7 @@ extern void noreturn stop_cpu(void); extern int arch_smp_init(void); extern int arch_cpu_init(int cpu, struct dt_device_node *dn); extern int arch_cpu_up(int cpu); +extern void arch_cpu_up_finish(void); int cpu_up_send_sgi(int cpu); diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index 412ae2286906..4a89b3a8345b 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -500,6 +500,7 @@ int __cpu_up(unsigned int cpu) init_data.cpuid = ~0; smp_up_cpu = MPIDR_INVALID; clean_dcache(smp_up_cpu); + arch_cpu_up_finish(); if ( !cpu_online(cpu) ) {