xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.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 19:31:08 +0100	[thread overview]
Message-ID: <937b6185-9a3e-f8b5-8335-2d948b3bb11a@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1908221101110.22783@sstabellini-ThinkPad-T480s>

Hi Stefano,

On 8/22/19 7:17 PM, Stefano Stabellini wrote:
> 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."

I have answered that on the arm64 version. Let's continue the 
conversation there.

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

What's wrong with the mov_w? Ok it is two instructions but... the 
constant will be stored in a literal and therefore induce a memory load 
(see patch #8).


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

align of?

> 
> 
>> +        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.


I have answered that on the arm64 version. Let's continue the 
conversation there.

> 
> 
>> +        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


I have answered that on the arm64 version. Let's continue the 
conversation there.

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

Cheers,

-- 
Julien Grall

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

  reply	other threads:[~2019-08-22 18:31 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-12 17:29 [Xen-devel] [PATCH v3 00/28] xen/arm: Rework head.S to make it more compliant with the Arm Arm Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 01/28] xen/arm: lpae: Allow more LPAE helpers to be used in assembly Julien Grall
2019-08-22 17:07   ` Stefano Stabellini
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used Julien Grall
2019-08-22 17:58   ` Stefano Stabellini
2019-08-22 18:25     ` Julien Grall
2019-08-22 18:29       ` Stefano Stabellini
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 03/28] xen/arm64: head: Rework and document setup_fixmap() Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 04/28] xen/arm64: head: Rework and document launch() Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 05/28] xen/arm64: head: Setup TTBR_EL2 in enable_mmu() and add missing isb Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 06/28] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol Julien Grall
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 07/28] xen/arm64: head: Fix typo in the documentation on top of init_uart() Julien Grall
2019-08-22 17:08   ` Stefano Stabellini
2019-08-12 17:29 ` [Xen-devel] [PATCH v3 08/28] xen/arm32: head: Add a macro to move an immediate constant into a 32-bit register Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 09/28] xen/arm32: head: Mark the end of subroutines with ENDPROC Julien Grall
2019-08-12 17:33   ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 10/28] xen/arm32: head: Don't clobber r14/lr in the macro PRINT Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 11/28] xen/arm32: head: Rework UART initialization on boot CPU Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 12/28] xen/arm32: head: Introduce print_reg Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 13/28] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs Julien Grall
2019-08-22 17:11   ` Stefano Stabellini
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 14/28] xen/arm32: head: Rework and document check_cpu_mode() Julien Grall
2019-08-22 17:14   ` Stefano Stabellini
2019-09-07 10:35     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 15/28] xen/arm32: head: Rework and document zero_bss() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 16/28] xen/arm32: head: Document create_pages_tables() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 17/28] xen/arm32: head: Document enable_mmu() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 18/28] xen/arm32: head: Move assembly switch to the runtime PT in secondary CPUs path Julien Grall
2019-08-22 17:17   ` Stefano Stabellini
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 19/28] xen/arm32: head: Don't setup the fixmap on secondary CPUs Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 20/28] xen/arm32: head: Remove 1:1 mapping as soon as it is not used Julien Grall
2019-08-22 18:17   ` Stefano Stabellini
2019-08-22 18:31     ` Julien Grall [this message]
2019-08-22 22:53       ` Stefano Stabellini
2019-08-22 23:39         ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 21/28] xen/arm32: head: Rework and document setup_fixmap() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 22/28] xen/arm32: head: Rework and document launch() Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 23/28] xen/arm32: head: Setup HTTBR in enable_mmu() and add missing isb Julien Grall
2019-08-22 17:15   ` Stefano Stabellini
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 24/28] xen/arm: Zero BSS after the MMU and D-cache is turned on Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 25/28] xen/arm64: head: Introduce macros to create table and mapping entry Julien Grall
2019-08-22 23:31   ` Stefano Stabellini
2019-08-22 23:44     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 26/28] xen/arm64: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall
2019-08-24  1:16   ` Stefano Stabellini
2019-09-17 17:53     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 27/28] xen/arm32: head: Introduce macros to create table and mapping entry Julien Grall
2019-08-23  1:10   ` Stefano Stabellini
2019-08-23  9:40     ` Julien Grall
2019-08-12 17:30 ` [Xen-devel] [PATCH v3 28/28] xen/arm32: head: Use a page mapping for the 1:1 mapping in create_page_tables() Julien Grall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=937b6185-9a3e-f8b5-8335-2d948b3bb11a@arm.com \
    --to=julien.grall@arm.com \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).