[Xen-devel] [PATCH 0/4] x86: Remove 16M total-size restriction

[Xen-devel] [PATCH 0/4] x86: Remove 16M total-size restriction

[Andrew Cooper]

It turns out that the note in c/s a8d27a54cc9cc

  Finally, leave a linker assert covering the fact that plenty of code blindly
  assumes that Xen is less that 16M.  This wants fixing in due course.

was easier to address than I had originally anticipated.  This series does so.

The end result can be tested by trying to boot with:

  diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
  index 759827a19a..fa83a9a28f 100644
  --- a/xen/arch/x86/setup.c
  +++ b/xen/arch/x86/setup.c
  @@ -52,6 +52,8 @@
   #include <asm/spec_ctrl.h>
   #include <asm/guest.h>

  +static uint8_t __used big_data[MB(16)] = { 42, };
  +
   /* opt_nosmp: If true, secondary processors are ignored. */
   static bool __initdata opt_nosmp;
   boolean_param("nosmp", opt_nosmp);

Before this series, Xen will triple fault in one of several places (first and
most obviously, __high_start on the first stack access, as cpu0_stack[] is
very near the end of Xen's linked image).

Andrew Cooper (4):
  x86/boot: Rename l?_identmap to l?_directmap
  x86/page: Remove bifurcated PAGE_HYPERVISOR constant
  x86/boot: Create the l2_xenmap[] mappings dynamically
  x86/boot: Size the boot/directmap mappings dynamically

 xen/arch/x86/boot/head.S          | 35 +++++++++++++++++++++-------
 xen/arch/x86/boot/x86_64.S        | 49 +++++++++++++++++----------------------
 xen/arch/x86/efi/efi-boot.h       | 43 +++++++++++++++++++++++++++-------
 xen/arch/x86/setup.c              |  6 ++---
 xen/arch/x86/xen.lds.S            |  6 ++---
 xen/include/asm-x86/page.h        |  2 +-
 xen/include/asm-x86/x86_64/page.h |  7 ------
 7 files changed, 90 insertions(+), 58 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 1/4] x86/boot: Rename l?_identmap to l?_directmap

[Andrew Cooper]

Since c/s faa85d4fb3 "x86/boot: Don't map 0 during boot", l1_identmap no
longer has an alias mapped at 0, meaning that none of the l?_identmap[]
pagetables are actually an identity map.

Rename them to l?_directmap, which avoids any kind of implication that they
might be mapped at 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S    |  2 +-
 xen/arch/x86/boot/x86_64.S  | 22 +++++++++++-----------
 xen/arch/x86/efi/efi-boot.h |  8 ++++----
 xen/arch/x86/setup.c        |  6 +++---
 xen/include/asm-x86/page.h  |  2 +-
 5 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d246e374f1..aaf0e119db 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -678,7 +678,7 @@ trampoline_setup:
         shr     $(L2_PAGETABLE_SHIFT-3),%ebx
         mov     $8,%ecx
 1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
-        mov     %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
+        mov     %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
         sub     $(1<<L2_PAGETABLE_SHIFT),%eax
         loop    1b
 
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index af62850589..c26eccea92 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -51,7 +51,7 @@ GLOBAL(stack_start)
  * of physical memory. In any case the VGA hole should be mapped with type UC.
  * Uses 1x 4k page.
  */
-l1_identmap:
+l1_directmap:
         pfn = 0
         .rept L1_PAGETABLE_ENTRIES
         /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
@@ -62,7 +62,7 @@ l1_identmap:
         .endif
         pfn = pfn + 1
         .endr
-        .size l1_identmap, . - l1_identmap
+        .size l1_directmap, . - l1_directmap
 
 /*
  * __page_tables_{start,end} cover the range of pagetables which need
@@ -73,12 +73,12 @@ GLOBAL(__page_tables_start)
 
 /*
  * Space for 4G worth of 2M mappings, first 2M actually mapped via
- * l1_identmap[].  Uses 4x 4k pages.
+ * l1_directmap[].  Uses 4x 4k pages.
  */
-GLOBAL(l2_identmap)
-        .quad sym_offs(l1_identmap) + __PAGE_HYPERVISOR
+GLOBAL(l2_directmap)
+        .quad sym_offs(l1_directmap) + __PAGE_HYPERVISOR
         .fill 4 * L2_PAGETABLE_ENTRIES - 1, 8, 0
-        .size l2_identmap, . - l2_identmap
+        .size l2_directmap, . - l2_directmap
 
 /*
  * L2 mapping the 1GB Xen text/data/bss region.  At boot it maps 16MB from
@@ -108,15 +108,15 @@ l2_fixmap:
         .endr
         .size l2_fixmap, . - l2_fixmap
 
-/* Identity map, covering the 4 l2_identmap tables.  Uses 1x 4k page. */
-l3_identmap:
+/* Direct map, initially covering the 4 l2_directmap tables.  Uses 1x 4k page. */
+l3_directmap:
         idx = 0
         .rept 4
-        .quad sym_offs(l2_identmap) + (idx << PAGE_SHIFT) + __PAGE_HYPERVISOR
+        .quad sym_offs(l2_directmap) + (idx << PAGE_SHIFT) + __PAGE_HYPERVISOR
         idx = idx + 1
         .endr
         .fill L3_PAGETABLE_ENTRIES - 4, 8, 0
-        .size l3_identmap, . - l3_identmap
+        .size l3_directmap, . - l3_directmap
 
 /* L3 mapping the fixmap.  Uses 1x 4k page. */
 l3_xenmap:
@@ -139,7 +139,7 @@ GLOBAL(idle_pg_table)
         idx = 1
         .rept L4_PAGETABLE_ENTRIES - 1
         .if idx == l4_table_offset(DIRECTMAP_VIRT_START)
-        .quad sym_offs(l3_identmap) + __PAGE_HYPERVISOR
+        .quad sym_offs(l3_directmap) + __PAGE_HYPERVISOR
         .elseif idx == l4_table_offset(XEN_VIRT_START)
         .quad sym_offs(l3_xenmap) + __PAGE_HYPERVISOR
         .else
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 203a9d3bb2..50d1499867 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -59,7 +59,7 @@ static void __init efi_arch_relocate_image(unsigned long delta)
         /*
          * Relevant l{2,3}_bootmap entries get initialized explicitly in
          * efi_arch_memory_setup(), so we must not apply relocations there.
-         * l2_identmap's first slot, otoh, should be handled normally, as
+         * l2_directmap's first slot, otoh, should be handled normally, as
          * efi_arch_memory_setup() won't touch it (xen_phys_start should
          * never be zero).
          */
@@ -586,8 +586,8 @@ static void __init efi_arch_memory_setup(void)
         return;
 
     /* Check that there is at least 4G of mapping space in l2_*map[] */
-    BUILD_BUG_ON((sizeof(l2_bootmap)  / L2_PAGETABLE_ENTRIES) < 4);
-    BUILD_BUG_ON((sizeof(l2_identmap) / L2_PAGETABLE_ENTRIES) < 4);
+    BUILD_BUG_ON((sizeof(l2_bootmap)   / L2_PAGETABLE_ENTRIES) < 4);
+    BUILD_BUG_ON((sizeof(l2_directmap) / L2_PAGETABLE_ENTRIES) < 4);
 
     /* Initialize L3 boot-map page directory entries. */
     for ( i = 0; i < 4; ++i )
@@ -603,7 +603,7 @@ static void __init efi_arch_memory_setup(void)
         unsigned int slot = (xen_phys_start >> L2_PAGETABLE_SHIFT) + i;
         paddr_t addr = slot << L2_PAGETABLE_SHIFT;
 
-        l2_identmap[slot] = l2e_from_paddr(addr, PAGE_HYPERVISOR|_PAGE_PSE);
+        l2_directmap[slot] = l2e_from_paddr(addr, PAGE_HYPERVISOR|_PAGE_PSE);
         l2_bootmap[slot] = l2e_from_paddr(addr, __PAGE_HYPERVISOR|_PAGE_PSE);
     }
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1b6ca4a47d..5bdc229bd6 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1031,7 +1031,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     for ( i = boot_e820.nr_map-1; i >= 0; i-- )
     {
         uint64_t s, e, mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
-        uint64_t end, limit = ARRAY_SIZE(l2_identmap) << L2_PAGETABLE_SHIFT;
+        uint64_t end, limit = ARRAY_SIZE(l2_directmap) << L2_PAGETABLE_SHIFT;
 
         if ( boot_e820.map[i].type != E820_RAM )
             continue;
@@ -1136,7 +1136,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             /* The only data mappings to be relocated are in the Xen area. */
             pl2e = __va(__pa(l2_xenmap));
             /*
-             * Undo the temporary-hooking of the l1_identmap.  __2M_text_start
+             * Undo the temporary-hooking of the l1_directmap.  __2M_text_start
              * is contained in this PTE.
              */
             BUG_ON(using_2M_mapping() &&
@@ -1349,7 +1349,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         /* Need to create mappings above PREBUILT_MAP_LIMIT. */
         map_s = max_t(uint64_t, s, PREBUILT_MAP_LIMIT);
         map_e = min_t(uint64_t, e,
-                      ARRAY_SIZE(l2_identmap) << L2_PAGETABLE_SHIFT);
+                      ARRAY_SIZE(l2_directmap) << L2_PAGETABLE_SHIFT);
 
         /* Pass mapped memory to allocator /before/ creating new mappings. */
         init_boot_pages(s, min(map_s, e));
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 05a8b1efa6..4b9a4fa33f 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -293,7 +293,7 @@ extern unsigned int   m2p_compat_vstart;
 extern l2_pgentry_t l2_xenmap[L2_PAGETABLE_ENTRIES],
     l2_bootmap[4*L2_PAGETABLE_ENTRIES];
 extern l3_pgentry_t l3_bootmap[L3_PAGETABLE_ENTRIES];
-extern l2_pgentry_t l2_identmap[4*L2_PAGETABLE_ENTRIES];
+extern l2_pgentry_t l2_directmap[4*L2_PAGETABLE_ENTRIES];
 extern l1_pgentry_t l1_fixmap[L1_PAGETABLE_ENTRIES];
 void paging_init(void);
 void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t);
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 1/4] x86/boot: Rename l?_identmap to l?_directmap

[Jan Beulich]

On 13.01.2020 18:50, Andrew Cooper wrote:
> Since c/s faa85d4fb3 "x86/boot: Don't map 0 during boot", l1_identmap no
> longer has an alias mapped at 0, meaning that none of the l?_identmap[]
> pagetables are actually an identity map.
> 
> Rename them to l?_directmap, which avoids any kind of implication that they
> might be mapped at 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

[Xen-devel] [PATCH 2/4] x86/page: Remove bifrucated PAGE_HYPERVISOR constant

[Andrew Cooper]

Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and
C code has nevertheless caused several bugs I should have known better about,
and contributed to review confusion.

There are exactly 4 uses of these constants in asm code (and one is shortly
going to disappear).

Instead of creating the constants which behave differently between ASM and C
code, expose all the constants and use non-ambuguous non-NX ones in ASM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S          | 2 +-
 xen/arch/x86/boot/x86_64.S        | 6 +++---
 xen/include/asm-x86/x86_64/page.h | 7 -------
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index aaf0e119db..c5acbf56ae 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -674,7 +674,7 @@ trampoline_setup:
          * the transition into long mode), using 2M superpages.
          */
         lea     sym_esi(start),%ebx
-        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
+        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
         shr     $(L2_PAGETABLE_SHIFT-3),%ebx
         mov     $8,%ecx
 1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index c26eccea92..aabf561b23 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -56,9 +56,9 @@ l1_directmap:
         .rept L1_PAGETABLE_ENTRIES
         /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
         .if pfn >= 0xa0 && pfn < 0xc0
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_UCMINUS | MAP_SMALL_PAGES
+        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
         .else
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
         .endif
         pfn = pfn + 1
         .endr
@@ -89,7 +89,7 @@ GLOBAL(l2_xenmap)
         .quad 0
         idx = 1
         .rept 7
-        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
+        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR_RWX | _PAGE_PSE)
         idx = idx + 1
         .endr
         .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
index 4fe0205553..1a4af85469 100644
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
 #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
 
-#ifdef __ASSEMBLY__
-/* Dependency on NX being available can't be expressed. */
-# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
-# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
-# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC      | _PAGE_GLOBAL)
-#else
 # define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
 # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
                                   _PAGE_GLOBAL | _PAGE_NX)
 # define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
                                   _PAGE_GLOBAL | _PAGE_NX)
-#endif
 
 #endif /* __X86_64_PAGE_H__ */
 
-- 
2.11.0


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

[Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Andrew Cooper]

Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and
C code has nevertheless caused several bugs I should have known better about,
and contributed to review confusion.

There are exactly 4 uses of these constants in asm code (and one is shortly
going to disappear).

Instead of creating the constants which behave differently between ASM and C
code, expose all the constants and use non-ambiguous non-NX ones in ASM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S          | 2 +-
 xen/arch/x86/boot/x86_64.S        | 6 +++---
 xen/include/asm-x86/x86_64/page.h | 7 -------
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index aaf0e119db..c5acbf56ae 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -674,7 +674,7 @@ trampoline_setup:
          * the transition into long mode), using 2M superpages.
          */
         lea     sym_esi(start),%ebx
-        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
+        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
         shr     $(L2_PAGETABLE_SHIFT-3),%ebx
         mov     $8,%ecx
 1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index c26eccea92..aabf561b23 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -56,9 +56,9 @@ l1_directmap:
         .rept L1_PAGETABLE_ENTRIES
         /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
         .if pfn >= 0xa0 && pfn < 0xc0
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_UCMINUS | MAP_SMALL_PAGES
+        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
         .else
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
         .endif
         pfn = pfn + 1
         .endr
@@ -89,7 +89,7 @@ GLOBAL(l2_xenmap)
         .quad 0
         idx = 1
         .rept 7
-        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
+        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR_RWX | _PAGE_PSE)
         idx = idx + 1
         .endr
         .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
index 4fe0205553..1a4af85469 100644
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
 #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
 
-#ifdef __ASSEMBLY__
-/* Dependency on NX being available can't be expressed. */
-# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
-# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
-# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC      | _PAGE_GLOBAL)
-#else
 # define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
 # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
                                   _PAGE_GLOBAL | _PAGE_NX)
 # define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
                                   _PAGE_GLOBAL | _PAGE_NX)
-#endif
 
 #endif /* __X86_64_PAGE_H__ */
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Jan Beulich]

On 13.01.2020 18:50, Andrew Cooper wrote:
> Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and
> C code has nevertheless caused several bugs I should have known better about,
> and contributed to review confusion.
> 
> There are exactly 4 uses of these constants in asm code (and one is shortly
> going to disappear).
> 
> Instead of creating the constants which behave differently between ASM and C
> code, expose all the constants and use non-ambiguous non-NX ones in ASM.

I'm okay with this in principle, but ...

> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
>  #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
>  #define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
>  
> -#ifdef __ASSEMBLY__
> -/* Dependency on NX being available can't be expressed. */
> -# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
> -# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC      | _PAGE_GLOBAL)
> -#else
>  # define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
>  # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
>                                    _PAGE_GLOBAL | _PAGE_NX)
>  # define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
>                                    _PAGE_GLOBAL | _PAGE_NX)

... I'm afraid the assembler error resulting from someone actually
(and mistakenly) using one of the constants making use of _PAGE_NX
is going to be rather cryptic. Which in turn may motivate people
to actually try to make _PAGE_NX "work" in assembly code. Therefore
I'd like to ask that together with the changes here _PAGE_NX's
#define be framed by #ifndef __ASSEMBLY__, such that any
diagnostic, if it mentions a symbol name, would name the actual
problem, rather than a derived one.

Furthermore from a style perspective the blanks between # and
"define" will also want to go away now.

Jan

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

Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Andrew Cooper]

On 14/01/2020 16:25, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/x86_64/page.h
>> +++ b/xen/include/asm-x86/x86_64/page.h
>> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
>>  #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
>>  #define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
>>  
>> -#ifdef __ASSEMBLY__
>> -/* Dependency on NX being available can't be expressed. */
>> -# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
>> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
>> -# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC      | _PAGE_GLOBAL)
>> -#else
>>  # define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
>>  # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
>>                                    _PAGE_GLOBAL | _PAGE_NX)
>>  # define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
>>                                    _PAGE_GLOBAL | _PAGE_NX)
> ... I'm afraid the assembler error resulting from someone actually
> (and mistakenly) using one of the constants making use of _PAGE_NX
> is going to be rather cryptic. Which in turn may motivate people
> to actually try to make _PAGE_NX "work" in assembly code. Therefore
> I'd like to ask that together with the changes here _PAGE_NX's
> #define be framed by #ifndef __ASSEMBLY__, such that any
> diagnostic, if it mentions a symbol name, would name the actual
> problem, rather than a derived one.

I can do this, but it doesn't make the error any less cryptic.

With _PAGE_NX hidden:

head.S: Assembler messages:
head.S:677: Error: invalid operands (*ABS* and *UND* sections) for `|'

With it visible:

head.S: Assembler messages:
head.S:677: Error: invalid character '?' in operand 1

I'm not aware of any way to get a useful symbol name out of the error.

~Andrew

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

Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Jan Beulich]

On 15.01.2020 13:53, Andrew Cooper wrote:
> On 14/01/2020 16:25, Jan Beulich wrote:
>>> --- a/xen/include/asm-x86/x86_64/page.h
>>> +++ b/xen/include/asm-x86/x86_64/page.h
>>> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
>>>  #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
>>>  #define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
>>>  
>>> -#ifdef __ASSEMBLY__
>>> -/* Dependency on NX being available can't be expressed. */
>>> -# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
>>> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
>>> -# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC      | _PAGE_GLOBAL)
>>> -#else
>>>  # define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
>>>  # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
>>>                                    _PAGE_GLOBAL | _PAGE_NX)
>>>  # define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
>>>                                    _PAGE_GLOBAL | _PAGE_NX)
>> ... I'm afraid the assembler error resulting from someone actually
>> (and mistakenly) using one of the constants making use of _PAGE_NX
>> is going to be rather cryptic. Which in turn may motivate people
>> to actually try to make _PAGE_NX "work" in assembly code. Therefore
>> I'd like to ask that together with the changes here _PAGE_NX's
>> #define be framed by #ifndef __ASSEMBLY__, such that any
>> diagnostic, if it mentions a symbol name, would name the actual
>> problem, rather than a derived one.
> 
> I can do this, but it doesn't make the error any less cryptic.
> 
> With _PAGE_NX hidden:
> 
> head.S: Assembler messages:
> head.S:677: Error: invalid operands (*ABS* and *UND* sections) for `|'

This is something that could be improved in the future in gas
(by simply naming the symbol found to be *UND*).

> With it visible:
> 
> head.S: Assembler messages:
> head.S:677: Error: invalid character '?' in operand 1

This, otoh, can't sensibly be expected to see an improvement.
(Well, perhaps the full line could be quoted, but that's true
for about every parsing error.)

Jan

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

[Xen-devel] [PATCH v2 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Andrew Cooper]

Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and
C code has nevertheless caused several bugs I should have known better about,
and contributed to review confusion.

There are exactly 4 uses of these constants in asm code (and one is shortly
going to disappear).

Instead of creating the constants which behave differently between ASM and C
code, expose all the constants and use non-ambiguous non-NX ones in ASM.
Adjust the hiding to just _PAGE_NX, which contains a C ternary expression.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Hide _PAGE_NX
---
 xen/arch/x86/boot/head.S          |  2 +-
 xen/arch/x86/boot/x86_64.S        |  6 +++---
 xen/include/asm-x86/page.h        |  4 ++++
 xen/include/asm-x86/x86_64/page.h | 17 +++++------------
 4 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d246e374f1..563bb19056 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -674,7 +674,7 @@ trampoline_setup:
          * the transition into long mode), using 2M superpages.
          */
         lea     sym_esi(start),%ebx
-        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
+        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
         shr     $(L2_PAGETABLE_SHIFT-3),%ebx
         mov     $8,%ecx
 1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index af62850589..662c8b7ead 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -56,9 +56,9 @@ l1_identmap:
         .rept L1_PAGETABLE_ENTRIES
         /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
         .if pfn >= 0xa0 && pfn < 0xc0
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_UCMINUS | MAP_SMALL_PAGES
+        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
         .else
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
         .endif
         pfn = pfn + 1
         .endr
@@ -89,7 +89,7 @@ GLOBAL(l2_xenmap)
         .quad 0
         idx = 1
         .rept 7
-        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE)
+        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR_RWX | _PAGE_PSE)
         idx = idx + 1
         .endr
         .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 05a8b1efa6..a3c76a403b 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -316,7 +316,11 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t);
 #define _PAGE_AVAIL    _AC(0xE00,U)
 #define _PAGE_PSE_PAT  _AC(0x1000,U)
 #define _PAGE_AVAIL_HIGH (_AC(0x7ff, U) << 12)
+
+#ifndef __ASSEMBLY__
+/* Dependency on NX being available can't be expressed. */
 #define _PAGE_NX       (cpu_has_nx ? _PAGE_NX_BIT : 0)
+#endif
 
 #define PAGE_CACHE_ATTRS (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)
 
diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
index 4fe0205553..9876634881 100644
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
 #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
 
-#ifdef __ASSEMBLY__
-/* Dependency on NX being available can't be expressed. */
-# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
-# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
-# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC      | _PAGE_GLOBAL)
-#else
-# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
-# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
-                                  _PAGE_GLOBAL | _PAGE_NX)
-# define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
-                                  _PAGE_GLOBAL | _PAGE_NX)
-#endif
+#define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
+#define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
+                                 _PAGE_GLOBAL | _PAGE_NX)
+#define PAGE_HYPERVISOR_UC      (__PAGE_HYPERVISOR_UC | \
+                                 _PAGE_GLOBAL | _PAGE_NX)
 
 #endif /* __X86_64_PAGE_H__ */
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Jan Beulich]

On 15.01.2020 15:08, Andrew Cooper wrote:
> Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and
> C code has nevertheless caused several bugs I should have known better about,
> and contributed to review confusion.
> 
> There are exactly 4 uses of these constants in asm code (and one is shortly
> going to disappear).
> 
> Instead of creating the constants which behave differently between ASM and C
> code, expose all the constants and use non-ambiguous non-NX ones in ASM.
> Adjust the hiding to just _PAGE_NX, which contains a C ternary expression.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

[Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Andrew Cooper]

The build-time construction of l2_xenmap[] imposes an arbitrary limit of 16M
total, which is a limit looking to be lifted.

Move l2_xenmap[] into the bss, and adjust both the BIOS and EFI paths to fill
it in dynamically, based on the final linked size of Xen.  For current builds,
this reduces the number of .text/etc mappings from 7 to 4.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

In principle, the non-EFI case could be made to work by having a post-link
script fill in a suitable number of _PAGE_PRESENT entries in l2_xenmap[].
This doesn't work for the EFI case, because pagetable relocation is instead
triggered on the ad-hoc relocation table, which would require the
_PAGE_PRESENT references to be in place before the link takes place.
---
 xen/arch/x86/boot/head.S    | 14 ++++++++++++++
 xen/arch/x86/boot/x86_64.S  | 23 ++++++++---------------
 xen/arch/x86/efi/efi-boot.h | 14 ++++++++++++++
 xen/arch/x86/xen.lds.S      |  3 +++
 4 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index c5acbf56ae..94bed4a2d3 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -668,6 +668,20 @@ trampoline_setup:
         add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
 2:      loop    1b
 
+        /* Map Xen into the higher mappings using 2M superpages. */
+        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
+        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        */
+        mov     $sym_offs(_end - 1), %edx
+        shr     $L2_PAGETABLE_SHIFT, %ecx   /* %ecx = First slot to write */
+        shr     $L2_PAGETABLE_SHIFT, %edx   /* %edx = Final slot to write */
+
+1:      mov     %eax, sym_offs(l2_xenmap)(%esi, %ecx, 8)
+        add     $1, %ecx
+        add     $1 << L2_PAGETABLE_SHIFT, %eax
+
+        cmp     %edx, %ecx
+        jbe     1b
+
         /*
          * Map Xen into the directmap (needed for early-boot pagetable
          * handling/walking), and identity map Xen into bootmap (needed for
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index aabf561b23..e63bece460 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -43,6 +43,14 @@ multiboot_ptr:
 GLOBAL(stack_start)
         .quad   cpu0_stack + STACK_SIZE - CPUINFO_sizeof
 
+        .section .bss.page_aligned, "aw", @nobits
+        .align PAGE_SIZE, 0
+
+/* L2 mapping the Xen text/data/bss region.  Uses 1x 4k page. */
+GLOBAL(l2_xenmap)
+        .fill L2_PAGETABLE_ENTRIES, 8, 0
+        .size l2_xenmap, . - l2_xenmap
+
         .section .data.page_aligned, "aw", @progbits
         .align PAGE_SIZE, 0
 /*
@@ -80,21 +88,6 @@ GLOBAL(l2_directmap)
         .fill 4 * L2_PAGETABLE_ENTRIES - 1, 8, 0
         .size l2_directmap, . - l2_directmap
 
-/*
- * L2 mapping the 1GB Xen text/data/bss region.  At boot it maps 16MB from
- * __image_base__, and is modified when Xen relocates itself.  Uses 1x 4k
- * page.
- */
-GLOBAL(l2_xenmap)
-        .quad 0
-        idx = 1
-        .rept 7
-        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR_RWX | _PAGE_PSE)
-        idx = idx + 1
-        .endr
-        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
-        .size l2_xenmap, . - l2_xenmap
-
 /* L2 mapping the fixmap.  Uses 1x 4k page. */
 l2_fixmap:
         idx = 0
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 50d1499867..e750db6f5c 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
     if ( !efi_enabled(EFI_LOADER) )
         return;
 
+    /*
+     * Map Xen into the higher mappings, using 2M superpages.
+     *
+     * NB: We are currently in physical mode, so a RIP-relative relocation
+     * against _start/_end gets their position as placed by the bootloader,
+     * not as expected in the final build.  This has arbitrary 2M alignment,
+     * so subtract xen_phys_start to get the appropriate slots in l2_xenmap[].
+     */
+    for ( i =  l2_table_offset((UINTN)_start   - xen_phys_start);
+          i <= l2_table_offset((UINTN)_end - 1 - xen_phys_start); ++i )
+        l2_xenmap[i] =
+            l2e_from_paddr(xen_phys_start + (i << L2_PAGETABLE_SHIFT),
+                           PAGE_HYPERVISOR_RWX | _PAGE_PSE);
+
     /* Check that there is at least 4G of mapping space in l2_*map[] */
     BUILD_BUG_ON((sizeof(l2_bootmap)   / L2_PAGETABLE_ENTRIES) < 4);
     BUILD_BUG_ON((sizeof(l2_directmap) / L2_PAGETABLE_ENTRIES) < 4);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 7f82f64078..7c351b9df3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -359,6 +359,9 @@ ASSERT(__image_base__ > XEN_VIRT_START |
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
+/* The Multiboot setup paths depend on this to simplify superpage PTE creation. */
+ASSERT(IS_ALIGNED(_start,            MB(2)), "_start misaligned")
+
 ASSERT(IS_ALIGNED(__2M_text_end,     SECTION_ALIGN), "__2M_text_end misaligned")
 ASSERT(IS_ALIGNED(__2M_rodata_start, SECTION_ALIGN), "__2M_rodata_start misaligned")
 ASSERT(IS_ALIGNED(__2M_rodata_end,   SECTION_ALIGN), "__2M_rodata_end misaligned")
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Jan Beulich]

On 13.01.2020 18:50, Andrew Cooper wrote:
> The build-time construction of l2_xenmap[] imposes an arbitrary limit of 16M
> total, which is a limit looking to be lifted.
> 
> Move l2_xenmap[] into the bss, and adjust both the BIOS and EFI paths to fill
> it in dynamically, based on the final linked size of Xen.  For current builds,
> this reduces the number of .text/etc mappings from 7 to 4.

Is the 4 named here applicable the same to xen.gz and xen.efi, despite
the latter using large pages with distinct permissions while the former
still doesn't?

> In principle, the non-EFI case could be made to work by having a post-link
> script fill in a suitable number of _PAGE_PRESENT entries in l2_xenmap[].
> This doesn't work for the EFI case, because pagetable relocation is instead
> triggered on the ad-hoc relocation table, which would require the
> _PAGE_PRESENT references to be in place before the link takes place.

And to be honest, such a post-link script would seem rather ugly
to have to me.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -668,6 +668,20 @@ trampoline_setup:
>          add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
>  2:      loop    1b
>  
> +        /* Map Xen into the higher mappings using 2M superpages. */
> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
> +        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        */

The comment is on the wrong line, isn't it? Perhaps

        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), \
                %eax                /* %eax = PTE to write        */

?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
>      if ( !efi_enabled(EFI_LOADER) )
>          return;
>  
> +    /*
> +     * Map Xen into the higher mappings, using 2M superpages.
> +     *
> +     * NB: We are currently in physical mode, so a RIP-relative relocation
> +     * against _start/_end gets their position as placed by the bootloader,
> +     * not as expected in the final build.  This has arbitrary 2M alignment,
> +     * so subtract xen_phys_start to get the appropriate slots in l2_xenmap[].
> +     */

It may just be a language issue, but I'm struggling with the
"arbitrary" here. Is this in any way related to the
--section-alignment=0x200000 option we pass to the linker (where
the value isn't arbitrary at all)?

Jan

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

Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Andrew Cooper]

On 14/01/2020 16:45, Jan Beulich wrote:
> On 13.01.2020 18:50, Andrew Cooper wrote:
>> The build-time construction of l2_xenmap[] imposes an arbitrary limit of 16M
>> total, which is a limit looking to be lifted.
>>
>> Move l2_xenmap[] into the bss, and adjust both the BIOS and EFI paths to fill
>> it in dynamically, based on the final linked size of Xen.  For current builds,
>> this reduces the number of .text/etc mappings from 7 to 4.
> Is the 4 named here applicable the same to xen.gz and xen.efi, despite
> the latter using large pages with distinct permissions while the former
> still doesn't?

TBH, I didn't check.  It will vary on the build anyway, and on
CONFIG_XEN_ALIGN_{DEFAULT,2M} (which is used to cause the shim xen.gz to
have 2M alignment.  I wonder how many people spotted this was a perf fix
for XSA-304...)

>> In principle, the non-EFI case could be made to work by having a post-link
>> script fill in a suitable number of _PAGE_PRESENT entries in l2_xenmap[].
>> This doesn't work for the EFI case, because pagetable relocation is instead
>> triggered on the ad-hoc relocation table, which would require the
>> _PAGE_PRESENT references to be in place before the link takes place.
> And to be honest, such a post-link script would seem rather ugly
> to have to me.

There are some post-link scripts which I'm intending to borrow from
Linux.  In particular, sorting the exception tables is a obvious thing
to do at build time rather than runtime.

A different one to consider is preparation of the IDT.  Mangling the
gates at build time would drop a moderate chunk of code, and allow for
earlier exception handling.

We have a lot of 32bit boot time code, which runs without any IDT. 
Adding even more 32bit code to construct an IDT is a waste of effort,
but if a build-time-prepared IDT were possible, the cost of one extra
lidt is a reasonable trade for a enough of a panic handler to at least
dump the registers, before continuing into a triple fault.

>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -668,6 +668,20 @@ trampoline_setup:
>>          add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
>>  2:      loop    1b
>>  
>> +        /* Map Xen into the higher mappings using 2M superpages. */
>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
>> +        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        */
> The comment is on the wrong line, isn't it? Perhaps
>
>         lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), \
>                 %eax                /* %eax = PTE to write        */
>
> ?

That is why the comment had the register name, rather than trying to
claim that $sym_offs(_start) was the PTE to write.

I didn't really think splitting the lea like that across 2 lines was
better than this.

How about /* %eax = PTE to write ^      */ which will point properly at
%eax?

>
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
>>      if ( !efi_enabled(EFI_LOADER) )
>>          return;
>>  
>> +    /*
>> +     * Map Xen into the higher mappings, using 2M superpages.
>> +     *
>> +     * NB: We are currently in physical mode, so a RIP-relative relocation
>> +     * against _start/_end gets their position as placed by the bootloader,
>> +     * not as expected in the final build.  This has arbitrary 2M alignment,
>> +     * so subtract xen_phys_start to get the appropriate slots in l2_xenmap[].
>> +     */
> It may just be a language issue, but I'm struggling with the
> "arbitrary" here. Is this in any way related to the
> --section-alignment=0x200000 option we pass to the linker (where
> the value isn't arbitrary at all)?

So this is the bug I spent ages trying to figure out console logging for.

The naive version of this loop (pre subtraction) ended up initialising
slots 173...177 which, when highlighted like that, is obviously why Xen
triple faulted when switching to the high mappings.

The point I'm trying to make is that l2_table_offset(_start) ends up
being junk because it is a rip-relative address and we're not running at
our linked address.  (It is in fact our physical position in memory's 2M
slot, modulo 512).

Subtracting xen_phys_start gets the number back into the same alias
which all the 32bit head.S code relies on, and gives us a sensible
sequence of slots starting from 1.

~Andrew

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

Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Jan Beulich]

On 14.01.2020 20:31, Andrew Cooper wrote:
> On 14/01/2020 16:45, Jan Beulich wrote:
>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -668,6 +668,20 @@ trampoline_setup:
>>>          add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
>>>  2:      loop    1b
>>>  
>>> +        /* Map Xen into the higher mappings using 2M superpages. */
>>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
>>> +        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        */
>> The comment is on the wrong line, isn't it? Perhaps
>>
>>         lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), \
>>                 %eax                /* %eax = PTE to write        */
>>
>> ?
> 
> That is why the comment had the register name, rather than trying to
> claim that $sym_offs(_start) was the PTE to write.
> 
> I didn't really think splitting the lea like that across 2 lines was
> better than this.
> 
> How about /* %eax = PTE to write ^      */ which will point properly at
> %eax?

Fine with me; I assume you mean this to go on a separate line?

>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
>>>      if ( !efi_enabled(EFI_LOADER) )
>>>          return;
>>>  
>>> +    /*
>>> +     * Map Xen into the higher mappings, using 2M superpages.
>>> +     *
>>> +     * NB: We are currently in physical mode, so a RIP-relative relocation
>>> +     * against _start/_end gets their position as placed by the bootloader,
>>> +     * not as expected in the final build.  This has arbitrary 2M alignment,
>>> +     * so subtract xen_phys_start to get the appropriate slots in l2_xenmap[].
>>> +     */
>> It may just be a language issue, but I'm struggling with the
>> "arbitrary" here. Is this in any way related to the
>> --section-alignment=0x200000 option we pass to the linker (where
>> the value isn't arbitrary at all)?
> 
> So this is the bug I spent ages trying to figure out console logging for.
> 
> The naive version of this loop (pre subtraction) ended up initialising
> slots 173...177 which, when highlighted like that, is obviously why Xen
> triple faulted when switching to the high mappings.
> 
> The point I'm trying to make is that l2_table_offset(_start) ends up
> being junk because it is a rip-relative address and we're not running at
> our linked address.  (It is in fact our physical position in memory's 2M
> slot, modulo 512).
> 
> Subtracting xen_phys_start gets the number back into the same alias
> which all the 32bit head.S code relies on, and gives us a sensible
> sequence of slots starting from 1.

Thanks for the explanation. What I'm still unclear about is this use
of "arbitrary", though. Looking at it again I guess I'm also
struggling to understand what "This" at the beginning of the sentence
refers to.

Jan

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

Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Andrew Cooper]

On 15/01/2020 09:23, Jan Beulich wrote:
> On 14.01.2020 20:31, Andrew Cooper wrote:
>> On 14/01/2020 16:45, Jan Beulich wrote:
>>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/boot/head.S
>>>> +++ b/xen/arch/x86/boot/head.S
>>>> @@ -668,6 +668,20 @@ trampoline_setup:
>>>>          add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
>>>>  2:      loop    1b
>>>>  
>>>> +        /* Map Xen into the higher mappings using 2M superpages. */
>>>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
>>>> +        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        */
>>> The comment is on the wrong line, isn't it? Perhaps
>>>
>>>         lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), \
>>>                 %eax                /* %eax = PTE to write        */
>>>
>>> ?
>> That is why the comment had the register name, rather than trying to
>> claim that $sym_offs(_start) was the PTE to write.
>>
>> I didn't really think splitting the lea like that across 2 lines was
>> better than this.
>>
>> How about /* %eax = PTE to write ^      */ which will point properly at
>> %eax?
> Fine with me; I assume you mean this to go on a separate line?

Yes.  i.e. exactly as the patch presented, but with an extra " ^" in the
comment.

>
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
>>>>      if ( !efi_enabled(EFI_LOADER) )
>>>>          return;
>>>>  
>>>> +    /*
>>>> +     * Map Xen into the higher mappings, using 2M superpages.
>>>> +     *
>>>> +     * NB: We are currently in physical mode, so a RIP-relative relocation
>>>> +     * against _start/_end gets their position as placed by the bootloader,
>>>> +     * not as expected in the final build.  This has arbitrary 2M alignment,
>>>> +     * so subtract xen_phys_start to get the appropriate slots in l2_xenmap[].
>>>> +     */
>>> It may just be a language issue, but I'm struggling with the
>>> "arbitrary" here. Is this in any way related to the
>>> --section-alignment=0x200000 option we pass to the linker (where
>>> the value isn't arbitrary at all)?
>> So this is the bug I spent ages trying to figure out console logging for.
>>
>> The naive version of this loop (pre subtraction) ended up initialising
>> slots 173...177 which, when highlighted like that, is obviously why Xen
>> triple faulted when switching to the high mappings.
>>
>> The point I'm trying to make is that l2_table_offset(_start) ends up
>> being junk because it is a rip-relative address and we're not running at
>> our linked address.  (It is in fact our physical position in memory's 2M
>> slot, modulo 512).
>>
>> Subtracting xen_phys_start gets the number back into the same alias
>> which all the 32bit head.S code relies on, and gives us a sensible
>> sequence of slots starting from 1.
> Thanks for the explanation. What I'm still unclear about is this use
> of "arbitrary", though. Looking at it again I guess I'm also
> struggling to understand what "This" at the beginning of the sentence
> refers to.

I'll attempt to rewrite the explanation from scratch and see if that
comes out clearer.

~Andrew

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

[Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

[Andrew Cooper]

... rather than presuming that 16M will do.  With this fixed, Xen is now
capable of booting even when its compiled size is larger than 16M.

On the EFI side, use l2e_add_flags() to reduce the code-generation overhead of
using l2e_from_paddr() twice.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

Can be tested by trying to boot with:

  diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
  index 759827a19a..fa83a9a28f 100644
  --- a/xen/arch/x86/setup.c
  +++ b/xen/arch/x86/setup.c
  @@ -52,6 +52,8 @@
   #include <asm/spec_ctrl.h>
   #include <asm/guest.h>

  +static uint8_t __used big_data[MB(16)] = { 42, };
  +
   /* opt_nosmp: If true, secondary processors are ignored. */
   static bool __initdata opt_nosmp;
   boolean_param("nosmp", opt_nosmp);

Before this series, Xen will triple fault in one of two places (both on the
transition to Xen's high mappings), both ultimately because of cpu0_stack[]
getting shifted off the top of the mappings.
---
 xen/arch/x86/boot/head.S    | 21 +++++++++++++--------
 xen/arch/x86/efi/efi-boot.h | 23 ++++++++++++++++++-----
 xen/arch/x86/xen.lds.S      |  3 ---
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 94bed4a2d3..eda3161fb1 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -687,14 +687,19 @@ trampoline_setup:
          * handling/walking), and identity map Xen into bootmap (needed for
          * the transition into long mode), using 2M superpages.
          */
-        lea     sym_esi(start),%ebx
-        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
-        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
-        mov     $8,%ecx
-1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
-        mov     %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
-        sub     $(1<<L2_PAGETABLE_SHIFT),%eax
-        loop    1b
+        lea     sym_esi(_start), %ecx
+        lea     -1 + sym_esi(_end), %edx
+        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */
+        shr     $L2_PAGETABLE_SHIFT, %ecx                   /* First slot to write. */
+        shr     $L2_PAGETABLE_SHIFT, %edx                   /* Final slot to write. */
+
+1:      mov     %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
+        mov     %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)
+        add     $1, %ecx
+        add     $1 << L2_PAGETABLE_SHIFT, %eax
+
+        cmp     %edx, %ecx
+        jbe     1b
 
         /* Initialize L3 boot-map page directory entries. */
         lea     __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index e750db6f5c..00df1736bd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -611,15 +611,28 @@ static void __init efi_arch_memory_setup(void)
      * Map Xen into the directmap (needed for early-boot pagetable
      * handling/walking), and identity map Xen into bootmap (needed for the
      * transition from the EFI pagetables to Xen), using 2M superpages.
+     *
+     * NB: We are currently in physical mode, so a RIP-relative relocation
+     * against _start/_end gets their real position in memory, which are the
+     * appropriate l2 slots to map.
      */
-    for ( i = 0; i < 8; ++i )
+#define l2_4G_offset(a)                                                 \
+    (((UINTN)(a) >> L2_PAGETABLE_SHIFT) & (4 * L2_PAGETABLE_ENTRIES - 1))
+
+    for ( i  = l2_4G_offset(_start);
+          i <= l2_4G_offset(_end - 1); ++i )
     {
-        unsigned int slot = (xen_phys_start >> L2_PAGETABLE_SHIFT) + i;
-        paddr_t addr = slot << L2_PAGETABLE_SHIFT;
+        l2_pgentry_t pte = l2e_from_paddr(i << L2_PAGETABLE_SHIFT,
+                                          __PAGE_HYPERVISOR | _PAGE_PSE);
+
+        l2_bootmap[i] = pte;
+
+        /* Bootmap RWX/Non-global.  Directmap RW/Global. */
+        l2e_add_flags(pte, PAGE_HYPERVISOR);
 
-        l2_directmap[slot] = l2e_from_paddr(addr, PAGE_HYPERVISOR|_PAGE_PSE);
-        l2_bootmap[slot] = l2e_from_paddr(addr, __PAGE_HYPERVISOR|_PAGE_PSE);
+        l2_directmap[i] = pte;
     }
+#undef l2_4G_offset
 }
 
 static void __init efi_arch_handle_module(struct file *file, const CHAR16 *name,
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 7c351b9df3..a71853a856 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
     "not enough room for trampoline and mbi data")
 ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
     "wakeup stack too small")
-
-/* Plenty of boot code assumes that Xen isn't larger than 16M. */
-ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

[Jan Beulich]

On 13.01.2020 18:50, Andrew Cooper wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -687,14 +687,19 @@ trampoline_setup:
>           * handling/walking), and identity map Xen into bootmap (needed for
>           * the transition into long mode), using 2M superpages.
>           */
> -        lea     sym_esi(start),%ebx
> -        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
> -        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
> -        mov     $8,%ecx
> -1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> -        mov     %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
> -        sub     $(1<<L2_PAGETABLE_SHIFT),%eax
> -        loop    1b
> +        lea     sym_esi(_start), %ecx
> +        lea     -1 + sym_esi(_end), %edx

This looks pretty odd - does

        lea     sym_esi(_end) - 1, %edx

not work?

> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */
> +        shr     $L2_PAGETABLE_SHIFT, %ecx                   /* First slot to write. */
> +        shr     $L2_PAGETABLE_SHIFT, %edx                   /* Final slot to write. */
> +
> +1:      mov     %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
> +        mov     %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)

I guess I could have noticed this on the previous patch already:
This would look better as

1:      mov     %eax, sym_esi(l2_bootmap,   %ecx, 8)
        mov     %eax, sym_esi(l2_directmap, %ecx, 8)

Can sym_esi() perhaps be made

#define sym_esi(sym, extra...)      sym_offs(sym)(%esi, ## extra)

?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
>      "not enough room for trampoline and mbi data")
>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>      "wakeup stack too small")
> -
> -/* Plenty of boot code assumes that Xen isn't larger than 16M. */
> -ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")

Following your reply to the cover letter, this can't be dropped just yet.
Even when that remaining issue got addressed, I think it would be better
to keep it, altering the bound to GB(1).

Jan

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

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

[Andrew Cooper]

On 14/01/2020 17:02, Jan Beulich wrote:
> On 13.01.2020 18:50, Andrew Cooper wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -687,14 +687,19 @@ trampoline_setup:
>>           * handling/walking), and identity map Xen into bootmap (needed for
>>           * the transition into long mode), using 2M superpages.
>>           */
>> -        lea     sym_esi(start),%ebx
>> -        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
>> -        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
>> -        mov     $8,%ecx
>> -1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
>> -        mov     %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
>> -        sub     $(1<<L2_PAGETABLE_SHIFT),%eax
>> -        loop    1b
>> +        lea     sym_esi(_start), %ecx
>> +        lea     -1 + sym_esi(_end), %edx
> This looks pretty odd - does
>
>         lea     sym_esi(_end) - 1, %edx
>
> not work?

No:

head.S: Assembler messages:
head.S:521: Error: junk `(%esi)-1' after expression

but it is not at all surprising when you expand the macro:

lea (_end - start)(%esi) - 1, %edx

The expression for the displacement ends up split across both sides of
the SIB.

>
>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */
>> +        shr     $L2_PAGETABLE_SHIFT, %ecx                   /* First slot to write. */
>> +        shr     $L2_PAGETABLE_SHIFT, %edx                   /* Final slot to write. */
>> +
>> +1:      mov     %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
>> +        mov     %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)
> I guess I could have noticed this on the previous patch already:
> This would look better as
>
> 1:      mov     %eax, sym_esi(l2_bootmap,   %ecx, 8)
>         mov     %eax, sym_esi(l2_directmap, %ecx, 8)
>
> Can sym_esi() perhaps be made
>
> #define sym_esi(sym, extra...)      sym_offs(sym)(%esi, ## extra)
>
> ?

I considered and dismissed this approach.  Yes, the code is slightly
shorter, but at the expense of readability.

The advantage of the longhand version is that it is obvious which half
is the displacement expression, and which half is the SIB.

The reduced version leaves a distinct possibility of %ecx being mistaken
as the base register, rather than the index.

>
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
>>      "not enough room for trampoline and mbi data")
>>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>>      "wakeup stack too small")
>> -
>> -/* Plenty of boot code assumes that Xen isn't larger than 16M. */
>> -ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
> Following your reply to the cover letter, this can't be dropped just yet.

Correct.

> Even when that remaining issue got addressed, I think it would be better
> to keep it, altering the bound to GB(1).

A 1G check wouldn't be correct.

We've already got a more suitable one, which is the check that Xen
doesn't encroach into the stubs range.

~Andrew

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

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

[Jan Beulich]

On 14.01.2020 18:27, Andrew Cooper wrote:
> On 14/01/2020 17:02, Jan Beulich wrote:
>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -687,14 +687,19 @@ trampoline_setup:
>>>           * handling/walking), and identity map Xen into bootmap (needed for
>>>           * the transition into long mode), using 2M superpages.
>>>           */
>>> -        lea     sym_esi(start),%ebx
>>> -        lea     (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR_RWX|_PAGE_PSE)(%ebx),%eax
>>> -        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
>>> -        mov     $8,%ecx
>>> -1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
>>> -        mov     %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
>>> -        sub     $(1<<L2_PAGETABLE_SHIFT),%eax
>>> -        loop    1b
>>> +        lea     sym_esi(_start), %ecx
>>> +        lea     -1 + sym_esi(_end), %edx
>> This looks pretty odd - does
>>
>>         lea     sym_esi(_end) - 1, %edx
>>
>> not work?
> 
> No:
> 
> head.S: Assembler messages:
> head.S:521: Error: junk `(%esi)-1' after expression
> 
> but it is not at all surprising when you expand the macro:
> 
> lea (_end - start)(%esi) - 1, %edx
> 
> The expression for the displacement ends up split across both sides of
> the SIB.

Hmm, seems I've mis-remembered that stuff ahead of ( and after
) gets concatenated.

>>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */
>>> +        shr     $L2_PAGETABLE_SHIFT, %ecx                   /* First slot to write. */
>>> +        shr     $L2_PAGETABLE_SHIFT, %edx                   /* Final slot to write. */
>>> +
>>> +1:      mov     %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
>>> +        mov     %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)
>> I guess I could have noticed this on the previous patch already:
>> This would look better as
>>
>> 1:      mov     %eax, sym_esi(l2_bootmap,   %ecx, 8)
>>         mov     %eax, sym_esi(l2_directmap, %ecx, 8)
>>
>> Can sym_esi() perhaps be made
>>
>> #define sym_esi(sym, extra...)      sym_offs(sym)(%esi, ## extra)
>>
>> ?
> 
> I considered and dismissed this approach.  Yes, the code is slightly
> shorter, but at the expense of readability.
> 
> The advantage of the longhand version is that it is obvious which half
> is the displacement expression, and which half is the SIB.
> 
> The reduced version leaves a distinct possibility of %ecx being mistaken
> as the base register, rather than the index.

With it being sym_esi() that gets used, I don't see any such risk.
But anyway, if you're convinced of the longer form being better,
so be it then.

>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
>>>      "not enough room for trampoline and mbi data")
>>>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>>>      "wakeup stack too small")
>>> -
>>> -/* Plenty of boot code assumes that Xen isn't larger than 16M. */
>>> -ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
>> Following your reply to the cover letter, this can't be dropped just yet.
> 
> Correct.
> 
>> Even when that remaining issue got addressed, I think it would be better
>> to keep it, altering the bound to GB(1).
> 
> A 1G check wouldn't be correct.
> 
> We've already got a more suitable one, which is the check that Xen
> doesn't encroach into the stubs range.

Oh, right. If only that check was correct. I guess it ought to be
using &, not |, and perhaps also __image_base__ == XEN_VIRT_START.
I'll give this a try and send a patch unless in the course of
doing so I realize there's a reason for it being the way it is.

Jan

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

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

[Jan Beulich]

On 15.01.2020 10:40, Jan Beulich wrote:
> On 14.01.2020 18:27, Andrew Cooper wrote:
>> On 14/01/2020 17:02, Jan Beulich wrote:
>>> Even when that remaining issue got addressed, I think it would be better
>>> to keep it, altering the bound to GB(1).
>>
>> A 1G check wouldn't be correct.
>>
>> We've already got a more suitable one, which is the check that Xen
>> doesn't encroach into the stubs range.
> 
> Oh, right. If only that check was correct. I guess it ought to be
> using &, not |, and perhaps also __image_base__ == XEN_VIRT_START.
> I'll give this a try and send a patch unless in the course of
> doing so I realize there's a reason for it being the way it is.

So the | is correct (to deal with the case of the intermediate
file getting linked at a different base address when producing
xen.efi), but probably misleading. I guess I'll submit a patch
anyway, despite the construct not being broken.

Jan

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

Re: [Xen-devel] [PATCH 0/4] x86: Remove 16M total-size restriction

[Andrew Cooper]

On 13/01/2020 17:50, Andrew Cooper wrote:
> It turns out that the note in c/s a8d27a54cc9cc
>
>   Finally, leave a linker assert covering the fact that plenty of code blindly
>   assumes that Xen is less that 16M.  This wants fixing in due course.
>
> was easier to address than I had originally anticipated.  This series does so.
>
> The end result can be tested by trying to boot with:
>
>   diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>   index 759827a19a..fa83a9a28f 100644
>   --- a/xen/arch/x86/setup.c
>   +++ b/xen/arch/x86/setup.c
>   @@ -52,6 +52,8 @@
>    #include <asm/spec_ctrl.h>
>    #include <asm/guest.h>
>
>   +static uint8_t __used big_data[MB(16)] = { 42, };
>   +
>    /* opt_nosmp: If true, secondary processors are ignored. */
>    static bool __initdata opt_nosmp;
>    boolean_param("nosmp", opt_nosmp);
>
> Before this series, Xen will triple fault in one of several places (first and
> most obviously, __high_start on the first stack access, as cpu0_stack[] is
> very near the end of Xen's linked image).

It turns out this is incomplete.  I've found another hardcoded 16M
intertwined with the trampoline relocation logic.

The code presented here is fine and ready for submission, with the
exception of the hunk in the final patch which drops the linker assertion.

~Andrew

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