Hi Julien,
On 13/01/2023 11:11, Julien Grall wrote:
>
>
> From: Julien Grall <jgr...@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 <jgr...@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
