Re: [PATCH v3 08/18] xen/arm32: head: Introduce an helper to flush the TLBs
On 14/12/2022 14:24, Michal Orzel wrote: Hi Julien, Hi Michal, On 12/12/2022 10:55, Julien Grall wrote: From: Julien Grall The sequence for flushing the TLBs is 4 instruction long and often requires an explanation how it works. So create an helper and use it in the boot code (switch_ttbr() is left Here and in title: s/an helper/a helper/ Done. alone for now). Could you explain why? So we need to decide how we expect switch_ttbr(). E.g. if Xen is relocated at a different, should the caller take care of the instruction/branch predictor flush? I have expanded to "switch_ttbr() is left alone until we decided the semantic of the call". 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 Apart from that, the patch is good, so: Reviewed-by: Michal Orzel Thanks! --- Changes in v3: * Fix typo * Update the documentation * Rename the argument from tmp1 to tmp --- xen/arch/arm/arm32/head.S | 30 +- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 40c1d7502007..315abbbaebec 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -66,6 +66,20 @@ add \rb, \rb, r10 .endm +/* + * Flush local TLBs + * + * @tmp:Scratch register As you are respinning a series anyway, could you add just one space after @tmp:? Ok. Cheers, -- Julien Grall
Re: [PATCH v3 08/18] xen/arm32: head: Introduce an helper to flush the TLBs
Hi Julien, On 12/12/2022 10:55, Julien Grall wrote: > > > From: Julien Grall > > The sequence for flushing the TLBs is 4 instruction long and often > requires an explanation how it works. > > So create an helper and use it in the boot code (switch_ttbr() is left Here and in title: s/an helper/a helper/ > alone for now). Could you explain why? > > 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 Apart from that, the patch is good, so: Reviewed-by: Michal Orzel > > --- > Changes in v3: > * Fix typo > * Update the documentation > * Rename the argument from tmp1 to tmp > --- > xen/arch/arm/arm32/head.S | 30 +- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 40c1d7502007..315abbbaebec 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -66,6 +66,20 @@ > add \rb, \rb, r10 > .endm > > +/* > + * Flush local TLBs > + * > + * @tmp:Scratch register As you are respinning a series anyway, could you add just one space after @tmp:? ~Michal
[PATCH v3 08/18] xen/arm32: head: Introduce an helper to flush the TLBs
From: Julien Grall The sequence for flushing the TLBs is 4 instruction long and often requires an explanation how it works. 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 --- Changes in v3: * Fix typo * Update the documentation * Rename the argument from tmp1 to tmp --- xen/arch/arm/arm32/head.S | 30 +- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 40c1d7502007..315abbbaebec 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -66,6 +66,20 @@ add \rb, \rb, r10 .endm +/* + * Flush local TLBs + * + * @tmp:Scratch register + * + * See asm/arm32/flushtlb.h for the explanation of the sequence. + */ +.macro flush_xen_tlb_local tmp +dsb nshst +mcr CP32(\tmp, TLBIALLH) +dsb nsh +isb +.endm + /* * Common register usage in this file: * r0 - @@ -232,11 +246,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. */ @@ -529,8 +539,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 @@ -605,12 +614,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) -- 2.38.1
