Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 6/27/19 7:55 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> +1:
>> +        /*
>> +         * Find the second slot used. Remove the entry for the first
>> +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
>> +         * For slot 1, it means the ID map was not created.
>> +         */
>> +        lsr   x1, x19, #SECOND_SHIFT
>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>> +        cmp   x1, #1
>> +        beq   id_map_removed
>> +        /* It is not in slot 1, remove the entry */
>> +        ldr   x0, =boot_second          /* x0 := second table */
>> +        str   xzr, [x0, x1, lsl #3]
> 
> Wouldn't it be a bit more reliable if we checked whether the slot in
> question for x19 (whether zero, first, second) is a pagetable pointer or
> section map, then zero it if it is a section map, otherwise go down one
> level? If we did it this way it would be independent from the way
> create_page_tables is written.

Your suggestion will not comply with the architecture compliance and how 
Xen is/will be working after the full rework. We want to remove 
everything (mapping + table) added specifically for the 1:1 mapping.

Otherwise, you may end up in a position where boot_first_id is still in 
place. We would need to use the break-before-make sequence in subsequent 
code if we were about to insert 1GB mapping at the same place.

After my rework, we would have virtually no place where 
break-before-make will be necessary as it will enforce all the mappings 
to be destroyed before hand. So I would rather avoid to make a specific 
case for the 1:1 mapping.

As a side note, the current code for the 1:1 mapping is completely wrong 
as using 1GB (or even 2MB) mapping may result to map MMIO region (or 
reserved-region). This may result to cache problem. I have this 
partially fixed on for the next version of series (see [1]).

> 
> With the current code, we are somewhat reliant on the behavior of
> create_page_tables, because we rely on the position of the slot for
> the ID map? Where the assumption for instance is that at level one, if
> the slot is zero, then we need to go down a level, etc. Instead, if we
> checked if the slot is a section map, we could remove it immediately, if
> it is a pagetable pointer, we proceed. The code should be similar in
> complexity and LOC, but it would be more robust.

See above :).

> 
> Something like the following, in pseudo-uncompiled assembly:
> 
>       lsr   x1, x19, #FIRST_SHIFT
>       ldr   x0, =boot_first           /* x0 := first table */
>       ldr   x2, [x0, x1, lsl #3]
>       # check x2 against #PT_MEM
>       cbz   x2, 1f
>       str   xzr, [x0, x1, lsl #3]
>       b     id_map_removed
> 
> 
>> +id_map_removed:
>> +        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */
> 
> Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the
> explanation you are referring to.

The big comment at the top of the header:

/*
  * Every invalidation operation use the following patterns:
  *
  * 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
  *
  * For Xen page-tables the ISB will discard any instructions fetched
  * from the old mappings.
  *
  * For the Stage-2 page-tables the ISB ensures the completion of the DSB
  * (and therefore the TLB invalidation) before continuing. So we know
  * the TLBs cannot contain an entry for a mapping we may have removed.
  */

Note that we are using nsh (and not ish) because we are using local TLB 
flush (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."

I discovered this section after the changes in flushtlb.h has been 
merged. But I am thinking to do a follow-up the local TLB flush code.

> 
> 
>> +        dsb   nshst
>> +        tlbi  alle2
>> +        dsb   nsh
>> +        isb
>> +
>> +        ret
>> +ENDPROC(remove_id_map)

[...]

[1] Rework for create_page_tables

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index a79ae54822..c019dd3e04 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -483,6 +483,60 @@ cpu_init:
  ENDPROC(cpu_init)

  /*
+ * Macro to create a page table entry in \ptbl to \tbl
+ *
+ * ptbl:    table symbol where the entry will be created
+ * tbl:     table symbol to point to
+ * virt:    virtual address
+ * shift:   #imm page table shift
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ * tmp3:    scratch register
+ *
+ * Preserves \virt
+ * Clobbers \tmp1, \tmp2, \tmp3
+ *
+ * Also use x20 for the phys offset.
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3
+        lsr   \tmp1, \virt, #\shift
+        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+        load_paddr \tmp2, \tbl
+        mov   \tmp3, #PT_PT                 /* \tmp3 := right for 
linear PT */
+        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
+        adr_l \tmp2, \ptbl
+        str   \tmp3, [\tmp2, \tmp1, lsl #3]
+.endm
+
+/*
+ * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
+ * level table is supported.
+ *
+ * tbl:     table symbol where the entry will be created
+ * virt:    virtual address
+ * paddr:   physical address (should be page aligned)
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ * tmp3:    scratch register
+ * type:    mapping type. If not specified it will be normal memory 
(PT_MEM_L3)
+ *
+ * Preserves \virt, \paddr
+ * Clobbers \tmp1, \tmp2, \tmp3
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_mapping_entry, tbl, virt, paddr, tmp1, tmp2, tmp3, 
type=PT_MEM_L3
+        lsr   \tmp1, \virt, #THIRD_SHIFT
+        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+        mov   \tmp2, #\type                 /* \tmp2 := right for 
section PT */
+        orr   \tmp2, \tmp2, \paddr          /*          + paddr */
+        adr_l \tmp3, \tbl
+        str   \tmp2, [\tmp3, \tmp1, lsl #3]
+.endm
+
+/*
   * Rebuild the boot pagetable's first-level entries. The structure
   * is described in mm.c.
   *
@@ -495,100 +549,17 @@ ENDPROC(cpu_init)
   *   x19: paddr(start)
   *   x20: phys offset
   *
- * Clobbers x0 - x4, x25
- *
- * Register usage within this function:
- *   x25: Identity map in place
+ * Clobbers x0 - x4
   */
  create_page_tables:
-        /*
-         * If Xen is loaded at exactly XEN_VIRT_START then we don't
-         * need an additional 1:1 mapping, the virtual mapping will
-         * suffice.
-         */
-        cmp   x19, #XEN_VIRT_START
-        cset  x25, eq                /* x25 := identity map in place, 
or not */
-
-        load_paddr x4, boot_pgtable
-
-        /* Setup boot_pgtable: */
-        load_paddr x1, boot_first
-
-        /* ... map boot_first in boot_pgtable[0] */
-        mov   x3, #PT_PT             /* x2 := table map of boot_first */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #0]           /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_pgtable+boot_first_id */
-        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in 
boot_pgtable */
-        cbz   x1, 1f                 /* It's in slot 0, map in boot_first
-                                      * or boot_second later on */
-
-        /*
-         * Level zero does not support superpage mappings, so we have
-         * to use an extra first level page in which we create a 1GB 
mapping.
-         */
-        load_paddr x2, boot_first_id
-
-        mov   x3, #PT_PT             /* x2 := table map of boot_first_id */
-        orr   x2, x2, x3             /*       + rights for linear PT */
-        str   x2, [x4, x1, lsl #3]
-
-        load_paddr x4, boot_first_id
-
-        lsr   x1, x19, #FIRST_SHIFT  /* x1 := Offset of base paddr in 
boot_first_id */
-        lsl   x2, x1, #FIRST_SHIFT   /* x2 := Base address for 1GB 
mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        and   x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */
-        str   x2, [x4, x1, lsl #3]   /* Mapping of paddr(start) */
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_first: */
-        load_paddr x4, boot_first   /* Next level into boot_first */
-
-        /* ... map boot_second in boot_first[0] */
-        load_paddr x1, boot_second
-        mov   x3, #PT_PT             /* x2 := table map of boot_second */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #0]           /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_first */
-        cbnz  x25, 1f                /* x25 is set if already created */
-        lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in 
boot_first */
-        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
-        cbz   x1, 1f                 /* It's in slot 0, map in 
boot_second */
-
-        lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_second: */
-        load_paddr x4, boot_second
-
-        /* ... map boot_third in boot_second[1] */
-        load_paddr x1, boot_third
-        mov   x3, #PT_PT             /* x2 := table map of boot_third */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #8]           /* Map it in slot 1 */
-
-        /* ... map of paddr(start) in boot_second */
-        cbnz  x25, 1f                /* x25 is set if already created */
-        lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in 
boot_second */
-        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
-        cmp   x1, #1
-        b.eq  virtphys_clash         /* It's in slot 1, which we cannot 
handle */
+        /* Prepare the page-tables for mapping Xen */
+        ldr   x0, =XEN_VIRT_START
+        create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, 
x1, x2, x3
+        create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, 
x1, x2, x3
+        create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, 
x1, x2, x3

-        lsl   x2, x2, #SECOND_SHIFT  /* Base address for 2MB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_third: */
-        load_paddr x4, boot_third
+        /* Map Xen */
+        adr_l x4, boot_third

          lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
          lsl   x2, x2, #THIRD_SHIFT
@@ -603,21 +574,68 @@ create_page_tables:
          cmp   x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */
          b.lt  1b

-        /* Defer fixmap and dtb mapping until after paging enabled, to
-         * avoid them clashing with the 1:1 mapping. */
+        /*
+         * If Xen is loaded at exactly XEN_VIRT_START then we don't
+         * need an additional 1:1 mapping, the virtual mapping will
+         * suffice.
+         */
+        cmp   x19, #XEN_VIRT_START
+        bne   1f
+        ret
+1:
+        /*
+         * Only the first page of Xen will be part of the 1:1 mapping.
+         * All the boot_*_id tables are linked together even if they may
+         * not be all used. They will then be linked to the boot page
+         * tables at the correct level.
+         */
+        create_table_entry boot_first_id, boot_second_id, x19, 
FIRST_SHIFT, x0, x1, x2
+        create_table_entry boot_second_id, boot_third_id, x19, 
SECOND_SHIFT, x0, x1, x2
+        create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
+
+        /*
+         * Find the zeroeth slot used. Link boot_first_id into
+         * boot_pgtable if the slot is not 0. For slot 0, the tables
+         * associated with the 1:1 mapping will need to be linked in
+         * boot_first or boot_second.
+         */
+        lsr   x0, x19, #ZEROETH_SHIFT   /* x0 := zeroeth slot */
+        cbz   x0, 1f
+        /* It is not in slot 0, Link boot_first_id into boot_pgtable */
+        create_table_entry boot_pgtable, boot_first_id, x19, 
ZEROETH_SHIFT, x0, x1, x2
+        ret
+
+1:
+        /*
+         * Find the first slot used. Link boot_second_id into boot_first
+         * if the slot is not 0. For slot 0, the tables associated with
+         * the 1:1 mapping will need to be linked in boot_second.
+         */
+        lsr   x0, x19, #FIRST_SHIFT
+        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
+        cbz   x0, 1f
+        /* It is not in slot 0, Link boot_second_id into boot_first */
+        create_table_entry boot_first, boot_second_id, x19, 
FIRST_SHIFT, x0, x1, x2
+        ret

-        /* boot pagetable setup complete */
+1:
+        /*
+         * Find the second slot used. Link boot_third_id into boot_second
+         * if the slot is not 1 (runtime Xen mapping is 2M - 4M).
+         * For slot 1, Xen is not yet able to handle it.
+         */
+        lsr   x0, x19, #SECOND_SHIFT
+        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
+        cmp   x0, #1
+        beq   virtphys_clash
+        /* It is not in slot 1, link boot_third_id into boot_second */
+        create_table_entry boot_second, boot_third_id, x19, 
SECOND_SHIFT, x0, x1, x2
+        ret

-        cbnz  x25, 1f                /* Did we manage to create an 
identity mapping ? */
-        PRINT("Unable to build boot page tables - Failed to identity 
map Xen.\r\n")
-        b     fail
  virtphys_clash:
          /* Identity map clashes with boot_third, which we cannot 
handle yet */
          PRINT("- Unable to build boot page tables - virt and phys 
addresses clash. -\r\n")
          b     fail
-
-1:
-        ret
  ENDPROC(create_page_tables)

  /*
@@ -719,28 +737,15 @@ ENDPROC(remove_identity_mapping)
   * The fixmap cannot be mapped in create_page_tables because it may
   * clash with the 1:1 mapping.
   *
- * Clobbers x1 - x4
+ * Clobbers x0 - x3
   */
  setup_fixmap:
  #ifdef CONFIG_EARLY_PRINTK
-        /* Add UART to the fixmap table */
-        ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
-        lsr   x2, x23, #THIRD_SHIFT
-        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
-        mov   x3, #PT_DEV_L3
-        orr   x2, x2, x3             /* x2 := 4K dev map including UART */
-        str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first 
fixmap's slot */
+        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
+        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, 
type=PT_DEV_L3
  #endif
-
-        /* Map fixmap into boot_second */
-        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
-        load_paddr x2, xen_fixmap
-        mov   x3, #PT_PT
-        orr   x2, x2, x3             /* x2 := table map of xen_fixmap */
-        ldr   x1, =FIXMAP_ADDR(0)
-        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
-        str   x2, [x4, x1]           /* Map it in the fixmap's slot */
-
+        ldr   x0, =FIXMAP_ADDR(0)
+        create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, 
x1, x2, x3
          /* Ensure any page table updates made above have occurred */
          dsb   nshst
          ret
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c2f1795a71..bc1824d3ca 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -107,6 +107,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
  DEFINE_BOOT_PAGE_TABLE(boot_first);
  DEFINE_BOOT_PAGE_TABLE(boot_first_id);
  #endif
+DEFINE_BOOT_PAGE_TABLE(boot_second_id);
+DEFINE_BOOT_PAGE_TABLE(boot_third_id);
  DEFINE_BOOT_PAGE_TABLE(boot_second);
  DEFINE_BOOT_PAGE_TABLE(boot_third);


-- 
Julien Grall

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