Re: [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xenproject.org,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used
Date: Thu, 22 Aug 2019 11:17:09 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.1908221101110.22783@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <20190812173019.11956-21-julien.grall@arm.com>

On Mon, 12 Aug 2019, Julien Grall wrote:
> The 1:1 mapping may clash with other parts of the Xen virtual memory
> layout. At the moment, Xen is handling the clash by only creating a
> mapping to the runtime virtual address before enabling the MMU.
> 
> The rest of the mappings (such as the fixmap) will be mapped after the
> MMU is enabled. However, the code doing the mapping is not safe as it
> replace mapping without using the Break-Before-Make sequence.
> 
> As the 1:1 mapping can be anywhere in the memory, it is easier to remove
> all the entries added as soon as the 1:1 mapping is not used rather than
> adding the Break-Before-Make sequence everywhere.
> 
> It is difficult to track where exactly the 1:1 mapping was created
> without a full rework of create_page_tables(). Instead, introduce a new
> function remove_identity_mapping() will look where is the top-level entry
> for the 1:1 mapping and remove it.
> 
> The new function is only called for the boot CPU. Secondary CPUs will
> switch directly to the runtime page-tables so there are no need to
> remove the 1:1 mapping. Note that this still doesn't make the Secondary
> CPUs path safe but it is not making it worst.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     It is very likely we will need to re-introduce the 1:1 mapping to cater
>     secondary CPUs boot and suspend/resume. For now, the attempt is to make
>     boot CPU path fully Arm Arm compliant.
> 
>     Changes in v3:
>         - Remove unused label
>         - Avoid harcoding slots
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 86 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 8f945d318a..34fc9ffcee 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -32,6 +32,10 @@
>  #define PT_UPPER(x) (PT_##x & 0xf00)
>  #define PT_LOWER(x) (PT_##x & 0x0ff)
>  
> +/* Convenience defines to get slot used by Xen mapping. */
> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> +
>  #if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
>  #include EARLY_PRINTK_INC
>  #endif
> @@ -158,6 +162,13 @@ past_zImage:
>          ldr   r0, =primary_switched
>          mov   pc, r0
>  primary_switched:
> +        /*
> +         * The 1:1 map may clash with other parts of the Xen virtual memory
> +         * layout. As it is not used anymore, remove it completely to
> +         * avoid having to worry about replacing existing mapping
> +         * afterwards.
> +         */
> +        bl    remove_identity_mapping
>          bl    setup_fixmap
>  #ifdef CONFIG_EARLY_PRINTK
>          /* Use a virtual address to access the UART. */
> @@ -474,12 +485,63 @@ enable_mmu:
>          mov   pc, lr
>  ENDPROC(enable_mmu)
>  
> -setup_fixmap:
> +/*
> + * Remove the 1:1 map for the page-tables. It is not easy to keep track
> + * where the 1:1 map was mapped, so we will look for the top-level entry
> + * exclusive to the 1:1 map and remove it.
> + *
> + * Inputs:
> + *   r9 : paddr(start)
> + *
> + * Clobbers r0 - r3
> + */
> +remove_identity_mapping:
> +        /* r2:r3 := invalid page-table entry */
> +        mov   r2, #0x0
> +        mov   r3, #0x0
>          /*
> -         * Now we can install the fixmap and dtb mappings, since we
> -         * don't need the 1:1 map any more
> +         * Find the first slot used. Remove the entry for the first
> +         * table if the slot is not XEN_FIRST_SLOT. For slot XEN_FIRST_SLOT,
> +         * the 1:1 mapping was done in the second table.

May I suggest something more similar to the arm64 version as a comment?
Something like:

"Find the first slot used. Remove the entry for the first table if the
slot is not XEN_FIRST_SLOT. In case of XEN_FIRST_SLOT, we have to look
at the second table as the slot is shared with the XEN_VIRT_START
mapping."

>           */
> -        dsb
> +        lsr   r1, r9, #FIRST_SHIFT
> +        mov_w r0, LPAE_ENTRY_MASK

ldr?

> +        and  r1, r1, r0              /* r1 := first slot */
> +        cmp  r1, #XEN_FIRST_SLOT

NIT: align

> +        beq   1f
> +        /* It is not in slot 0, remove the entry */
> +        ldr   r0, =boot_pgtable      /* r0 := root table */
> +        lsl   r1, r1, #3             /* r1 := Slot offset */
> +        strd  r2, r3, [r0, r1]
> +        b     identity_mapping_removed
> +
> +1:
> +        /*
> +         * Find the second slot used. Remove the entry for the first
> +         * table if the slot is not XEN_SECOND_SLOT. For slot XEN_SECOND_SLOT,
> +         * it means the 1:1 mapping was not created.
> +         */
> +        lsr   r1, r9, #SECOND_SHIFT
> +        mov_w r0, LPAE_ENTRY_MASK
> +        and   r1, r1, r0             /* r1 := second slot */
> +        cmp   r1, #XEN_SECOND_SLOT
> +        beq   identity_mapping_removed
> +        /* It is not in slot 1, remove the entry */
> +        ldr   r0, =boot_second       /* r0 := second table */
> +        lsl   r1, r1, #3             /* r1 := Slot offset */
> +        strd  r2, r3, [r0, r1]
> +
> +identity_mapping_removed:
> +        /* See asm-arm/arm32/flushtlb.h for the explanation of the sequence. */
> +        dsb   nshst
> +        mcr   CP32(r0, TLBIALLH)
> +        dsb   nsh
> +        isb

As for the arm64 patch, I would like to understand dsb ishst vs. dsb
nshst.

> +        mov   pc, lr
> +ENDPROC(remove_identity_mapping)
> +
> +setup_fixmap:
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>          /* Add UART to the fixmap table */
>          ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
> @@ -489,7 +551,6 @@ setup_fixmap:
>          orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
>          mov   r3, #0x0
>          strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> -1:
>  
>          /* Map fixmap into boot_second */
>          ldr   r1, =boot_second       /* r1 := vaddr (boot_second) */
> @@ -501,19 +562,10 @@ setup_fixmap:
>          mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
>          mov   r3, #0x0
>          strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
> -#endif
> -
> -        /*
> -         * Flush the TLB in case the 1:1 mapping happens to clash with
> -         * the virtual addresses used by the fixmap or DTB.
> -         */
> -        dsb                          /* Ensure any page table updates made above
> -                                      * have occurred. */
>  
> -        isb
> -        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
> -        dsb                          /* Ensure completion of TLB flush */
> -        isb
> +        /* Ensure any page table updates made above have occurred. */
> +        dsb   nshst

same here

> +#endif
>          mov   pc, lr
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel