| Message ID | 20221022150422.17707-4-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> > > The sequence for flushing the TLBs is 4 instruction long and often > require an explanation how it works. s/require/requires/ > > So create an helper and use it in the boot code (switch_ttbr() is left > alone for now). > > Note that in secondary_switched, we were also flushing the instruction > cache and branch predictor. Neither of them was necessary because: > * We are only supporting IVIPT cache on arm32, so the instruction > cache flush is only necessary when executable code is modified. > None of the boot code is doing that. > * The instruction cache is not invalidated and misprediction is not > a problem at boot. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm32/head.S | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 163bd6596dec..aeaa8d105aeb 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -66,6 +66,21 @@ > add \rb, \rb, r10 > .endm > > +/* > + * Flush local TLBs > + * > + * tmp1: Scratch register I would love to adhere to the way of describing macro params like you did in mov_w. This would mean: @tmp: scratch register Apart from that, the change looks ok. Question on the side: Why do we use nshst in assembly and ishst in TLB helper macro? Is it because the latter is also used to flush the inner TLBs whereas the former only local ones? ~Michal
On 25/10/2022 10:53, Michal Orzel wrote: > Hi Julien, Hi Michal, > On 22/10/2022 17:04, Julien Grall wrote: >> >> >> From: Julien Grall <jgrall@amazon.com> >> >> The sequence for flushing the TLBs is 4 instruction long and often >> require an explanation how it works. > s/require/requires/ Will fix it. > >> >> So create an helper and use it in the boot code (switch_ttbr() is left >> alone for now). >> >> Note that in secondary_switched, we were also flushing the instruction >> cache and branch predictor. Neither of them was necessary because: >> * We are only supporting IVIPT cache on arm32, so the instruction >> cache flush is only necessary when executable code is modified. >> None of the boot code is doing that. >> * The instruction cache is not invalidated and misprediction is not >> a problem at boot. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> xen/arch/arm/arm32/head.S | 31 ++++++++++++++++++------------- >> 1 file changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index 163bd6596dec..aeaa8d105aeb 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -66,6 +66,21 @@ >> add \rb, \rb, r10 >> .endm >> >> +/* >> + * Flush local TLBs >> + * >> + * tmp1: Scratch register > I would love to adhere to the way of describing macro params like you did in mov_w. This would mean: > @tmp: scratch register ok. > > Apart from that, the change looks ok. > > Question on the side: > Why do we use nshst in assembly and ishst in TLB helper macro? > Is it because the latter is also used to flush the inner TLBs whereas the former only local ones? nshst is sufficient for local TLBs flush. The *_local helpers could also use 'nshst' but when they were introduced it wasn't clear whether 'nshst' was enough. I only got the confirmation after and never took the opportunity to update the code. Cheers,
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 163bd6596dec..aeaa8d105aeb 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -66,6 +66,21 @@ add \rb, \rb, r10 .endm +/* + * Flush local TLBs + * + * tmp1: Scratch register + * + * See asm/arm32/flushtlb.h for the explanation of the sequence. + */ +.macro flush_xen_tlb_local tmp1 + /* See asm/arm32/flushtlb.h for the explanation of the sequence. */ + dsb nshst + mcr CP32(\tmp1, TLBIALLH) + dsb nsh + isb +.endm + /* * Common register usage in this file: * r0 - @@ -233,11 +248,7 @@ secondary_switched: 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 + flush_xen_tlb_local r0 #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ @@ -530,8 +541,7 @@ enable_mmu: * The state of the TLBs is unknown before turning on the MMU. * Flush them to avoid stale one. */ - mcr CP32(r0, TLBIALLH) /* Flush hypervisor TLBs */ - dsb nsh + flush_xen_tlb_local r0 /* Write Xen's PT's paddr into the HTTBR */ load_paddr r0, boot_pgtable @@ -606,12 +616,7 @@ remove_identity_mapping: strd r2, r3, [r0, r1] identity_mapping_removed: - /* See asm/arm32/flushtlb.h for the explanation of the sequence. */ - dsb nshst - mcr CP32(r0, TLBIALLH) - dsb nsh - isb - + flush_xen_tlb_local r0 mov pc, lr ENDPROC(remove_identity_mapping)