[PATCH v2 0/8] Map Xen code/data/bss with superpages

[PATCH v2 0/8] Map Xen code/data/bss with superpages

[Andrew Cooper]

And make use of NX and RO attributes wherever possible.

After this series, the pagetable layout looks like:

(XEN) *** Dumping Xen text/data/bss mappings from ffff82d080000000
(XEN) cr3 0000000826484000, idle_pg_table ffff82d080818000, pa 00000000ac018000
(XEN) l2_xenmap: ffff82d080814000, pa 00000000ac014000
(XEN)  L4[261] = 00000000ac017163 X Gl S RW P
(XEN)   L3[322] = 00000000ac014163 X Gl S RW P
(XEN)    L2[000] = 00000000ab8001e1 X Gl + S RO P    <- .text
(XEN)    L2[001] = 00000000aba001a1 X Gl + S RO P    <- .text
(XEN)    L2[002] = 80000000abc001a1 NX Gl + S RO P   <- .rodata
                                                     <- discarded .init
(XEN)    L2[004] = 80000000ac0001e3 NX Gl + S RW P   <- .data and .bss
(XEN)    L2[511] = 000000084dcc7063 X S RW P         <- stubs


Andrew Cooper (8):
  xen/lockprof: Move .lockprofile.data into .rodata
  xen/x86: Improvements to build-time pagetable generation
  xen/x86: Construct the {l2,l3}_bootmap at compile time
  xen/memguard: Drop memguard_init() entirely
  xen/x86: Disable CR0.WP while applying alternatives
  xen/x86: Reorder .data and .init when linking
  xen/x86: Use 2M superpages for text/data/bss mappings
  xen/x86: Unilaterally remove .init mappings

 xen/arch/arm/xen.lds.S     |  14 +++----
 xen/arch/x86/alternative.c |   7 ++++
 xen/arch/x86/boot/head.S   |  18 +++-----
 xen/arch/x86/boot/x86_64.S |  64 ++++++++++++++++++++--------
 xen/arch/x86/mm.c          |  16 -------
 xen/arch/x86/setup.c       |  64 ++++++++++++++++++++--------
 xen/arch/x86/x86_64/mm.c   |   4 --
 xen/arch/x86/xen.lds.S     | 102 +++++++++++++++++++++++++++++----------------
 xen/include/asm-arm/mm.h   |   1 -
 xen/include/asm-x86/mm.h   |   2 -
 xen/include/xen/kernel.h   |   7 ++++
 xen/include/xen/spinlock.h |   2 +-
 12 files changed, 186 insertions(+), 115 deletions(-)

-- 
2.1.4

[PATCH v2 1/8] xen/lockprof: Move .lockprofile.data into .rodata

[Andrew Cooper]

The entire contents of .lockprofile.data are unchanging pointers to
lock_profile structure in .data.  Annotate the type as such, and link the
section in .rodata.  As these are just pointers, there is no need for 32byte
alignment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>

New in v2
---
 xen/arch/arm/xen.lds.S     | 14 +++++++-------
 xen/arch/x86/xen.lds.S     | 13 ++++++-------
 xen/include/xen/spinlock.h |  2 +-
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index f501a2f..6bbfba1 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -50,6 +50,13 @@ SECTIONS
        __stop_bug_frames_2 = .;
        *(.rodata)
        *(.rodata.*)
+
+#ifdef LOCK_PROFILE
+       __lock_profile_start = .;
+       *(.lockprofile.data)
+       __lock_profile_end = .;
+#endif
+
         _erodata = .;          /* End of read-only data */
   } :text
 
@@ -83,13 +90,6 @@ SECTIONS
        *(.data.rel.ro.*)
   } :text
 
-#ifdef LOCK_PROFILE
-  . = ALIGN(32);
-  __lock_profile_start = .;
-  .lockprofile.data : { *(.lockprofile.data) } :text
-  __lock_profile_end = .;
-#endif
-
   . = ALIGN(8);
   .arch.info : {
       _splatform = .;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 9fde1db..106067a 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -65,6 +65,12 @@ SECTIONS
 
        *(.rodata)
        *(.rodata.*)
+
+#ifdef LOCK_PROFILE
+       __lock_profile_start = .;
+       *(.lockprofile.data)
+       __lock_profile_end = .;
+#endif
   } :text
 
   . = ALIGN(SMP_CACHE_BYTES);
@@ -97,13 +103,6 @@ SECTIONS
        CONSTRUCTORS
   } :text
 
-#ifdef LOCK_PROFILE
-  . = ALIGN(32);
-  __lock_profile_start = .;
-  .lockprofile.data : { *(.lockprofile.data) } :text
-  __lock_profile_end = .;
-#endif
-
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 77ab7d5..88b53f9 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -79,7 +79,7 @@ struct lock_profile_qhead {
 
 #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 }
 #define _LOCK_PROFILE_PTR(name)                                               \
-    static struct lock_profile *__lock_profile_##name                         \
+    static struct lock_profile * const __lock_profile_##name                  \
     __used_section(".lockprofile.data") =                                     \
     &__lock_profile_data_##name
 #define _SPIN_LOCK_UNLOCKED(x) { { 0 }, SPINLOCK_NO_CPU, 0, _LOCK_DEBUG, x }
-- 
2.1.4

[PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Andrew Cooper]

 * Additional comments, including size and runtime use.
 * Consistent use of idx, rather than a mix including pfn.
 * Consistent use of .quad, rather than a mix including .long.
 * Consistent use of PAGE_HYPERVISOR for addresses in the Xen global range,
   which include _PAGE_GLOBAL.

No change in runtime behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

New in v2
---
 xen/arch/x86/boot/x86_64.S | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 94cf089..cc10932 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -86,32 +86,40 @@ GLOBAL(__page_tables_start)
  * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
  * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
  * of physical memory. In any case the VGA hole should be mapped with type UC.
+ * Uses 1x 4k page.
  */
 GLOBAL(l1_identmap)
-        pfn = 0
+        idx = 0
         .rept L1_PAGETABLE_ENTRIES
         /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
-        .if pfn >= 0xa0 && pfn < 0xc0
-        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
+        .if idx >= 0xa0 && idx < 0xc0
+        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
         .else
-        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
         .endif
-        .long 0
-        pfn = pfn + 1
+        idx = idx + 1
         .endr
         .size l1_identmap, . - l1_identmap
 
-/* Mapping of first 16 megabytes of memory. */
+/*
+ * Space for mapping the first 4GB of memory, with the first 16 megabytes
+ * actualy mapped (mostly using superpages).  Uses 4x 4k pages.
+ */
 GLOBAL(l2_identmap)
-        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
-        pfn = 0
+        .quad sym_phys(l1_identmap) + PAGE_HYPERVISOR
+        idx = 1
         .rept 7
-        pfn = pfn + (1 << PAGETABLE_ORDER)
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
+        .quad (idx << L2_PAGETABLE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
+        idx = idx + 1
         .endr
         .fill 4 * L2_PAGETABLE_ENTRIES - 8, 8, 0
         .size l2_identmap, . - l2_identmap
 
+/*
+ * 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)
         idx = 0
         .rept 8
@@ -121,11 +129,12 @@ GLOBAL(l2_xenmap)
         .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
         .size l2_xenmap, . - l2_xenmap
 
+/* L2 mapping the fixmap.  Uses 1x 4k page. */
 l2_fixmap:
         idx = 0
         .rept L2_PAGETABLE_ENTRIES
         .if idx == l2_table_offset(FIXADDR_TOP - 1)
-        .quad sym_phys(l1_fixmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l1_fixmap) + PAGE_HYPERVISOR
         .else
         .quad 0
         .endif
@@ -133,22 +142,24 @@ l2_fixmap:
         .endr
         .size l2_fixmap, . - l2_fixmap
 
+/* Identity map, covering the 4 l2_identmap tables.  Uses 1x 4k page. */
 GLOBAL(l3_identmap)
         idx = 0
         .rept 4
-        .quad sym_phys(l2_identmap) + (idx << PAGE_SHIFT) + __PAGE_HYPERVISOR
+        .quad sym_phys(l2_identmap) + (idx << PAGE_SHIFT) + PAGE_HYPERVISOR
         idx = idx + 1
         .endr
         .fill L3_PAGETABLE_ENTRIES - 4, 8, 0
         .size l3_identmap, . - l3_identmap
 
+/* L3 mapping the fixmap.  Uses 1x 4k page. */
 l3_xenmap:
         idx = 0
         .rept L3_PAGETABLE_ENTRIES
         .if idx == l3_table_offset(XEN_VIRT_START)
-        .quad sym_phys(l2_xenmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l2_xenmap) + PAGE_HYPERVISOR
         .elseif idx == l3_table_offset(FIXADDR_TOP - 1)
-        .quad sym_phys(l2_fixmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l2_fixmap) + PAGE_HYPERVISOR
         .else
         .quad 0
         .endif
@@ -162,9 +173,9 @@ GLOBAL(idle_pg_table)
         idx = 1
         .rept L4_PAGETABLE_ENTRIES - 1
         .if idx == l4_table_offset(DIRECTMAP_VIRT_START)
-        .quad sym_phys(l3_identmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l3_identmap) + PAGE_HYPERVISOR
         .elseif idx == l4_table_offset(XEN_VIRT_START)
-        .quad sym_phys(l3_xenmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l3_xenmap) + PAGE_HYPERVISOR
         .else
         .quad 0
         .endif
-- 
2.1.4

[PATCH v2 3/8] xen/x86: Construct the {l2, l3}_bootmap at compile time

[Andrew Cooper]

... rather than at runtime.

The bootmaps are discarded in zap_low_mappings(), so the tables themselves can
live in .init.data and be reclaimed after boot.

Hooking the l1_identmap into l2_xenmap stays for safety, along with a longer
comment explaining why.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

New in v2
---
 xen/arch/x86/boot/head.S   | 18 +++++-------------
 xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++
 xen/arch/x86/x86_64/mm.c   |  4 ----
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ac4962b..f3501fd 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -150,21 +150,13 @@ __start:
         mov     %eax,sym_phys(boot_tsc_stamp)
         mov     %edx,sym_phys(boot_tsc_stamp+4)
 
-        /* Initialise L2 boot-map page table entries (16MB). */
-        mov     $sym_phys(l2_bootmap),%edx
-        mov     $PAGE_HYPERVISOR|_PAGE_PSE,%eax
-        mov     $8,%ecx
-1:      mov     %eax,(%edx)
-        add     $8,%edx
-        add     $(1<<L2_PAGETABLE_SHIFT),%eax
-        loop    1b
-        /* Initialise L3 boot-map page directory entry. */
-        mov     $sym_phys(l2_bootmap)+__PAGE_HYPERVISOR,%eax
-        mov     %eax,sym_phys(l3_bootmap) + 0*8
-        /* Hook 4kB mappings of first 2MB of memory into L2. */
+        /*
+         * During boot, hook 4kB mappings of first 2MB of memory into L2.
+         * This avoids mixing cachability for the legacy VGA region, and is
+         * corrected when Xen relocates itself.
+         */
         mov     $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
         mov     %edi,sym_phys(l2_xenmap)
-        mov     %edi,sym_phys(l2_bootmap)
 
         /* Apply relocations to bootstrap trampoline. */
         mov     sym_phys(trampoline_phys),%edx
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index cc10932..4fea3f0 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -184,3 +184,22 @@ GLOBAL(idle_pg_table)
         .size idle_pg_table, . - idle_pg_table
 
 GLOBAL(__page_tables_end)
+
+/* Init pagetables.  Enough page directories to map into the bottom 1GB. */
+        .section .init.data, "a", @progbits
+        .align PAGE_SIZE, 0
+
+GLOBAL(l2_bootmap)
+        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
+        idx = 1
+        .rept 7
+        .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE
+        idx = idx + 1
+        .endr
+        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
+        .size l2_bootmap, . - l2_bootmap
+
+GLOBAL(l3_bootmap)
+        .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR
+        .fill L3_PAGETABLE_ENTRIES - 1, 8, 0
+        .size l3_bootmap, . - l3_bootmap
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 2228898..e07e69e 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -42,10 +42,6 @@ asm(".file \"" __FILE__ "\"");
 
 unsigned int __read_mostly m2p_compat_vstart = __HYPERVISOR_COMPAT_VIRT_START;
 
-/* Enough page directories to map into the bottom 1GB. */
-l3_pgentry_t __section(".bss.page_aligned") l3_bootmap[L3_PAGETABLE_ENTRIES];
-l2_pgentry_t __section(".bss.page_aligned") l2_bootmap[L2_PAGETABLE_ENTRIES];
-
 l2_pgentry_t *compat_idle_pg_table_l2;
 
 void *do_page_walk(struct vcpu *v, unsigned long addr)
-- 
2.1.4

[PATCH v2 4/8] xen/memguard: Drop memguard_init() entirely

[Andrew Cooper]

The use of MAP_SMALL_PAGES causes shattering of the superpages making up the
Xen virtual region, and is counter to the purpose of this series.
Furthermore, it is not required for the memguard infrastructure to function
(which itself uses map_pages_to_xen() for creating holes).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>

v2: Reword the commmit message
---
 xen/arch/x86/mm.c        | 16 ----------------
 xen/arch/x86/setup.c     |  2 --
 xen/include/asm-arm/mm.h |  1 -
 xen/include/asm-x86/mm.h |  2 --
 4 files changed, 21 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d6aaed8..ed8ab02 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6346,22 +6346,6 @@ void free_perdomain_mappings(struct domain *d)
 
 #ifdef MEMORY_GUARD
 
-void memguard_init(void)
-{
-    unsigned long start = max_t(unsigned long, xen_phys_start, 1UL << 20);
-    map_pages_to_xen(
-        (unsigned long)__va(start),
-        start >> PAGE_SHIFT,
-        (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
-        __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES);
-    BUG_ON(start != xen_phys_start);
-    map_pages_to_xen(
-        XEN_VIRT_START,
-        start >> PAGE_SHIFT,
-        (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
-        __PAGE_HYPERVISOR|MAP_SMALL_PAGES);
-}
-
 static void __memguard_change_range(void *p, unsigned long l, int guard)
 {
     unsigned long _p = (unsigned long)p;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b8a28d7..cddf954 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1146,8 +1146,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                    ~((1UL << L2_PAGETABLE_SHIFT) - 1);
     destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
 
-    memguard_init();
-
     nr_pages = 0;
     for ( i = 0; i < e820.nr_map; i++ )
         if ( e820.map[i].type == E820_RAM )
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 2e9d0b2..68cf203 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -331,7 +331,6 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
 
 extern struct domain *dom_xen, *dom_io, *dom_cow;
 
-#define memguard_init(_s)              (_s)
 #define memguard_guard_stack(_p)       ((void)0)
 #define memguard_guard_range(_p,_l)    ((void)0)
 #define memguard_unguard_range(_p,_l)  ((void)0)
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index a097382..23a4092 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -479,11 +479,9 @@ extern struct rangeset *mmio_ro_ranges;
 #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20))
 
 #ifdef MEMORY_GUARD
-void memguard_init(void);
 void memguard_guard_range(void *p, unsigned long l);
 void memguard_unguard_range(void *p, unsigned long l);
 #else
-#define memguard_init()                ((void)0)
 #define memguard_guard_range(_p,_l)    ((void)0)
 #define memguard_unguard_range(_p,_l)  ((void)0)
 #endif
-- 
2.1.4

[PATCH v2 5/8] xen/x86: Disable CR0.WP while applying alternatives

[Andrew Cooper]

In preparation for marking .text as read-only, care needs to be taken not to
fault while applying alternatives.

Swapping back to RW mappings is a possibility, but would require additional
TLB management.  A temporary disabling of CR0.WP is cleaner.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

New in v2.  (The one downside of my very-quick-to-reboot test box is that it
is sufficiently old to not have any alternatives needing patching.)
---
 xen/arch/x86/alternative.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 46ac0fd..d123fa7 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -147,11 +147,15 @@ static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
     struct alt_instr *a;
     u8 *instr, *replacement;
     u8 insnbuf[MAX_PATCH_LEN];
+    unsigned long cr0 = read_cr0();
 
     ASSERT(!local_irq_is_enabled());
 
     printk(KERN_INFO "alt table %p -> %p\n", start, end);
 
+    /* Disable WP to allow application of alternatives to read-only pages. */
+    write_cr0(cr0 & ~X86_CR0_WP);
+
     /*
      * The scan order should be from start to end. A later scanned
      * alternative code can overwrite a previous scanned alternative code.
@@ -181,6 +185,9 @@ static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
                  a->instrlen - a->replacementlen);
         text_poke_early(instr, insnbuf, a->instrlen);
     }
+
+    /* Reinstate WP. */
+    write_cr0(cr0);
 }
 
 void __init alternative_instructions(void)
-- 
2.1.4

[PATCH v2 6/8] xen/x86: Reorder .data and .init when linking

[Andrew Cooper]

In preparation for using superpage mappings, .data and .bss will both want to
be mapped as read-write.  By making them adjacent, they can share the same
superpage and will not require superpage alignment between themselves.

While making this change, fix an exposed alignment bug.  __init_end only needs
page alignment, while .bss.stack_aligned needs STACK_SIZE alignment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

New in v2.
---
 xen/arch/x86/xen.lds.S | 63 +++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 106067a..63dbcff 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -73,36 +73,6 @@ SECTIONS
 #endif
   } :text
 
-  . = ALIGN(SMP_CACHE_BYTES);
-  .data.read_mostly : {
-       /* Exception table */
-       __start___ex_table = .;
-       *(.ex_table)
-       __stop___ex_table = .;
-
-       /* Pre-exception table */
-       __start___pre_ex_table = .;
-       *(.ex_table.pre)
-       __stop___pre_ex_table = .;
-
-       *(.data.read_mostly)
-       . = ALIGN(8);
-       __start_schedulers_array = .;
-       *(.data.schedulers)
-       __end_schedulers_array = .;
-       *(.data.rel.ro)
-       *(.data.rel.ro.*)
-  } :text
-
-  .data : {                    /* Data */
-       . = ALIGN(PAGE_SIZE);
-       *(.data.page_aligned)
-       *(.data)
-       *(.data.rel)
-       *(.data.rel.*)
-       CONSTRUCTORS
-  } :text
-
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
@@ -162,11 +132,42 @@ SECTIONS
        *(.xsm_initcall.init)
        __xsm_initcall_end = .;
   } :text
-  . = ALIGN(STACK_SIZE);
+  . = ALIGN(PAGE_SIZE);
   __init_end = .;
 
+  . = ALIGN(SMP_CACHE_BYTES);
+  .data.read_mostly : {
+       /* Exception table */
+       __start___ex_table = .;
+       *(.ex_table)
+       __stop___ex_table = .;
+
+       /* Pre-exception table */
+       __start___pre_ex_table = .;
+       *(.ex_table.pre)
+       __stop___pre_ex_table = .;
+
+       *(.data.read_mostly)
+       . = ALIGN(8);
+       __start_schedulers_array = .;
+       *(.data.schedulers)
+       __end_schedulers_array = .;
+       *(.data.rel.ro)
+       *(.data.rel.ro.*)
+  } :text
+
+  .data : {                    /* Data */
+       . = ALIGN(PAGE_SIZE);
+       *(.data.page_aligned)
+       *(.data)
+       *(.data.rel)
+       *(.data.rel.*)
+       CONSTRUCTORS
+  } :text
+
   .bss : {                     /* BSS */
        __bss_start = .;
+       . = ALIGN(STACK_SIZE);
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
        *(.bss.page_aligned*)
-- 
2.1.4

[PATCH v2 7/8] xen/x86: Use 2M superpages for text/data/bss mappings

[Andrew Cooper]

This balloons the size of Xen in memory from 4.4MB to 8MB, because of the
required alignment adjustments.

However
 * All mappings are 2M superpages.
 * .text (and .init at boot) are the only sections marked executable.
 * .text and .rodata are marked read-only.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

v2:
 * .data and .bss are adjcent (from earlier patch), so don't require 2M
   alignment
---
 xen/arch/x86/setup.c     | 38 +++++++++++++++++++++++++++++++++++---
 xen/arch/x86/xen.lds.S   | 27 +++++++++++++++++++++++++++
 xen/include/xen/kernel.h |  7 +++++++
 3 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index cddf954..806fa95 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -920,14 +920,46 @@ 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
+             * is contained in this PTE.
+             */
             *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
-                                   PAGE_HYPERVISOR_RWX | _PAGE_PSE);
+                                   PAGE_HYPERVISOR_RX | _PAGE_PSE);
             for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
             {
+                unsigned int flags;
+
                 if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
                     continue;
-                *pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) +
-                                        xen_phys_start);
+
+                if ( i < l2_table_offset((unsigned long)&__2M_text_end) )
+                {
+                    flags = PAGE_HYPERVISOR_RX | _PAGE_PSE;
+                }
+                else if ( i >= l2_table_offset((unsigned long)&__2M_rodata_start) &&
+                          i <  l2_table_offset((unsigned long)&__2M_rodata_end) )
+                {
+                    flags = PAGE_HYPERVISOR_RO | _PAGE_PSE;
+                }
+                else if ( i >= l2_table_offset((unsigned long)&__2M_init_start) &&
+                          i <  l2_table_offset((unsigned long)&__2M_init_end) )
+                {
+                    flags = PAGE_HYPERVISOR_RWX | _PAGE_PSE;
+                }
+                else if ( (i >= l2_table_offset((unsigned long)&__2M_rwdata_start) &&
+                           i <  l2_table_offset((unsigned long)&__2M_rwdata_end)) )
+                {
+                    flags = PAGE_HYPERVISOR_RW | _PAGE_PSE;
+                }
+                else
+                {
+                    *pl2e = l2e_empty();
+                    continue;
+                }
+
+                *pl2e = l2e_from_paddr(
+                    l2e_get_paddr(*pl2e) + xen_phys_start, flags);
             }
 
             /* Re-sync the stack and then switch to relocated pagetables. */
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 63dbcff..aadb082 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -38,6 +38,9 @@ SECTIONS
   . = __XEN_VIRT_START;
   __image_base__ = .;
 #endif
+
+  __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
+
   . = __XEN_VIRT_START + MB(1);
   _start = .;
   .text : {
@@ -50,6 +53,10 @@ SECTIONS
        _etext = .;             /* End of text section */
   } :text = 0x9090
 
+  . = ALIGN(MB(2));
+  __2M_text_end = .;
+
+  __2M_rodata_start = .;       /* Start of 2M superpages, mapped RO. */
   .rodata : {
        /* Bug frames table */
        . = ALIGN(4);
@@ -73,6 +80,10 @@ SECTIONS
 #endif
   } :text
 
+  . = ALIGN(MB(2));
+  __2M_rodata_end = .;
+
+  __2M_init_start = .;         /* Start of 2M superpages, mapped RWX (boot only). */
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
@@ -135,6 +146,10 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);
   __init_end = .;
 
+  . = ALIGN(MB(2));
+  __2M_init_end = .;
+
+  __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
   . = ALIGN(SMP_CACHE_BYTES);
   .data.read_mostly : {
        /* Exception table */
@@ -183,6 +198,9 @@ SECTIONS
   } :text
   _end = . ;
 
+  . = ALIGN(MB(2));
+  __2M_rwdata_end = .;
+
 #ifdef EFI
   . = ALIGN(4);
   .reloc : {
@@ -229,4 +247,13 @@ ASSERT(__image_base__ > XEN_VIRT_START ||
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
+ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
+ASSERT(IS_ALIGNED(__2M_text_end,     MB(2)), "__2M_text_end misaligned")
+ASSERT(IS_ALIGNED(__2M_rodata_start, MB(2)), "__2M_rodata_start misaligned")
+ASSERT(IS_ALIGNED(__2M_rodata_end,   MB(2)), "__2M_rodata_end misaligned")
+ASSERT(IS_ALIGNED(__2M_init_start,   MB(2)), "__2M_init_start misaligned")
+ASSERT(IS_ALIGNED(__2M_init_end,     MB(2)), "__2M_init_end misaligned")
+ASSERT(IS_ALIGNED(__2M_rwdata_start, MB(2)), "__2M_rwdata_start misaligned")
+ASSERT(IS_ALIGNED(__2M_rwdata_end,   MB(2)), "__2M_rwdata_end misaligned")
+
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64d..8a48334 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -65,6 +65,13 @@
 	1;                                      \
 })
 
+#ifdef CONFIG_X86
+extern const char __2M_text_start[], __2M_text_end[];
+extern const char __2M_rodata_start[], __2M_rodata_end[];
+extern char __2M_init_start[], __2M_init_end[];
+extern char __2M_rwdata_start[], __2M_rwdata_end[];
+#endif
+
 extern char _start[], _end[], start[];
 #define is_kernel(p) ({                         \
     char *__p = (char *)(unsigned long)(p);     \
-- 
2.1.4

[PATCH v2 8/8] xen/x86: Unilaterally remove .init mappings

[Andrew Cooper]

Because of the new 2M alignment of .init and .bss, the existing memory
guarding infrastructure causes a shattered 2M superpage with non-present
entries for .init, and present entries for the alignment space.

Do away with the difference in behaviour between debug and non-debug builds;
always destroy the .init mappings, and reuse the space for xenheap.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/setup.c   | 24 +++++++++++-------------
 xen/arch/x86/xen.lds.S |  3 +++
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 806fa95..8431f06 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -176,16 +176,6 @@ void __init discard_initial_images(void)
     initial_images = NULL;
 }
 
-static void free_xen_data(char *s, char *e)
-{
-#ifndef MEMORY_GUARD
-    init_xenheap_pages(__pa(s), __pa(e));
-#endif
-    memguard_guard_range(s, e-s);
-    /* Also zap the mapping in the 1:1 area. */
-    memguard_guard_range(__va(__pa(s)), e-s);
-}
-
 extern char __init_begin[], __init_end[], __bss_start[], __bss_end[];
 
 static void __init init_idle_domain(void)
@@ -509,13 +499,21 @@ static void __init kexec_reserve_area(struct e820map *e820)
 
 static void noinline init_done(void)
 {
+    void *va;
+
     system_state = SYS_STATE_active;
 
     domain_unpause_by_systemcontroller(hardware_domain);
 
-    /* Free (or page-protect) the init areas. */
-    memset(__init_begin, 0xcc, __init_end - __init_begin); /* int3 poison */
-    free_xen_data(__init_begin, __init_end);
+    /* Zero the .init code and data. */
+    for ( va = __init_begin; va < _p(__init_end); va += PAGE_SIZE )
+        clear_page(va);
+
+    /* Destroy Xen's mappings, and reuse the pages. */
+    destroy_xen_mappings((unsigned long)&__2M_init_start,
+                         (unsigned long)&__2M_init_end);
+    init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
+
     printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
 
     startup_cpu_idle_loop();
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index aadb082..f3908d1 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -257,3 +257,6 @@ ASSERT(IS_ALIGNED(__2M_rwdata_start, MB(2)), "__2M_rwdata_start misaligned")
 ASSERT(IS_ALIGNED(__2M_rwdata_end,   MB(2)), "__2M_rwdata_end misaligned")
 
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
+
+ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned")
+ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZE), "__init_end misaligned")
-- 
2.1.4

Re: [PATCH v2 1/8] xen/lockprof: Move .lockprofile.data into .rodata

[Jan Beulich]

>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -65,6 +65,12 @@ SECTIONS
>  
>         *(.rodata)
>         *(.rodata.*)
> +
> +#ifdef LOCK_PROFILE
> +       __lock_profile_start = .;
> +       *(.lockprofile.data)
> +       __lock_profile_end = .;
> +#endif
>    } :text
>  
>    . = ALIGN(SMP_CACHE_BYTES);
> @@ -97,13 +103,6 @@ SECTIONS
>         CONSTRUCTORS
>    } :text
>  
> -#ifdef LOCK_PROFILE
> -  . = ALIGN(32);
> -  __lock_profile_start = .;
> -  .lockprofile.data : { *(.lockprofile.data) } :text
> -  __lock_profile_end = .;
> -#endif

As I've said on a different patch of yours where you also moved
stuff around - an alignment of 32 is not needed here, but completely
dropping the ALIGN() is wrong, since __lock_profile_start may end
up misaligned (and not on the start of the first .lockprofile.data
section).

Jan

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Jan Beulich]

 >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> * Additional comments, including size and runtime use.
>  * Consistent use of idx, rather than a mix including pfn.

I don't see anything wrong with this - when we have a PFN why
should we not call it a PFN? As opposed to when we have a table
index not representing the corresponding PFN.

>  GLOBAL(l1_identmap)
> -        pfn = 0
> +        idx = 0
>          .rept L1_PAGETABLE_ENTRIES
>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
> -        .if pfn >= 0xa0 && pfn < 0xc0
> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
> +        .if idx >= 0xa0 && idx < 0xc0
> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>          .else
> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR

Please don't eliminate the MAP_SMALL_PAGES here, they serve an
(at least documentation) purpose.

> -/* Mapping of first 16 megabytes of memory. */
> +/*
> + * Space for mapping the first 4GB of memory, with the first 16 megabytes
> + * actualy mapped (mostly using superpages).  Uses 4x 4k pages.
> + */
>  GLOBAL(l2_identmap)
> -        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
> -        pfn = 0
> +        .quad sym_phys(l1_identmap) + PAGE_HYPERVISOR

This is wrong - the G bit is defined for leaf entries only.

Jan

Re: [PATCH v2 3/8] xen/x86: Construct the {l2, l3}_bootmap at compile time

[Jan Beulich]

>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> ---
>  xen/arch/x86/boot/head.S   | 18 +++++-------------
>  xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++
>  xen/arch/x86/x86_64/mm.c   |  4 ----
>  3 files changed, 24 insertions(+), 17 deletions(-)

Is this intentionally leaving the EFI equivalent untouched?

> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table)
>          .size idle_pg_table, . - idle_pg_table
>  
>  GLOBAL(__page_tables_end)
> +
> +/* Init pagetables.  Enough page directories to map into the bottom 1GB. */
> +        .section .init.data, "a", @progbits
> +        .align PAGE_SIZE, 0
> +
> +GLOBAL(l2_bootmap)
> +        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR

The relocation needed for this and ...

> +        idx = 1
> +        .rept 7
> +        .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE
> +        idx = idx + 1
> +        .endr
> +        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
> +        .size l2_bootmap, . - l2_bootmap
> +
> +GLOBAL(l3_bootmap)
> +        .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR

... this won't get properly adjusted by efi_arch_relocate_image(),
due to living outside of [__page_tables_start, __page_tables_end).

Jan

Re: [PATCH v2 3/8] xen/x86: Construct the {l2, l3}_bootmap at compile time

[Andrew Cooper]

On 24/02/16 11:34, Jan Beulich wrote:
>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>> ---
>>  xen/arch/x86/boot/head.S   | 18 +++++-------------
>>  xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++
>>  xen/arch/x86/x86_64/mm.c   |  4 ----
>>  3 files changed, 24 insertions(+), 17 deletions(-)
> Is this intentionally leaving the EFI equivalent untouched?

Yes.

>
>> --- a/xen/arch/x86/boot/x86_64.S
>> +++ b/xen/arch/x86/boot/x86_64.S
>> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table)
>>          .size idle_pg_table, . - idle_pg_table
>>  
>>  GLOBAL(__page_tables_end)
>> +
>> +/* Init pagetables.  Enough page directories to map into the bottom 1GB. */
>> +        .section .init.data, "a", @progbits
>> +        .align PAGE_SIZE, 0
>> +
>> +GLOBAL(l2_bootmap)
>> +        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
> The relocation needed for this and ...
>
>> +        idx = 1
>> +        .rept 7
>> +        .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE
>> +        idx = idx + 1
>> +        .endr
>> +        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
>> +        .size l2_bootmap, . - l2_bootmap
>> +
>> +GLOBAL(l3_bootmap)
>> +        .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR
> ... this won't get properly adjusted by efi_arch_relocate_image(),
> due to living outside of [__page_tables_start, __page_tables_end).

Deliberately so.

The EFI needs to relocate the tables anyway.  It currently (re)writes
them fresh at the relocated address, and leaving this be is the more
simple option.

~Andrew

Re: [PATCH v2 6/8] xen/x86: Reorder .data and .init when linking

[Jan Beulich]

>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> In preparation for using superpage mappings, .data and .bss will both want to
> be mapped as read-write.  By making them adjacent, they can share the same
> superpage and will not require superpage alignment between themselves.
> 
> While making this change, fix an exposed alignment bug.  __init_end only needs
> page alignment, while .bss.stack_aligned needs STACK_SIZE alignment.

Well, this has become a bug only with your changes (perhaps
that what you mean with "fix an exposed alignment bug", but
it reads as if there was a latent one, which isn't the case afaict).

>    .bss : {                     /* BSS */
>         __bss_start = .;
> +       . = ALIGN(STACK_SIZE);

These two lines should be swapped - there's no point in starting
the BSS ahead of the alignment, causing us to needlessly zero
a few more pages during boot.

Jan

Re: [PATCH v2 6/8] xen/x86: Reorder .data and .init when linking

[Andrew Cooper]

On 24/02/16 11:41, Jan Beulich wrote:
>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>> In preparation for using superpage mappings, .data and .bss will both want to
>> be mapped as read-write.  By making them adjacent, they can share the same
>> superpage and will not require superpage alignment between themselves.
>>
>> While making this change, fix an exposed alignment bug.  __init_end only needs
>> page alignment, while .bss.stack_aligned needs STACK_SIZE alignment.
> Well, this has become a bug only with your changes (perhaps
> that what you mean with "fix an exposed alignment bug", but
> it reads as if there was a latent one, which isn't the case afaict).

It is a latent bug.  The alignment directive for .bss.stack_aligned was
part of .init rather than .bss

>
>>    .bss : {                     /* BSS */
>>         __bss_start = .;
>> +       . = ALIGN(STACK_SIZE);
> These two lines should be swapped - there's no point in starting
> the BSS ahead of the alignment, causing us to needlessly zero
> a few more pages during boot.

Will do.

~Andrew

Re: [PATCH v2 3/8] xen/x86: Construct the {l2, l3}_bootmap at compile time

[Jan Beulich]

>>> On 24.02.16 at 12:40, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 11:34, Jan Beulich wrote:
>>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>> ---
>>>  xen/arch/x86/boot/head.S   | 18 +++++-------------
>>>  xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++
>>>  xen/arch/x86/x86_64/mm.c   |  4 ----
>>>  3 files changed, 24 insertions(+), 17 deletions(-)
>> Is this intentionally leaving the EFI equivalent untouched?
> 
> Yes.
> 
>>
>>> --- a/xen/arch/x86/boot/x86_64.S
>>> +++ b/xen/arch/x86/boot/x86_64.S
>>> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table)
>>>          .size idle_pg_table, . - idle_pg_table
>>>  
>>>  GLOBAL(__page_tables_end)
>>> +
>>> +/* Init pagetables.  Enough page directories to map into the bottom 1GB. */
>>> +        .section .init.data, "a", @progbits
>>> +        .align PAGE_SIZE, 0
>>> +
>>> +GLOBAL(l2_bootmap)
>>> +        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
>> The relocation needed for this and ...
>>
>>> +        idx = 1
>>> +        .rept 7
>>> +        .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE
>>> +        idx = idx + 1
>>> +        .endr
>>> +        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
>>> +        .size l2_bootmap, . - l2_bootmap
>>> +
>>> +GLOBAL(l3_bootmap)
>>> +        .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR
>> ... this won't get properly adjusted by efi_arch_relocate_image(),
>> due to living outside of [__page_tables_start, __page_tables_end).
> 
> Deliberately so.
> 
> The EFI needs to relocate the tables anyway.  It currently (re)writes
> them fresh at the relocated address, and leaving this be is the more
> simple option.

Okay, I guess you mean to say that both together continue to
work, which without it being said explicitly (allowing the implication
that this also has got tested) I was rather unsure about. Hence
I'd like to at least ask for an explicit respective statement in the
commit message.

Jan

Re: [PATCH v2 3/8] xen/x86: Construct the {l2, l3}_bootmap at compile time

[Andrew Cooper]

On 24/02/16 11:50, Jan Beulich wrote:
>>>> On 24.02.16 at 12:40, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 11:34, Jan Beulich wrote:
>>>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>> ---
>>>>  xen/arch/x86/boot/head.S   | 18 +++++-------------
>>>>  xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++
>>>>  xen/arch/x86/x86_64/mm.c   |  4 ----
>>>>  3 files changed, 24 insertions(+), 17 deletions(-)
>>> Is this intentionally leaving the EFI equivalent untouched?
>> Yes.
>>
>>>> --- a/xen/arch/x86/boot/x86_64.S
>>>> +++ b/xen/arch/x86/boot/x86_64.S
>>>> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table)
>>>>          .size idle_pg_table, . - idle_pg_table
>>>>  
>>>>  GLOBAL(__page_tables_end)
>>>> +
>>>> +/* Init pagetables.  Enough page directories to map into the bottom 1GB. */
>>>> +        .section .init.data, "a", @progbits
>>>> +        .align PAGE_SIZE, 0
>>>> +
>>>> +GLOBAL(l2_bootmap)
>>>> +        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
>>> The relocation needed for this and ...
>>>
>>>> +        idx = 1
>>>> +        .rept 7
>>>> +        .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE
>>>> +        idx = idx + 1
>>>> +        .endr
>>>> +        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
>>>> +        .size l2_bootmap, . - l2_bootmap
>>>> +
>>>> +GLOBAL(l3_bootmap)
>>>> +        .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR
>>> ... this won't get properly adjusted by efi_arch_relocate_image(),
>>> due to living outside of [__page_tables_start, __page_tables_end).
>> Deliberately so.
>>
>> The EFI needs to relocate the tables anyway.  It currently (re)writes
>> them fresh at the relocated address, and leaving this be is the more
>> simple option.
> Okay, I guess you mean to say that both together continue to
> work, which without it being said explicitly (allowing the implication
> that this also has got tested) I was rather unsure about. Hence
> I'd like to at least ask for an explicit respective statement in the
> commit message.

Ok.  The other reason why leaving the EFI side along is that these
tables can't both be in .init.data and covered by [__page_tables_start,
__page_tables_end).

~Andrew

Re: [PATCH v2 7/8] xen/x86: Use 2M superpages for text/data/bss mappings

[Jan Beulich]

>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> This balloons the size of Xen in memory from 4.4MB to 8MB, because of the
> required alignment adjustments.

Interesting - on v1 it was 12Mb iirc, and aiui you folded just one
pair of 2M pages, which would yield 10M now; did you perhaps
not account for .text spanning 2 large pages, due to the 1M bias
it starts at? But anyway...

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -65,6 +65,13 @@
>  	1;                                      \
>  })
>  
> +#ifdef CONFIG_X86
> +extern const char __2M_text_start[], __2M_text_end[];
> +extern const char __2M_rodata_start[], __2M_rodata_end[];
> +extern char __2M_init_start[], __2M_init_end[];
> +extern char __2M_rwdata_start[], __2M_rwdata_end[];
> +#endif

I think this would better go into an x86-specific header. After all
there's no point for this patch to touch anything that's not x86-
specific anyway.

Jan

Re: [PATCH v2 7/8] xen/x86: Use 2M superpages for text/data/bss mappings

[Andrew Cooper]

On 24/02/16 13:17, Jan Beulich wrote:
>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>> This balloons the size of Xen in memory from 4.4MB to 8MB, because of the
>> required alignment adjustments.
> Interesting - on v1 it was 12Mb iirc, and aiui you folded just one
> pair of 2M pages, which would yield 10M now; did you perhaps
> not account for .text spanning 2 large pages, due to the 1M bias
> it starts at? But anyway...

The original 12 was me just counting 6 superpages.  I forgot to account
for the 1 released back because of .init (which I suppose is now in the
following patch).  That, combined with the alignment drop results in 4
in-use superpages after boot.

~Andrew

Re: [PATCH v2 7/8] xen/x86: Use 2M superpages for text/data/bss mappings

[Andrew Cooper]

On 24/02/16 13:21, Andrew Cooper wrote:
> On 24/02/16 13:17, Jan Beulich wrote:
>>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>> This balloons the size of Xen in memory from 4.4MB to 8MB, because of the
>>> required alignment adjustments.
>> Interesting - on v1 it was 12Mb iirc, and aiui you folded just one
>> pair of 2M pages, which would yield 10M now; did you perhaps
>> not account for .text spanning 2 large pages, due to the 1M bias
>> it starts at? But anyway...
> The original 12 was me just counting 6 superpages.  I forgot to account
> for the 1 released back because of .init (which I suppose is now in the
> following patch).  That, combined with the alignment drop results in 4
> in-use superpages after boot.

This is perhaps more clear given the debugging shown in the cover letter.

~Andrew

Re: [PATCH v2 4/8] xen/memguard: Drop memguard_init() entirely

[Jan Beulich]

>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> The use of MAP_SMALL_PAGES causes shattering of the superpages making up the
> Xen virtual region, and is counter to the purpose of this series.
> Furthermore, it is not required for the memguard infrastructure to function
> (which itself uses map_pages_to_xen() for creating holes).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

Did you perhaps mean to Cc Stefano instead for the (trivial) ARM
change?

> v2: Reword the commmit message
> ---
>  xen/arch/x86/mm.c        | 16 ----------------
>  xen/arch/x86/setup.c     |  2 --
>  xen/include/asm-arm/mm.h |  1 -
>  xen/include/asm-x86/mm.h |  2 --
>  4 files changed, 21 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d6aaed8..ed8ab02 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6346,22 +6346,6 @@ void free_perdomain_mappings(struct domain *d)
>  
>  #ifdef MEMORY_GUARD
>  
> -void memguard_init(void)
> -{
> -    unsigned long start = max_t(unsigned long, xen_phys_start, 1UL << 20);
> -    map_pages_to_xen(
> -        (unsigned long)__va(start),
> -        start >> PAGE_SHIFT,
> -        (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
> -        __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES);
> -    BUG_ON(start != xen_phys_start);
> -    map_pages_to_xen(
> -        XEN_VIRT_START,
> -        start >> PAGE_SHIFT,
> -        (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
> -        __PAGE_HYPERVISOR|MAP_SMALL_PAGES);
> -}
> -
>  static void __memguard_change_range(void *p, unsigned long l, int guard)
>  {
>      unsigned long _p = (unsigned long)p;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index b8a28d7..cddf954 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1146,8 +1146,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                     ~((1UL << L2_PAGETABLE_SHIFT) - 1);
>      destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + 
> BOOTSTRAP_MAP_BASE);
>  
> -    memguard_init();
> -
>      nr_pages = 0;
>      for ( i = 0; i < e820.nr_map; i++ )
>          if ( e820.map[i].type == E820_RAM )
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 2e9d0b2..68cf203 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -331,7 +331,6 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
>  
>  extern struct domain *dom_xen, *dom_io, *dom_cow;
>  
> -#define memguard_init(_s)              (_s)
>  #define memguard_guard_stack(_p)       ((void)0)
>  #define memguard_guard_range(_p,_l)    ((void)0)
>  #define memguard_unguard_range(_p,_l)  ((void)0)
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index a097382..23a4092 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -479,11 +479,9 @@ extern struct rangeset *mmio_ro_ranges;
>  #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 
> 20))
>  
>  #ifdef MEMORY_GUARD
> -void memguard_init(void);
>  void memguard_guard_range(void *p, unsigned long l);
>  void memguard_unguard_range(void *p, unsigned long l);
>  #else
> -#define memguard_init()                ((void)0)
>  #define memguard_guard_range(_p,_l)    ((void)0)
>  #define memguard_unguard_range(_p,_l)  ((void)0)
>  #endif
> -- 
> 2.1.4

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Andrew Cooper]

On 24/02/16 11:24, Jan Beulich wrote:
>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>> * Additional comments, including size and runtime use.
>>  * Consistent use of idx, rather than a mix including pfn.
> I don't see anything wrong with this - when we have a PFN why
> should we not call it a PFN? As opposed to when we have a table
> index not representing the corresponding PFN.

In the case of l2_identmap, the logic is more clearly expressed in terms
of idx.  That leaves l1_identmap as the odd one out, where idx and pfn
are equally applicable.

>
>>  GLOBAL(l1_identmap)
>> -        pfn = 0
>> +        idx = 0
>>          .rept L1_PAGETABLE_ENTRIES
>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>> -        .if pfn >= 0xa0 && pfn < 0xc0
>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>> +        .if idx >= 0xa0 && idx < 0xc0
>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>          .else
>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
> (at least documentation) purpose.

How?  Its in a l1 so are necessarily small pages, and the other l1's
don't use the constant.

~Andrew

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Jan Beulich]

>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 11:24, Jan Beulich wrote:
>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>  GLOBAL(l1_identmap)
>>> -        pfn = 0
>>> +        idx = 0
>>>          .rept L1_PAGETABLE_ENTRIES
>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>> +        .if idx >= 0xa0 && idx < 0xc0
>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>          .else
>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>> (at least documentation) purpose.
> 
> How?  Its in a l1 so are necessarily small pages, and the other l1's
> don't use the constant.

MAP_SMALL_PAGES documents (and enforces) that the mappings
shouldn't be re-combined into 2M ones, even if - after adjustments
to the other attributes - they could be.

Jan

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Andrew Cooper]

On 24/02/16 14:15, Jan Beulich wrote:
>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 11:24, Jan Beulich wrote:
>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>  GLOBAL(l1_identmap)
>>>> -        pfn = 0
>>>> +        idx = 0
>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>          .else
>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>> (at least documentation) purpose.
>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>> don't use the constant.
> MAP_SMALL_PAGES documents (and enforces) that the mappings
> shouldn't be re-combined into 2M ones, even if - after adjustments
> to the other attributes - they could be.

In which case, is actively wrong.  Were the cacheabilities to change
(e.g. booting HVMLite and knowing that there was no legacy VGA hole),
the mappings should be recombined into a 2M superpage.

~Andrew

Re: [PATCH v2 4/8] xen/memguard: Drop memguard_init() entirely

[Stefano Stabellini]

On Wed, 24 Feb 2016, Jan Beulich wrote:
> >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> > The use of MAP_SMALL_PAGES causes shattering of the superpages making up the
> > Xen virtual region, and is counter to the purpose of this series.
> > Furthermore, it is not required for the memguard infrastructure to function
> > (which itself uses map_pages_to_xen() for creating holes).
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> > CC: Tim Deegan <tim@xen.org>
> > CC: Ian Campbell <Ian.Campbell@citrix.com>
> 
> Did you perhaps mean to Cc Stefano instead for the (trivial) ARM
> change?

For the ARM changes:

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> > v2: Reword the commmit message
> > ---
> >  xen/arch/x86/mm.c        | 16 ----------------
> >  xen/arch/x86/setup.c     |  2 --
> >  xen/include/asm-arm/mm.h |  1 -
> >  xen/include/asm-x86/mm.h |  2 --
> >  4 files changed, 21 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index d6aaed8..ed8ab02 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -6346,22 +6346,6 @@ void free_perdomain_mappings(struct domain *d)
> >  
> >  #ifdef MEMORY_GUARD
> >  
> > -void memguard_init(void)
> > -{
> > -    unsigned long start = max_t(unsigned long, xen_phys_start, 1UL << 20);
> > -    map_pages_to_xen(
> > -        (unsigned long)__va(start),
> > -        start >> PAGE_SHIFT,
> > -        (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
> > -        __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES);
> > -    BUG_ON(start != xen_phys_start);
> > -    map_pages_to_xen(
> > -        XEN_VIRT_START,
> > -        start >> PAGE_SHIFT,
> > -        (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
> > -        __PAGE_HYPERVISOR|MAP_SMALL_PAGES);
> > -}
> > -
> >  static void __memguard_change_range(void *p, unsigned long l, int guard)
> >  {
> >      unsigned long _p = (unsigned long)p;
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index b8a28d7..cddf954 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1146,8 +1146,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >                     ~((1UL << L2_PAGETABLE_SHIFT) - 1);
> >      destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + 
> > BOOTSTRAP_MAP_BASE);
> >  
> > -    memguard_init();
> > -
> >      nr_pages = 0;
> >      for ( i = 0; i < e820.nr_map; i++ )
> >          if ( e820.map[i].type == E820_RAM )
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index 2e9d0b2..68cf203 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -331,7 +331,6 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
> >  
> >  extern struct domain *dom_xen, *dom_io, *dom_cow;
> >  
> > -#define memguard_init(_s)              (_s)
> >  #define memguard_guard_stack(_p)       ((void)0)
> >  #define memguard_guard_range(_p,_l)    ((void)0)
> >  #define memguard_unguard_range(_p,_l)  ((void)0)
> > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> > index a097382..23a4092 100644
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -479,11 +479,9 @@ extern struct rangeset *mmio_ro_ranges;
> >  #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 
> > 20))
> >  
> >  #ifdef MEMORY_GUARD
> > -void memguard_init(void);
> >  void memguard_guard_range(void *p, unsigned long l);
> >  void memguard_unguard_range(void *p, unsigned long l);
> >  #else
> > -#define memguard_init()                ((void)0)
> >  #define memguard_guard_range(_p,_l)    ((void)0)
> >  #define memguard_unguard_range(_p,_l)  ((void)0)
> >  #endif
> > -- 
> > 2.1.4
> 
> 
> 

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Jan Beulich]

>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 14:15, Jan Beulich wrote:
>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>  GLOBAL(l1_identmap)
>>>>> -        pfn = 0
>>>>> +        idx = 0
>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>          .else
>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>> (at least documentation) purpose.
>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>> don't use the constant.
>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>> shouldn't be re-combined into 2M ones, even if - after adjustments
>> to the other attributes - they could be.
> 
> In which case, is actively wrong.  Were the cacheabilities to change
> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
> the mappings should be recombined into a 2M superpage.

No, I think there are reasons (to do with fixed range MTRRs and
errata) which require us to keep these as small pages even if they
could be recombined.

Jan

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Andrew Cooper]

On 24/02/16 15:18, Jan Beulich wrote:
>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>  GLOBAL(l1_identmap)
>>>>>> -        pfn = 0
>>>>>> +        idx = 0
>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>          .else
>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>> (at least documentation) purpose.
>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>> don't use the constant.
>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>> to the other attributes - they could be.
>> In which case, is actively wrong.  Were the cacheabilities to change
>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>> the mappings should be recombined into a 2M superpage.
> No, I think there are reasons (to do with fixed range MTRRs and
> errata)

Any idea about which generation this might apply to?

>  which require us to keep these as small pages even if they
> could be recombined.

In the HVMLite case, MTRRs can be sensibly be disable in the guest which
would also allow the host EPT mappings to recombine back to a superpage.

~Andrew

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Jan Beulich]

>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 15:18, Jan Beulich wrote:
>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>  GLOBAL(l1_identmap)
>>>>>>> -        pfn = 0
>>>>>>> +        idx = 0
>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>          .else
>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>> (at least documentation) purpose.
>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>> don't use the constant.
>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>> to the other attributes - they could be.
>>> In which case, is actively wrong.  Were the cacheabilities to change
>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>> the mappings should be recombined into a 2M superpage.
>> No, I think there are reasons (to do with fixed range MTRRs and
>> errata)
> 
> Any idea about which generation this might apply to?

Just read the SDM sub-section "Large Page Size Considerations"
inside the section on MTRRs.

Jan

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Andrew Cooper]

On 24/02/16 15:48, Jan Beulich wrote:
>>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 15:18, Jan Beulich wrote:
>>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>  GLOBAL(l1_identmap)
>>>>>>>> -        pfn = 0
>>>>>>>> +        idx = 0
>>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>>          .else
>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>>> (at least documentation) purpose.
>>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>>> don't use the constant.
>>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>>> to the other attributes - they could be.
>>>> In which case, is actively wrong.  Were the cacheabilities to change
>>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>>> the mappings should be recombined into a 2M superpage.
>>> No, I think there are reasons (to do with fixed range MTRRs and
>>> errata)
>> Any idea about which generation this might apply to?
> Just read the SDM sub-section "Large Page Size Considerations"
> inside the section on MTRRs.

Right, and all that says is "don't accidentally mix cacheabilities
between paging and MTRRs".

In the example of an HVMLite guest, It is entirely reasonable (and
indeed preferable) to avoid shattered superpages (both guest and host)
for mapping gfn 0.  The best case is that an HVMLite guest ends up as an
exact multiple of 1GB, and uses 1GB HAP superpages.

~Andrew

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Jan Beulich]

>>> On 24.02.16 at 17:14, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 15:48, Jan Beulich wrote:
>>>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>> On 24/02/16 15:18, Jan Beulich wrote:
>>>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>  GLOBAL(l1_identmap)
>>>>>>>>> -        pfn = 0
>>>>>>>>> +        idx = 0
>>>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>>>          .else
>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>>>> (at least documentation) purpose.
>>>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>>>> don't use the constant.
>>>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>>>> to the other attributes - they could be.
>>>>> In which case, is actively wrong.  Were the cacheabilities to change
>>>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>>>> the mappings should be recombined into a 2M superpage.
>>>> No, I think there are reasons (to do with fixed range MTRRs and
>>>> errata)
>>> Any idea about which generation this might apply to?
>> Just read the SDM sub-section "Large Page Size Considerations"
>> inside the section on MTRRs.
> 
> Right, and all that says is "don't accidentally mix cacheabilities
> between paging and MTRRs".

Exactly. But we don't check back with the MTRRs when deciding
whether to re-combine a large page. Hence that flag to prevent
any such attempt.

Plus it also says something about a performance impact when
nevertheless using a large page for the first 2 or 4 MB.

Jan

Re: [PATCH v2 2/8] xen/x86: Improvements to build-time pagetable generation

[Andrew Cooper]

On 24/02/16 16:59, Jan Beulich wrote:
>>>> On 24.02.16 at 17:14, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 15:48, Jan Beulich wrote:
>>>>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/02/16 15:18, Jan Beulich wrote:
>>>>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>>  GLOBAL(l1_identmap)
>>>>>>>>>> -        pfn = 0
>>>>>>>>>> +        idx = 0
>>>>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>>>>          .else
>>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>>>>> (at least documentation) purpose.
>>>>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>>>>> don't use the constant.
>>>>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>>>>> to the other attributes - they could be.
>>>>>> In which case, is actively wrong.  Were the cacheabilities to change
>>>>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>>>>> the mappings should be recombined into a 2M superpage.
>>>>> No, I think there are reasons (to do with fixed range MTRRs and
>>>>> errata)
>>>> Any idea about which generation this might apply to?
>>> Just read the SDM sub-section "Large Page Size Considerations"
>>> inside the section on MTRRs.
>> Right, and all that says is "don't accidentally mix cacheabilities
>> between paging and MTRRs".
> Exactly. But we don't check back with the MTRRs when deciding
> whether to re-combine a large page. Hence that flag to prevent
> any such attempt.
>
> Plus it also says something about a performance impact when
> nevertheless using a large page for the first 2 or 4 MB.

The performance impact is only noted in relation to using "the most
conservative caching options".

I.e. if you use a 2M superpage and set UC because of mixed MTRRs, this
is a performance impact as the tradeoff against avoiding undefined
behaviour.

I will reinstate the bits for now, but I do intend them to be removed
longterm.

~Andrew