[PATCH] xen/x86: Map Xen code/data/bss with superpages

[PATCH] xen/x86: Map Xen code/data/bss with superpages

[Andrew Cooper]

And make use of NX and RO attributes wherever possible.

Andrew Cooper (4):
  xen: Introduce IS_ALIGNED()
  xen/memguard: Drop memguard_init() entirely
  xen/x86: Use 2M superpages for text/data/bss mappings
  xen/x86: Unilaterally remove .init mappings

 xen/arch/x86/mm.c          | 24 +++-------------
 xen/arch/x86/setup.c       | 70 ++++++++++++++++++++++++++++++++++------------
 xen/arch/x86/xen.lds.S     | 38 ++++++++++++++++++++++++-
 xen/include/asm-arm/mm.h   |  1 -
 xen/include/asm-x86/mm.h   |  2 --
 xen/include/xen/config.h   |  2 ++
 xen/include/xen/kernel.h   |  6 ++++
 xen/include/xen/tmem_xen.h |  3 +-
 8 files changed, 102 insertions(+), 44 deletions(-)

-- 
2.1.4

[PATCH] xen: Introduce IS_ALIGNED()

[Andrew Cooper]

And a few open-coded alignment checks which I encountered

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>
---
 xen/arch/x86/mm.c          | 8 ++++----
 xen/arch/x86/xen.lds.S     | 2 +-
 xen/include/xen/config.h   | 2 ++
 xen/include/xen/tmem_xen.h | 3 +--
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ea3f9f2..d6aaed8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5957,8 +5957,8 @@ void destroy_xen_mappings(unsigned long s, unsigned long e)
     unsigned int  i;
     unsigned long v = s;
 
-    ASSERT((s & ~PAGE_MASK) == 0);
-    ASSERT((e & ~PAGE_MASK) == 0);
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
 
     while ( v < e )
     {
@@ -6369,8 +6369,8 @@ static void __memguard_change_range(void *p, unsigned long l, int guard)
     unsigned int flags = __PAGE_HYPERVISOR_RW | MAP_SMALL_PAGES;
 
     /* Ensure we are dealing with a page-aligned whole number of pages. */
-    ASSERT((_p&~PAGE_MASK) == 0);
-    ASSERT((_l&~PAGE_MASK) == 0);
+    ASSERT(IS_ALIGNED(_p, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(_l, PAGE_SIZE));
 
     if ( guard )
         flags &= ~_PAGE_PRESENT;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 3b199ca..9fde1db 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -229,4 +229,4 @@ ASSERT(__image_base__ > XEN_VIRT_START ||
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
-ASSERT((cpu0_stack & (STACK_SIZE - 1)) == 0, "cpu0_stack misaligned")
+ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
index a992933..bd78176 100644
--- a/xen/include/xen/config.h
+++ b/xen/include/xen/config.h
@@ -74,6 +74,8 @@
 #define MB(_mb)     (_AC(_mb, ULL) << 20)
 #define GB(_gb)     (_AC(_gb, ULL) << 30)
 
+#define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
+
 #define __STR(...) #__VA_ARGS__
 #define STR(...) __STR(__VA_ARGS__)
 
diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h
index 0fdbf68..c770f3e 100644
--- a/xen/include/xen/tmem_xen.h
+++ b/xen/include/xen/tmem_xen.h
@@ -23,8 +23,7 @@
 #endif
 typedef uint32_t pagesize_t;  /* like size_t, must handle largest PAGE_SIZE */
 
-#define IS_PAGE_ALIGNED(addr) \
-  ((void *)((((unsigned long)addr + (PAGE_SIZE - 1)) & PAGE_MASK)) == addr)
+#define IS_PAGE_ALIGNED(addr) IS_ALIGNED((unsigned long)(addr), PAGE_SIZE)
 #define IS_VALID_PAGE(_pi)  ( mfn_valid(page_to_mfn(_pi)) )
 
 extern struct page_list_head tmem_page_list;
-- 
2.1.4

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

[Andrew Cooper]

It is not obvious what this code is doing.  Most of it dates from 2007/2008,
and there have been substantial changes in Xen's memory handling since then.

It was previously optional, and isn't needed for any of the memguard
infrastructure to function.  The use of MAP_SMALL_PAGES causes needless
shattering of superpages.

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>
---
 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] xen/memguard: Drop memguard_init() entirely

[Jan Beulich]

>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
> It is not obvious what this code is doing.  Most of it dates from 2007/2008,
> and there have been substantial changes in Xen's memory handling since then.

Deleting code which isn't understood what it is or was once used
for is sub-optimal.

> It was previously optional, and isn't needed for any of the memguard
> infrastructure to function.  The use of MAP_SMALL_PAGES causes needless
> shattering of superpages.

Perhaps that's what is its purpose? Let's ask Keir, whom you didn't
even Cc.

Jan

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

[Andrew Cooper]

On 19/02/16 14:44, Jan Beulich wrote:
>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>> It is not obvious what this code is doing.  Most of it dates from 2007/2008,
>> and there have been substantial changes in Xen's memory handling since then.
> Deleting code which isn't understood what it is or was once used
> for is sub-optimal.
>
>> It was previously optional, and isn't needed for any of the memguard
>> infrastructure to function.  The use of MAP_SMALL_PAGES causes needless
>> shattering of superpages.
> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't
> even Cc.

I don't see this patch being different in nature to your "x86: drop
failsafe callback invocation from assembly".

As I explain in the second paragraph, these calls are strictly optional,
as they are omitted for release builds.  They also have no impact on the
rest of the memguard infrastructure to function, as
__memguard_change_range() also uses map_pages_to_xen().

So despite not being sure why it is like it is, I am stating with that
it is not needed with Xen in its current form.

~Andrew

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

[Jan Beulich]

>>> On 19.02.16 at 17:18, <andrew.cooper3@citrix.com> wrote:
> On 19/02/16 14:44, Jan Beulich wrote:
>>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>>> It is not obvious what this code is doing.  Most of it dates from 2007/2008,
>>> and there have been substantial changes in Xen's memory handling since then.
>> Deleting code which isn't understood what it is or was once used
>> for is sub-optimal.
>>
>>> It was previously optional, and isn't needed for any of the memguard
>>> infrastructure to function.  The use of MAP_SMALL_PAGES causes needless
>>> shattering of superpages.
>> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't
>> even Cc.
> 
> I don't see this patch being different in nature to your "x86: drop
> failsafe callback invocation from assembly".

That other patch explains why the code is (and never was)
necessary, whereas you just guess.

> As I explain in the second paragraph, these calls are strictly optional,
> as they are omitted for release builds.  They also have no impact on the
> rest of the memguard infrastructure to function, as
> __memguard_change_range() also uses map_pages_to_xen().
> 
> So despite not being sure why it is like it is, I am stating with that
> it is not needed with Xen in its current form.

I actually think the reason is to avoid the memory allocation which
might result the first time a 2M page gets split up, as that memory
allocation might fail (which nowadays gets a proper -ENOMEM
communicated out of map_pages_to_xen(), but that hasn't been
the case in the early days).

Jan

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

[Andrew Cooper]

On 22/02/16 10:02, Jan Beulich wrote:
>>>> On 19.02.16 at 17:18, <andrew.cooper3@citrix.com> wrote:
>> On 19/02/16 14:44, Jan Beulich wrote:
>>>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>>>> It is not obvious what this code is doing.  Most of it dates from 2007/2008,
>>>> and there have been substantial changes in Xen's memory handling since then.
>>> Deleting code which isn't understood what it is or was once used
>>> for is sub-optimal.
>>>
>>>> It was previously optional, and isn't needed for any of the memguard
>>>> infrastructure to function.  The use of MAP_SMALL_PAGES causes needless
>>>> shattering of superpages.
>>> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't
>>> even Cc.
>> I don't see this patch being different in nature to your "x86: drop
>> failsafe callback invocation from assembly".
> That other patch explains why the code is (and never was)
> necessary, whereas you just guess.
>
>> As I explain in the second paragraph, these calls are strictly optional,
>> as they are omitted for release builds.  They also have no impact on the
>> rest of the memguard infrastructure to function, as
>> __memguard_change_range() also uses map_pages_to_xen().
>>
>> So despite not being sure why it is like it is, I am stating with that
>> it is not needed with Xen in its current form.
> I actually think the reason is to avoid the memory allocation which
> might result the first time a 2M page gets split up, as that memory
> allocation might fail (which nowadays gets a proper -ENOMEM
> communicated out of map_pages_to_xen(), but that hasn't been
> the case in the early days).

And isn't the case anywhere in the memguard infrastructure.

At the end of this series, there is no shattering of any superpages in
the normal .text/data/bss region, but there is guarding of the pcpu
stack which is liable to shatter specific superpages mapping the
directmap region.

~Andrew

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

[Jan Beulich]

>>> On 22.02.16 at 11:29, <andrew.cooper3@citrix.com> wrote:
> On 22/02/16 10:02, Jan Beulich wrote:
>>>>> On 19.02.16 at 17:18, <andrew.cooper3@citrix.com> wrote:
>>> On 19/02/16 14:44, Jan Beulich wrote:
>>>>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>>>>> It is not obvious what this code is doing.  Most of it dates from 2007/2008,
>>>>> and there have been substantial changes in Xen's memory handling since then.
>>>> Deleting code which isn't understood what it is or was once used
>>>> for is sub-optimal.
>>>>
>>>>> It was previously optional, and isn't needed for any of the memguard
>>>>> infrastructure to function.  The use of MAP_SMALL_PAGES causes needless
>>>>> shattering of superpages.
>>>> Perhaps that's what is its purpose? Let's ask Keir, whom you didn't
>>>> even Cc.
>>> I don't see this patch being different in nature to your "x86: drop
>>> failsafe callback invocation from assembly".
>> That other patch explains why the code is (and never was)
>> necessary, whereas you just guess.
>>
>>> As I explain in the second paragraph, these calls are strictly optional,
>>> as they are omitted for release builds.  They also have no impact on the
>>> rest of the memguard infrastructure to function, as
>>> __memguard_change_range() also uses map_pages_to_xen().
>>>
>>> So despite not being sure why it is like it is, I am stating with that
>>> it is not needed with Xen in its current form.
>> I actually think the reason is to avoid the memory allocation which
>> might result the first time a 2M page gets split up, as that memory
>> allocation might fail (which nowadays gets a proper -ENOMEM
>> communicated out of map_pages_to_xen(), but that hasn't been
>> the case in the early days).
> 
> And isn't the case anywhere in the memguard infrastructure.
> 
> At the end of this series, there is no shattering of any superpages in
> the normal .text/data/bss region, but there is guarding of the pcpu
> stack which is liable to shatter specific superpages mapping the
> directmap region.

In which case I guess I'm fine with the change, but I'd like you to
re-word the commit message accordingly (in particular make it less
vague).

Jan

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

[Andrew Cooper]

This balloons the size of Xen in memory from 3.4MB to 12MB, 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>
---
 xen/arch/x86/setup.c     | 44 +++++++++++++++++++++++++++++++++++++++++---
 xen/arch/x86/xen.lds.S   | 33 +++++++++++++++++++++++++++++++++
 xen/include/xen/kernel.h |  6 ++++++
 3 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index cddf954..c360fbc 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -921,13 +921,51 @@ 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));
             *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 ( /*
+                      * Should be:
+                      *
+                      * i >= l2_table_offset((unsigned long)&__2M_text_start) &&
+                      *
+                      * but the EFI build can't manage the relocation.  It
+                      * evaluates to 0, so just use the upper bound.
+                      */
+                     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_data_start) &&
+                           i <  l2_table_offset((unsigned long)&__2M_data_end)) ||
+                          (i >= l2_table_offset((unsigned long)&__2M_bss_start) &&
+                           i <  l2_table_offset((unsigned long)&__2M_bss_end)) )
+                {
+                    flags = PAGE_HYPERVISOR_RW | _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
+                {
+                    *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 9fde1db..6c23c02 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);
@@ -67,6 +74,10 @@ SECTIONS
        *(.rodata.*)
   } :text
 
+  . = ALIGN(MB(2));
+  __2M_rodata_end = .;
+
+  __2M_data_start = .;         /* Start of 2M superpages, mapped RW. */
   . = ALIGN(SMP_CACHE_BYTES);
   .data.read_mostly : {
        /* Exception table */
@@ -104,6 +115,10 @@ SECTIONS
   __lock_profile_end = .;
 #endif
 
+  . = ALIGN(MB(2));
+  __2M_data_end = .;
+
+  __2M_init_start = .;         /* Start of 2M superpages, mapped RWX (boot only). */
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
   .init.text : {
@@ -166,6 +181,10 @@ SECTIONS
   . = ALIGN(STACK_SIZE);
   __init_end = .;
 
+  . = ALIGN(MB(2));
+  __2M_init_end = .;
+
+  __2M_bss_start = .;          /* Start of 2M superpages, mapped RW. */
   .bss : {                     /* BSS */
        __bss_start = .;
        *(.bss.stack_aligned)
@@ -183,6 +202,9 @@ SECTIONS
   } :text
   _end = . ;
 
+  . = ALIGN(MB(2));
+  __2M_bss_end = .;
+
 #ifdef EFI
   . = ALIGN(4);
   .reloc : {
@@ -229,4 +251,15 @@ 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_data_start,   MB(2)), "__2M_data_start misaligned")
+ASSERT(IS_ALIGNED(__2M_data_end,     MB(2)), "__2M_data_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_bss_start,    MB(2)), "__2M_bss_start misaligned")
+ASSERT(IS_ALIGNED(__2M_bss_end,      MB(2)), "__2M_bss_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..8d08eca 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -65,6 +65,12 @@
 	1;                                      \
 })
 
+extern unsigned long __2M_text_start[], __2M_text_end[];
+extern unsigned long __2M_rodata_start[], __2M_rodata_end[];
+extern unsigned long __2M_data_start[], __2M_data_end[];
+extern unsigned long __2M_init_start[], __2M_init_end[];
+extern unsigned long __2M_bss_start[], __2M_bss_end[];
+
 extern char _start[], _end[], start[];
 #define is_kernel(p) ({                         \
     char *__p = (char *)(unsigned long)(p);     \
-- 
2.1.4

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

[Jan Beulich]

>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -921,13 +921,51 @@ 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));
>              *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 ( /*
> +                      * Should be:
> +                      *
> +                      * i >= l2_table_offset((unsigned long)&__2M_text_start) &&
> +                      *
> +                      * but the EFI build can't manage the relocation.  It
> +                      * evaluates to 0, so just use the upper bound.
> +                      */
> +                     i < l2_table_offset((unsigned long)&__2M_text_end) )

I'll need some more detail about this, not the least because
excusing what looks like a hack with EFI, under which we won't
ever get here, is suspicious.

> +                {
> +                    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_data_start) &&
> +                           i <  l2_table_offset((unsigned long)&__2M_data_end)) ||
> +                          (i >= l2_table_offset((unsigned long)&__2M_bss_start) &&
> +                           i <  l2_table_offset((unsigned long)&__2M_bss_end)) )

This is odd - why can't .data and .bss share a (multiple of) 2M
region, at once presumably getting the whole image down to 10M
again?

> --- 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. */

Is the reason for aforementioned build problem perhaps the fact
that this label (and the others too) lives outside of any section?

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -65,6 +65,12 @@
>  	1;                                      \
>  })
>  
> +extern unsigned long __2M_text_start[], __2M_text_end[];
> +extern unsigned long __2M_rodata_start[], __2M_rodata_end[];
> +extern unsigned long __2M_data_start[], __2M_data_end[];
> +extern unsigned long __2M_init_start[], __2M_init_end[];
> +extern unsigned long __2M_bss_start[], __2M_bss_end[];

I'd really like to see at least the ones which are reference to r/o
sections marked const.

Jan

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

[Andrew Cooper]

On 19/02/16 14:58, Jan Beulich wrote:
>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -921,13 +921,51 @@ 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));
>>              *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 ( /*
>> +                      * Should be:
>> +                      *
>> +                      * i >= l2_table_offset((unsigned long)&__2M_text_start) &&
>> +                      *
>> +                      * but the EFI build can't manage the relocation.  It
>> +                      * evaluates to 0, so just use the upper bound.
>> +                      */
>> +                     i < l2_table_offset((unsigned long)&__2M_text_end) )
> I'll need some more detail about this, not the least because
> excusing what looks like a hack with EFI, under which we won't
> ever get here, is suspicious.

Specifically, the EFI uses i386pep and it objects to a 64bit relocation
of 0xfffffffffffffffc.

I can't explain why this symbol ends up with that relocation.

>
>> +                {
>> +                    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_data_start) &&
>> +                           i <  l2_table_offset((unsigned long)&__2M_data_end)) ||
>> +                          (i >= l2_table_offset((unsigned long)&__2M_bss_start) &&
>> +                           i <  l2_table_offset((unsigned long)&__2M_bss_end)) )
> This is odd - why can't .data and .bss share a (multiple of) 2M
> region, at once presumably getting the whole image down to 10M
> again?

.init is between .data and .bss, to allow .bss to be the final section
and not included in the result of mkelf32

>
>> --- 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. */
> Is the reason for aforementioned build problem perhaps the fact
> that this label (and the others too) lives outside of any section?

I am not sure.  It is only this symbol which is a problem.  All others
are fine.

I actually intended this to be an RFC patch, to see if anyone had
suggestions.

>
>> --- a/xen/include/xen/kernel.h
>> +++ b/xen/include/xen/kernel.h
>> @@ -65,6 +65,12 @@
>>  	1;                                      \
>>  })
>>  
>> +extern unsigned long __2M_text_start[], __2M_text_end[];
>> +extern unsigned long __2M_rodata_start[], __2M_rodata_end[];
>> +extern unsigned long __2M_data_start[], __2M_data_end[];
>> +extern unsigned long __2M_init_start[], __2M_init_end[];
>> +extern unsigned long __2M_bss_start[], __2M_bss_end[];
> I'd really like to see at least the ones which are reference to r/o
> sections marked const.

Ok.  I should probably also make them char rather than unsigned long, so
pointer arithmetic works sensibly for the length.

~Andrew

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

[Jan Beulich]

>>> On 19.02.16 at 16:51, <andrew.cooper3@citrix.com> wrote:
> On 19/02/16 14:58, Jan Beulich wrote:
>>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -921,13 +921,51 @@ 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));
>>>              *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 ( /*
>>> +                      * Should be:
>>> +                      *
>>> +                      * i >= l2_table_offset((unsigned long)&__2M_text_start) &&
>>> +                      *
>>> +                      * but the EFI build can't manage the relocation.  It
>>> +                      * evaluates to 0, so just use the upper bound.
>>> +                      */
>>> +                     i < l2_table_offset((unsigned long)&__2M_text_end) )
>> I'll need some more detail about this, not the least because
>> excusing what looks like a hack with EFI, under which we won't
>> ever get here, is suspicious.
> 
> Specifically, the EFI uses i386pep and it objects to a 64bit relocation
> of 0xfffffffffffffffc.
> 
> I can't explain why this symbol ends up with that relocation.

Interesting.

>>> +                {
>>> +                    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_data_start) &&
>>> +                           i <  l2_table_offset((unsigned long)&__2M_data_end)) ||
>>> +                          (i >= l2_table_offset((unsigned long)&__2M_bss_start) &&
>>> +                           i <  l2_table_offset((unsigned long)&__2M_bss_end)) )
>> This is odd - why can't .data and .bss share a (multiple of) 2M
>> region, at once presumably getting the whole image down to 10M
>> again?
> 
> .init is between .data and .bss, to allow .bss to be the final section
> and not included in the result of mkelf32

But I don't think it needs to remain there? Should be possible to be
put between .text and .rodata, or between .rodata and .data ...

>>> --- 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. */
>> Is the reason for aforementioned build problem perhaps the fact
>> that this label (and the others too) lives outside of any section?
> 
> I am not sure.  It is only this symbol which is a problem.  All others
> are fine.
> 
> I actually intended this to be an RFC patch, to see if anyone had
> suggestions.

Since you now imply this to be at the image base, I don't see
why using e.g. __XEN_VIRT_START (in its place, or via #define)
wouldn't be as good an option.

>>> --- a/xen/include/xen/kernel.h
>>> +++ b/xen/include/xen/kernel.h
>>> @@ -65,6 +65,12 @@
>>>  	1;                                      \
>>>  })
>>>  
>>> +extern unsigned long __2M_text_start[], __2M_text_end[];
>>> +extern unsigned long __2M_rodata_start[], __2M_rodata_end[];
>>> +extern unsigned long __2M_data_start[], __2M_data_end[];
>>> +extern unsigned long __2M_init_start[], __2M_init_end[];
>>> +extern unsigned long __2M_bss_start[], __2M_bss_end[];
>> I'd really like to see at least the ones which are reference to r/o
>> sections marked const.
> 
> Ok.  I should probably also make them char rather than unsigned long, so
> pointer arithmetic works sensibly for the length.

Good idea indeed.

Jan

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

[Andrew Cooper]

On 22/02/16 09:55, Jan Beulich wrote:
>
>>>> +                {
>>>> +                    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_data_start) &&
>>>> +                           i <  l2_table_offset((unsigned long)&__2M_data_end)) ||
>>>> +                          (i >= l2_table_offset((unsigned long)&__2M_bss_start) &&
>>>> +                           i <  l2_table_offset((unsigned long)&__2M_bss_end)) )
>>> This is odd - why can't .data and .bss share a (multiple of) 2M
>>> region, at once presumably getting the whole image down to 10M
>>> again?
>> .init is between .data and .bss, to allow .bss to be the final section
>> and not included in the result of mkelf32
> But I don't think it needs to remain there? Should be possible to be
> put between .text and .rodata, or between .rodata and .data ...

Actually yes - shifting .init to between .rodata and .data should work fine.

>
>>>> --- 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. */
>>> Is the reason for aforementioned build problem perhaps the fact
>>> that this label (and the others too) lives outside of any section?
>> I am not sure.  It is only this symbol which is a problem.  All others
>> are fine.
>>
>> I actually intended this to be an RFC patch, to see if anyone had
>> suggestions.
> Since you now imply this to be at the image base, I don't see
> why using e.g. __XEN_VIRT_START (in its place, or via #define)
> wouldn't be as good an option.

I don't understand what you mean here.

If you are suggesting #define __2M_text_start __XEN_VIRT_START then I
don't see how that is going to help with the relocation.

~Andrew

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

[Jan Beulich]

>>> On 22.02.16 at 11:24, <andrew.cooper3@citrix.com> wrote:
> On 22/02/16 09:55, Jan Beulich wrote:
>>>>> --- 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. */
>>>> Is the reason for aforementioned build problem perhaps the fact
>>>> that this label (and the others too) lives outside of any section?
>>> I am not sure.  It is only this symbol which is a problem.  All others
>>> are fine.
>>>
>>> I actually intended this to be an RFC patch, to see if anyone had
>>> suggestions.
>> Since you now imply this to be at the image base, I don't see
>> why using e.g. __XEN_VIRT_START (in its place, or via #define)
>> wouldn't be as good an option.
> 
> I don't understand what you mean here.
> 
> If you are suggesting #define __2M_text_start __XEN_VIRT_START then I
> don't see how that is going to help with the relocation.

Why not? __XEN_VIRT_START is a constant, i.e. won't result in
any relocations to get emitted.

Jan

[PATCH] 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>
---
CC: 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 c360fbc..e6d1fe6 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 6c23c02..72578f0 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -263,3 +263,6 @@ ASSERT(IS_ALIGNED(__2M_bss_start,    MB(2)), "__2M_bss_start misaligned")
 ASSERT(IS_ALIGNED(__2M_bss_end,      MB(2)), "__2M_bss_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] xen/x86: Unilaterally remove .init mappings

[Jan Beulich]

>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
> 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>

Re: [PATCH] xen/x86: Map Xen code/data/bss with superpages

[Andrew Cooper]

On 18/02/16 18:03, Andrew Cooper wrote:
> And make use of NX and RO attributes wherever possible.
>
> Andrew Cooper (4):
>   xen: Introduce IS_ALIGNED()
>   xen/memguard: Drop memguard_init() entirely
>   xen/x86: Use 2M superpages for text/data/bss mappings
>   xen/x86: Unilaterally remove .init mappings

Apologies - I messed up with git format-patch and omitted the patch
numbers.  (at least there are only 4)

The patches are available on
http://xenbits.xen.org/git-http/people/andrewcoop/xen.git xen-super-v1

~Andrew