Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 9 Mar 2022, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> 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.
> 
> Take the opportunity to instruction cache flush as the operation is
> only necessary when the memory is updated.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ---
> 
>     TODO:
>         * Rename _end_boot to _end_id_mapping or similar
>         * Check the memory barriers
>         * I suspect the instruction cache flush will be necessary
>           for cache coloring.
> ---
>  xen/arch/arm/arm64/head.S | 31 ++++++++++++++++++++-----------
>  xen/arch/arm/mm.c         | 14 +++++++++++++-
>  2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 878649280d73..c5cc72b8fe6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -803,36 +803,45 @@ fail:   PRINT("- Boot failed -\r\n")
>          b     1b
>  ENDPROC(fail)
>  
> -GLOBAL(_end_boot)
> -
>  /*
>   * Switch TTBR
>   *
>   * x0    ttbr
>   *
> - * TODO: This code does not comply with break-before-make.
> + * XXX: Check the barriers
>   */
> -ENTRY(switch_ttbr)
> +ENTRY(switch_ttbr_id)
>          dsb   sy                     /* Ensure the flushes happen before
>                                        * continuing */
>          isb                          /* Ensure synchronization with previous
>                                        * changes to text */
> +
> +        /* Turn off MMU */
> +        mrs    x1, SCTLR_EL2
> +        bic    x1, x1, #SCTLR_Axx_ELx_M
> +        msr    SCTLR_EL2, x1
> +        dsb    sy
> +        isb
> +
>          tlbi   alle2                 /* Flush hypervisor TLB */
> -        ic     iallu                 /* Flush I-cache */
>          dsb    sy                    /* Ensure completion of TLB flush */
>          isb
>  
> -        msr    TTBR0_EL2, x0
> +        msr   TTBR0_EL2, x0
> +
> +        mrs   x1, SCTLR_EL2
> +        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
> +        msr   SCTLR_EL2, x1
>  
>          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 */
> -        isb
> +        /* Turn on the MMU */
> +
>  
>          ret
> -ENDPROC(switch_ttbr)
> +ENDPROC(switch_ttbr_id)
> +
> +GLOBAL(_end_boot)
>  
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5c4dece16f7f..a53760af7af0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
>      flush_xen_tlb_local();
>  }
>  
> -extern void switch_ttbr(uint64_t ttbr);
> +extern void switch_ttbr_id(uint64_t ttbr);
> +
> +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> +
> +static void switch_ttbr(uint64_t ttbr)
> +{
> +    vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> +    switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> +
> +    update_identity_mapping(true);
> +    fn(ttbr);
> +    update_identity_mapping(false);
> +}

As far as I can tell this should work for coloring too. In the case of
coloring:

    /* running on the old mapping, same as non-coloring */
    update_identity_mapping(true);

    /* jumping to the 1:1 mapping of the old Xen and switching to the
     * new pagetable */
    fn(ttbr);

    /* new pagetable is enabled, now we are back to addresses greater
     * than XEN_VIRT_START, which correspond to new cache-colored Xen */
    update_identity_mapping(false);


The only doubt that I have is: are we sure than a single page of 1:1
mapping is enough? What if:

virt_to_maddr(switch_ttbr_id) - virt_to_maddr(_start) > PAGE_SIZE


We might have to do a 1:1 mapping of size = _end-_start. It would work
with coloring too because we are doing a 1:1 mapping of the old copy of
Xen which is non-colored and contiguous (not the new copy which is
colored and fragmented).


Thanks Julien very much for your help!