| Message ID | 20190722213958.5761-30-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Rework head.S to make it more compliant with the Arm Arm | expand |
On Mon, 22 Jul 2019, Julien Grall wrote: > The assembly switch to the runtime PT is only necessary for the > secondary CPUs. So move the code in the secondary CPUs path. > > While this is definitely not compliant with the Arm Arm as we are > switching between two differents set of page-tables without turning off > the MMU. Turning off the MMU is impossible here as the ID map may clash > with other mappings in the runtime page-tables. This will require more > rework to avoid the problem. So for now add a TODO in the code. > > Finally, the code is currently assume that r5 will be properly set to 0 > before hand. This is done by create_page_tables() which is called quite > early in the boot process. There are a risk this may be oversight in the > future and therefore breaking secondary CPUs boot. Instead, set r5 to 0 > just before using it. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v2: > - Patch added > --- > xen/arch/arm/arm32/head.S | 42 ++++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 22 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 4081a52dfa..6dc6032498 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -201,6 +201,26 @@ GLOBAL(init_secondary) > mov pc, r0 > secondary_switched: > bl setup_fixmap > + > + /* > + * 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 r4, =init_ttbr /* VA of HTTBR value stashed by CPU 0 */ > + mov r5, #0 Why do we need to zero r5? Shouldn't ldrd overwrite r5 anyway? > + ldrd r4, r5, [r4] /* Actual value */ > + dsb > + mcrr CP64(r4, r5, HTTBR) > + dsb > + isb > + mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */ > + mcr CP32(r0, ICIALLU) /* Flush I-cache */ > + mcr CP32(r0, BPIALL) /* Flush branch predictor */ > + dsb /* Ensure completion of TLB+BP flush */ > + isb > + > b launch > ENDPROC(init_secondary) > > @@ -504,28 +524,6 @@ ENDPROC(setup_fixmap) > launch: > PRINT("- Ready -\r\n") > > - /* The boot CPU should go straight into C now */ > - teq r12, #0 > - beq 1f > - > - /* > - * Non-boot CPUs need to move on to the proper pagetables, which were > - * setup in init_secondary_pagetables. > - */ > - > - ldr r4, =init_ttbr /* VA of HTTBR value stashed by CPU 0 */ > - ldrd r4, r5, [r4] /* Actual value */ > - dsb > - mcrr CP64(r4, r5, HTTBR) > - dsb > - isb > - mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */ > - mcr CP32(r0, ICIALLU) /* Flush I-cache */ > - mcr CP32(r0, BPIALL) /* Flush branch predictor */ > - dsb /* Ensure completion of TLB+BP flush */ > - isb > - > -1: > ldr r0, =init_data > add r0, #INITINFO_stack /* Find the boot-time stack */ > ldr sp, [r0] > -- > 2.11.0 >
Hi Stefano, On 7/30/19 9:25 PM, Stefano Stabellini wrote: > On Mon, 22 Jul 2019, Julien Grall wrote: >> The assembly switch to the runtime PT is only necessary for the >> secondary CPUs. So move the code in the secondary CPUs path. >> >> While this is definitely not compliant with the Arm Arm as we are >> switching between two differents set of page-tables without turning off >> the MMU. Turning off the MMU is impossible here as the ID map may clash >> with other mappings in the runtime page-tables. This will require more >> rework to avoid the problem. So for now add a TODO in the code. >> >> Finally, the code is currently assume that r5 will be properly set to 0 >> before hand. This is done by create_page_tables() which is called quite >> early in the boot process. There are a risk this may be oversight in the >> future and therefore breaking secondary CPUs boot. Instead, set r5 to 0 >> just before using it. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in v2: >> - Patch added >> --- >> xen/arch/arm/arm32/head.S | 42 ++++++++++++++++++++---------------------- >> 1 file changed, 20 insertions(+), 22 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index 4081a52dfa..6dc6032498 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -201,6 +201,26 @@ GLOBAL(init_secondary) >> mov pc, r0 >> secondary_switched: >> bl setup_fixmap >> + >> + /* >> + * 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 r4, =init_ttbr /* VA of HTTBR value stashed by CPU 0 */ >> + mov r5, #0 > > Why do we need to zero r5? Shouldn't ldrd overwrite r5 anyway? I was on auto-pilot mode and saw the trailing "d" and thought it was a "strd". I will remove it in the next version. Cheers,
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 4081a52dfa..6dc6032498 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -201,6 +201,26 @@ GLOBAL(init_secondary) mov pc, r0 secondary_switched: bl setup_fixmap + + /* + * 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 r4, =init_ttbr /* VA of HTTBR value stashed by CPU 0 */ + mov r5, #0 + ldrd r4, r5, [r4] /* Actual value */ + dsb + mcrr CP64(r4, r5, HTTBR) + dsb + isb + mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */ + mcr CP32(r0, ICIALLU) /* Flush I-cache */ + mcr CP32(r0, BPIALL) /* Flush branch predictor */ + dsb /* Ensure completion of TLB+BP flush */ + isb + b launch ENDPROC(init_secondary) @@ -504,28 +524,6 @@ ENDPROC(setup_fixmap) launch: PRINT("- Ready -\r\n") - /* The boot CPU should go straight into C now */ - teq r12, #0 - beq 1f - - /* - * Non-boot CPUs need to move on to the proper pagetables, which were - * setup in init_secondary_pagetables. - */ - - ldr r4, =init_ttbr /* VA of HTTBR value stashed by CPU 0 */ - ldrd r4, r5, [r4] /* Actual value */ - dsb - mcrr CP64(r4, r5, HTTBR) - dsb - isb - mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLB */ - mcr CP32(r0, ICIALLU) /* Flush I-cache */ - mcr CP32(r0, BPIALL) /* Flush branch predictor */ - dsb /* Ensure completion of TLB+BP flush */ - isb - -1: ldr r0, =init_data add r0, #INITINFO_stack /* Find the boot-time stack */ ldr sp, [r0]
The assembly switch to the runtime PT is only necessary for the secondary CPUs. So move the code in the secondary CPUs path. While this is definitely not compliant with the Arm Arm as we are switching between two differents set of page-tables without turning off the MMU. Turning off the MMU is impossible here as the ID map may clash with other mappings in the runtime page-tables. This will require more rework to avoid the problem. So for now add a TODO in the code. Finally, the code is currently assume that r5 will be properly set to 0 before hand. This is done by create_page_tables() which is called quite early in the boot process. There are a risk this may be oversight in the future and therefore breaking secondary CPUs boot. Instead, set r5 to 0 just before using it. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Patch added --- xen/arch/arm/arm32/head.S | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-)