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

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 02/28] xen/arm64: head: Remove 1:1 mapping as soon as it is not used
Date: Thu, 22 Aug 2019 19:25:51 +0100	[thread overview]
Message-ID: <4484acd2-2f9c-1650-270a-b6ada0a7f531@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1908221034570.22783@sstabellini-ThinkPad-T480s>

Hi Stefano,

On 8/22/19 6:58 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:
>>          - Avoid hardcoding slots
>>
>>      Changes in v2:
>>          - s/ID map/1:1 mapping/
>>          - Rename remove_id_map() to remove_identity_mapping()
>>          - Add missing signed-off-by
>> ---
>>   xen/arch/arm/arm64/head.S | 94 +++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 79 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 50cff08756..ec138aae3e 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -33,6 +33,11 @@
>>   #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
>>   #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
>>   
>> +/* Convenience defines to get slot used by Xen mapping. */
>> +#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
>> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
>> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
>> +
>>   #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
>>   
>>   #define __HEAD_FLAG_PHYS_BASE   1
>> @@ -301,6 +306,13 @@ real_start_efi:
>>           ldr   x0, =primary_switched
>>           br    x0
>>   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. */
>> @@ -626,10 +638,71 @@ enable_mmu:
>>           ret
>>   ENDPROC(enable_mmu)
>>   
>> +/*
>> + * Remove the 1:1 map for the page-tables. It is not easy to keep track
>                           ^ from

I will fix it in the next version.

> 
>> + * 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:
>> + *   x19: paddr(start)
>> + *
>> + * Clobbers x0 - x1
>> + */
>> +remove_identity_mapping:
>> +        /*
>> +         * Find the zeroeth slot used. Remove the entry from zeroeth
>> +         * table if the slot is not XEN_ZEROETH_SLOT.
> 
> This part of the comment is good
> 
> 
>> +         * For slot XEN_ZEROETH_SLOT, the 1:1 mapping was either done in first
>> +         * or second table.
> 
> I don't think this sentence is very useful now that the slot is not
> hard-coded anymore. I would remove it. Instead, I would write something
> like "The slot XEN_ZEROETH_SLOT is used for the XEN_VIRT_START mapping."
> The same goes for all the other levels.

I think this is a bit confusing because one may think the XEN_VIRT_START 
mapping is using the full slot. Whereas it is only partially using it.

So I would prefer to drop the sentence completely.

> 
> 
>> +         */
>> +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
>> +        cmp   x1, #XEN_ZEROETH_SLOT
>> +        beq   1f
>> +        /* It is not in slot XEN_ZEROETH_SLOT, remove the entry. */
>> +        ldr   x0, =boot_pgtable         /* x0 := root table */
>> +        str   xzr, [x0, x1, lsl #3]
>> +        b     identity_mapping_removed
>> +
>> +1:
>> +        /*
>> +         * 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.
>> +         */
>> +        lsr   x1, x19, #FIRST_SHIFT
>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>> +        cmp   x1, #XEN_FIRST_SLOT
>> +        beq   1f
>> +        /* It is not in slot XEN_FIRST_SLOT, remove the entry. */
>> +        ldr   x0, =boot_first           /* x0 := first table */
>> +        str   xzr, [x0, x1, lsl #3]
>> +        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   x1, x19, #SECOND_SHIFT
>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>> +        cmp   x1, #XEN_SECOND_SLOT
>> +        beq   identity_mapping_removed
>> +        /* It is not in slot 1, remove the entry */
>> +        ldr   x0, =boot_second          /* x0 := second table */
>> +        str   xzr, [x0, x1, lsl #3]
>> +
>> +identity_mapping_removed:
>> +        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */
>> +        dsb   nshst
>> +        tlbi  alle2
>> +        dsb   nsh
>> +        isb
> 
> I just want to point out that asm-arm/arm64/flushtlb.h says to use:
> 
>   * DSB ISHST        // Ensure prior page-tables updates have completed
>   * TLBI...          // Invalidate the TLB
>   * DSB ISH          // Ensure the TLB invalidation has completed
>   * ISB              // See explanation below
> 
> Also implemented as:
> 
>      asm volatile(               \
>          "dsb  ishst;"           \
>          "tlbi "  # tlbop  ";"   \
>          "dsb  ish;"             \
>          "isb;"                  \
>          : : : "memory");        \
> 
> Why is non-shareable enough? Shouldn't it be inner shareable?

I thought I answered this before. I should have probably clarified in 
the commit message.

nsh is used (rather than ish) because the TLB flush is local (see page 
D5-230 ARM DDI 0487D.a). For convenience here is the text:

"In all cases in this section where a DMB or DSB is referred to, it
refers to a DMB or DSB whose required access type is
both loads and stores. A DSB NSH is sufficient to ensure completion of
TLB maintenance instructions that apply to a
single PE. A DSB ISH is sufficient to ensure completion of TLB
maintenance instructions that apply to PEs in the
same Inner Shareable domain."

This is something Linux already does but I wasn't able to find the 
proper justification in the Arm Arm. So I chose a more conservative 
approach that is explained in section K11.5.3 (ARM DDI 0487D.a).

I have an action to update tlbflush.h before this is in a huge pile of 
cleanup/optimization.

Cheers,

-- 
Julien Grall

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