| Message ID | 20230113101136.479-14-julien@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Don't switch TTBR while the MMU is on | expand |
> On 13 Jan 2023, at 10:11, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > At the moment, switch_ttbr() is switching the TTBR whilst the MMU is > still on. > > Switching TTBR is like replacing existing mappings with new ones. So > we need to follow the break-before-make sequence. > > In this case, it means the MMU needs to be switched off while the > TTBR is updated. In order to disable the MMU, we need to first > jump to an identity mapping. > > Rename switch_ttbr() to switch_ttbr_id() and create an helper on > top to temporary map the identity mapping and call switch_ttbr() > via the identity address. > > switch_ttbr_id() is now reworked to temporarily turn off the MMU > before updating the TTBR. > > We also need to make sure the helper switch_ttbr() is part of the > identity mapping. So move _end_boot past it. > > 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> The sequence looks ok to me, also the reasoning about barriers and register dependencies discussed in the previous version. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> I’ve also built for arm32/64 and test this patch on fvp, booting Dom0 and creating/running/destroying some guests Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Hi Julien, On 13/01/2023 11:11, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > At the moment, switch_ttbr() is switching the TTBR whilst the MMU is > still on. > > Switching TTBR is like replacing existing mappings with new ones. So > we need to follow the break-before-make sequence. > > In this case, it means the MMU needs to be switched off while the > TTBR is updated. In order to disable the MMU, we need to first > jump to an identity mapping. > > Rename switch_ttbr() to switch_ttbr_id() and create an helper on > top to temporary map the identity mapping and call switch_ttbr() > via the identity address. > > switch_ttbr_id() is now reworked to temporarily turn off the MMU > before updating the TTBR. > > We also need to make sure the helper switch_ttbr() is part of the > identity mapping. So move _end_boot past it. > > 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> > > ---- > Changes in v4: > - Don't modify setup_pagetables() as we don't handle arm32. > - Move the clearing of the boot page tables in an earlier patch > - Fix the numbering > > Changes in v2: > - Remove the arm32 changes. This will be addressed differently > - Re-instate the instruct cache flush. This is not strictly > necessary but kept it for safety. > - Use "dsb ish" rather than "dsb sy". > > > TODO: > * Handle the case where the runtime Xen is loaded at a different > position for cache coloring. This will be dealt separately. > --- > xen/arch/arm/arm64/head.S | 50 +++++++++++++++++++++++------------ > xen/arch/arm/arm64/mm.c | 30 +++++++++++++++++++++ > xen/arch/arm/include/asm/mm.h | 2 ++ > xen/arch/arm/mm.c | 2 -- > 4 files changed, 65 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 663f5813b12e..5efd442b24af 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -816,30 +816,46 @@ ENDPROC(fail) > * Switch TTBR > * > * x0 ttbr > - * > - * TODO: This code does not comply with break-before-make. > */ > -ENTRY(switch_ttbr) > - dsb sy /* Ensure the flushes happen before > - * continuing */ > - isb /* Ensure synchronization with previous > - * changes to text */ > - tlbi alle2 /* Flush hypervisor TLB */ > - ic iallu /* Flush I-cache */ > - dsb sy /* Ensure completion of TLB flush */ > +ENTRY(switch_ttbr_id) > + /* 1) Ensure any previous read/write have completed */ > + dsb ish > + isb > + > + /* 2) Turn off MMU */ > + mrs x1, SCTLR_EL2 > + bic x1, x1, #SCTLR_Axx_ELx_M > + msr SCTLR_EL2, x1 > + isb > + > + /* > + * 3) Flush the TLBs. > + * See asm/arm64/flushtlb.h for the explanation of the sequence. > + */ > + dsb nshst > + tlbi alle2 > + dsb nsh > + isb > + > + /* 4) Update the TTBR */ > + msr TTBR0_EL2, x0 > isb > > - msr TTBR0_EL2, x0 > + /* > + * 5) Flush I-cache > + * This should not be necessary but it is kept for safety. > + */ > + ic iallu > + isb > > - isb /* Ensure synchronization with previous > - * changes to text */ > - tlbi alle2 /* Flush hypervisor TLB */ > - ic iallu /* Flush I-cache */ > - dsb sy /* Ensure completion of TLB flush */ > + /* 6) Turn on the MMU */ > + mrs x1, SCTLR_EL2 > + orr x1, x1, #SCTLR_Axx_ELx_M /* Enable MMU */ > + msr SCTLR_EL2, x1 > isb > > ret > -ENDPROC(switch_ttbr) > +ENDPROC(switch_ttbr_id) > > #ifdef CONFIG_EARLY_PRINTK > /* > diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c > index 798ae93ad73c..2ede4e75ae33 100644 > --- a/xen/arch/arm/arm64/mm.c > +++ b/xen/arch/arm/arm64/mm.c > @@ -120,6 +120,36 @@ void update_identity_mapping(bool enable) > BUG_ON(rc); > } > > +extern void switch_ttbr_id(uint64_t ttbr); > + > +typedef void (switch_ttbr_fn)(uint64_t ttbr); > + > +void __init switch_ttbr(uint64_t ttbr) > +{ > + vaddr_t id_addr = virt_to_maddr(switch_ttbr_id); Shouldn't id_addr be of type paddr_t? > + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; > + lpae_t pte; > + > + /* Enable the identity mapping in the boot page tables */ > + update_identity_mapping(true); Could you please add an empty line here? ~Michal
On 16/01/2023 09:23, Michal Orzel wrote: > Hi Julien, Hi Michal, > On 13/01/2023 11:11, Julien Grall wrote: >> diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c >> index 798ae93ad73c..2ede4e75ae33 100644 >> --- a/xen/arch/arm/arm64/mm.c >> +++ b/xen/arch/arm/arm64/mm.c >> @@ -120,6 +120,36 @@ void update_identity_mapping(bool enable) >> BUG_ON(rc); >> } >> >> +extern void switch_ttbr_id(uint64_t ttbr); >> + >> +typedef void (switch_ttbr_fn)(uint64_t ttbr); >> + >> +void __init switch_ttbr(uint64_t ttbr) >> +{ >> + vaddr_t id_addr = virt_to_maddr(switch_ttbr_id); > Shouldn't id_addr be of type paddr_t? No because... > >> + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; ... here it will be used as a virtual address. >> + lpae_t pte; >> + >> + /* Enable the identity mapping in the boot page tables */ >> + update_identity_mapping(true); > Could you please add an empty line here? Sure. Cheers,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 663f5813b12e..5efd442b24af 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -816,30 +816,46 @@ ENDPROC(fail) * Switch TTBR * * x0 ttbr - * - * TODO: This code does not comply with break-before-make. */ -ENTRY(switch_ttbr) - dsb sy /* Ensure the flushes happen before - * continuing */ - isb /* Ensure synchronization with previous - * changes to text */ - tlbi alle2 /* Flush hypervisor TLB */ - ic iallu /* Flush I-cache */ - dsb sy /* Ensure completion of TLB flush */ +ENTRY(switch_ttbr_id) + /* 1) Ensure any previous read/write have completed */ + dsb ish + isb + + /* 2) Turn off MMU */ + mrs x1, SCTLR_EL2 + bic x1, x1, #SCTLR_Axx_ELx_M + msr SCTLR_EL2, x1 + isb + + /* + * 3) Flush the TLBs. + * See asm/arm64/flushtlb.h for the explanation of the sequence. + */ + dsb nshst + tlbi alle2 + dsb nsh + isb + + /* 4) Update the TTBR */ + msr TTBR0_EL2, x0 isb - msr TTBR0_EL2, x0 + /* + * 5) Flush I-cache + * This should not be necessary but it is kept for safety. + */ + ic iallu + isb - isb /* Ensure synchronization with previous - * changes to text */ - tlbi alle2 /* Flush hypervisor TLB */ - ic iallu /* Flush I-cache */ - dsb sy /* Ensure completion of TLB flush */ + /* 6) Turn on the MMU */ + mrs x1, SCTLR_EL2 + orr x1, x1, #SCTLR_Axx_ELx_M /* Enable MMU */ + msr SCTLR_EL2, x1 isb ret -ENDPROC(switch_ttbr) +ENDPROC(switch_ttbr_id) #ifdef CONFIG_EARLY_PRINTK /* diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c index 798ae93ad73c..2ede4e75ae33 100644 --- a/xen/arch/arm/arm64/mm.c +++ b/xen/arch/arm/arm64/mm.c @@ -120,6 +120,36 @@ void update_identity_mapping(bool enable) BUG_ON(rc); } +extern void switch_ttbr_id(uint64_t ttbr); + +typedef void (switch_ttbr_fn)(uint64_t ttbr); + +void __init switch_ttbr(uint64_t ttbr) +{ + vaddr_t id_addr = virt_to_maddr(switch_ttbr_id); + switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr; + lpae_t pte; + + /* Enable the identity mapping in the boot page tables */ + update_identity_mapping(true); + /* Enable the identity mapping in the runtime page tables */ + pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id); + pte.pt.table = 1; + pte.pt.xn = 0; + pte.pt.ro = 1; + write_pte(&xen_third_id[third_table_offset(id_addr)], pte); + + /* Switch TTBR */ + fn(ttbr); + + /* + * Disable the identity mapping in the runtime page tables. + * Note it is not necessary to disable it in the boot page tables + * because they are not going to be used by this CPU anymore. + */ + update_identity_mapping(false); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 68adcac9fa8d..bff6923f3ea9 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -196,6 +196,8 @@ extern unsigned long total_pages; extern void setup_pagetables(unsigned long boot_phys_offset); /* Map FDT in boot pagetable */ extern void *early_fdt_map(paddr_t fdt_paddr); +/* Switch to a new root page-tables */ +extern void switch_ttbr(uint64_t ttbr); /* Remove early mappings */ extern void remove_early_mappings(void); /* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 39e0d9e03c9c..b9c698088b25 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -476,8 +476,6 @@ static void xen_pt_enforce_wnx(void) flush_xen_tlb_local(); } -extern void switch_ttbr(uint64_t ttbr); - /* Clear a translation table and clean & invalidate the cache */ static void clear_table(void *table) {