[PATCH 0/3] xen: support of large memory maps

[PATCH 0/3] xen: support of large memory maps

[Juergen Gross]

This patch series is the first part for adding support of large EFI
memory maps (> the current limit of 128 entries) while reducing
trampoline size.

I'm not posting the final patch for making the trampoline size
reduction effective in order not to add major rebase work to Daniel's
multiboot2 series.

Juergen Gross (3):
  xen/x86: split boot trampoline into permanent and temporary part
  xen/x86: use trampoline e820 buffer for BIOS interface only
  xen/x86: support larger memory map from EFI

 xen/arch/x86/boot/head.S       |  2 ++
 xen/arch/x86/boot/mem.S        |  7 ++++---
 xen/arch/x86/boot/trampoline.S | 29 +++++++++++++++++++++++++--
 xen/arch/x86/boot/wakeup.S     |  4 +++-
 xen/arch/x86/e820.c            | 22 ++++++++++-----------
 xen/arch/x86/efi/efi-boot.h    | 10 +++++-----
 xen/arch/x86/setup.c           | 45 ++++++++++++++++++++++--------------------
 xen/include/asm-x86/e820.h     | 14 +++++++++----
 8 files changed, 86 insertions(+), 47 deletions(-)

-- 
2.10.2


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

[PATCH 1/3] xen/x86: split boot trampoline into permanent and temporary part

[Juergen Gross]

The hypervisor needs a trampoline in low memory for early boot and
later for bringing up cpus and during wakeup from suspend. Today this
trampoline is kept completely even if most of it isn't needed later.

Split the trampoline into a permanent part and a temporary part needed
at early boot only. Introduce a new entry at the boundary.

Reduce the stack for wakeup coding in order for the permanent
trampoline to fit in a single page. 4k of stack seems excessive, about
3.5k should be more than enough.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/boot/head.S       |  2 ++
 xen/arch/x86/boot/trampoline.S | 29 +++++++++++++++++++++++++++--
 xen/arch/x86/boot/wakeup.S     |  4 +++-
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index a2177c3..39e760c 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -587,6 +587,8 @@ cmdline_parse_early:
 reloc:
 #include "reloc.S"
 
+        .align 16
+
 ENTRY(trampoline_start)
 #include "trampoline.S"
 ENTRY(trampoline_end)
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 2715d17..073fdee 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -1,4 +1,20 @@
-        .code16
+/*
+ * Trampoline code relocated to low memory.
+ *
+ * Care must taken when referencing symbols: they live in the relocated
+ * trampoline and in the hypervisor binary. The hypervisor symbols can either
+ * be accessed by their virtual address or by the physical address. When
+ * using the physical address eventually the physical start address of the
+ * hypervisor must be taken into account: after early boot the hypervisor
+ * will copy itself to high memory and writes its physical start address to
+ * trampoline_xen_phys_start in the low memory trampoline copy.
+ *
+ * Parts of the trampoline are needed for early boot only, while some other
+ * parts are needed as long as the hypervisor is active (e.g. wakeup code
+ * after suspend, bringup code for secondary cpus). The permanent parts should
+ * not reference any temporary low memory trampoline parts as those parts are
+ * not guaranteed to persist.
+ */
 
 /* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */
 #undef bootsym
@@ -18,6 +34,10 @@
         .long 111b - (off) - .;            \
         .popsection
 
+/* Start of the permanent trampoline code. */
+
+        .code16
+
 GLOBAL(trampoline_realmode_entry)
         mov     %cs,%ax
         mov     %ax,%ds
@@ -131,6 +151,12 @@ start64:
         movabs  $__high_start,%rax
         jmpq    *%rax
 
+#include "wakeup.S"
+
+ENTRY(trampoline_temp_start)
+
+/* From here on early boot only. */
+
         .code32
 trampoline_boot_cpu_entry:
         cmpb    $0,bootsym_rel(skip_realmode,5)
@@ -246,4 +272,3 @@ rm_idt: .word   256*4-1, 0, 0
 #include "mem.S"
 #include "edd.S"
 #include "video.S"
-#include "wakeup.S"
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index 08ea9b2..67c9e8f 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -173,6 +173,8 @@ bogus_saved_magic:
         movw    $0x0e00 + 'S', 0xb8014
         jmp     bogus_saved_magic
 
+/* Stack for wakeup: rest of first trampoline page. */
         .align  16
-        .fill   PAGE_SIZE,1,0
+.Lwakeup_stack_start:
+        .fill   PAGE_SIZE - (.Lwakeup_stack_start - trampoline_start),1,0
 wakeup_stack:
-- 
2.10.2


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

Re: [PATCH 1/3] xen/x86: split boot trampoline into permanent and temporary part

[Jan Beulich]

>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
> The hypervisor needs a trampoline in low memory for early boot and
> later for bringing up cpus and during wakeup from suspend. Today this
> trampoline is kept completely even if most of it isn't needed later.
> 
> Split the trampoline into a permanent part and a temporary part needed
> at early boot only. Introduce a new entry at the boundary.

s/entry/label/?

> Reduce the stack for wakeup coding in order for the permanent

"coding"?

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -587,6 +587,8 @@ cmdline_parse_early:
>  reloc:
>  #include "reloc.S"
>  
> +        .align 16

Why?

>  ENTRY(trampoline_start)

ENTRY() already does this, with proper NOP padding.

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -1,4 +1,20 @@
> -        .code16
> +/*
> + * Trampoline code relocated to low memory.
> + *
> + * Care must taken when referencing symbols: they live in the relocated
> + * trampoline and in the hypervisor binary. The hypervisor symbols can either
> + * be accessed by their virtual address or by the physical address. When
> + * using the physical address eventually the physical start address of the
> + * hypervisor must be taken into account: after early boot the hypervisor
> + * will copy itself to high memory and writes its physical start address to
> + * trampoline_xen_phys_start in the low memory trampoline copy.
> + *
> + * Parts of the trampoline are needed for early boot only, while some other
> + * parts are needed as long as the hypervisor is active (e.g. wakeup code
> + * after suspend, bringup code for secondary cpus). The permanent parts should
> + * not reference any temporary low memory trampoline parts as those parts are
> + * not guaranteed to persist.
> + */

It would of course be nice if we had a way to enforce this
restriction, but I can't seem to be able to think of a workable
one.

> @@ -131,6 +151,12 @@ start64:
>          movabs  $__high_start,%rax
>          jmpq    *%rax
>  
> +#include "wakeup.S"
> +
> +ENTRY(trampoline_temp_start)

s/temp/boot/ ?

> --- a/xen/arch/x86/boot/wakeup.S
> +++ b/xen/arch/x86/boot/wakeup.S
> @@ -173,6 +173,8 @@ bogus_saved_magic:
>          movw    $0x0e00 + 'S', 0xb8014
>          jmp     bogus_saved_magic
>  
> +/* Stack for wakeup: rest of first trampoline page. */
>          .align  16
> -        .fill   PAGE_SIZE,1,0
> +.Lwakeup_stack_start:
> +        .fill   PAGE_SIZE - (.Lwakeup_stack_start - trampoline_start),1,0
>  wakeup_stack:

Is this stack being used at boot time? If not, what's the point of
putting a gap in here? Instead it could simply be calculated by its
users as trampoline_start + PAGE_SIZE, overwriting whatever
was left there. All that would then be desirable would be a
BUILD_BUG_ON()-like construct to make sure the stack won't
grow too small.

Jan


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

Re: [PATCH 1/3] xen/x86: split boot trampoline into permanent and temporary part

[Juergen Gross]

On 22/03/17 12:33, Jan Beulich wrote:
>>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
>> The hypervisor needs a trampoline in low memory for early boot and
>> later for bringing up cpus and during wakeup from suspend. Today this
>> trampoline is kept completely even if most of it isn't needed later.
>>
>> Split the trampoline into a permanent part and a temporary part needed
>> at early boot only. Introduce a new entry at the boundary.
> 
> s/entry/label/?

Yes.

> 
>> Reduce the stack for wakeup coding in order for the permanent
> 
> "coding"?

code

> 
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -587,6 +587,8 @@ cmdline_parse_early:
>>  reloc:
>>  #include "reloc.S"
>>  
>> +        .align 16
> 
> Why?
> 
>>  ENTRY(trampoline_start)
> 
> ENTRY() already does this, with proper NOP padding.

Okay, will drop the .align

> 
>> --- a/xen/arch/x86/boot/trampoline.S
>> +++ b/xen/arch/x86/boot/trampoline.S
>> @@ -1,4 +1,20 @@
>> -        .code16
>> +/*
>> + * Trampoline code relocated to low memory.
>> + *
>> + * Care must taken when referencing symbols: they live in the relocated
>> + * trampoline and in the hypervisor binary. The hypervisor symbols can either
>> + * be accessed by their virtual address or by the physical address. When
>> + * using the physical address eventually the physical start address of the
>> + * hypervisor must be taken into account: after early boot the hypervisor
>> + * will copy itself to high memory and writes its physical start address to
>> + * trampoline_xen_phys_start in the low memory trampoline copy.
>> + *
>> + * Parts of the trampoline are needed for early boot only, while some other
>> + * parts are needed as long as the hypervisor is active (e.g. wakeup code
>> + * after suspend, bringup code for secondary cpus). The permanent parts should
>> + * not reference any temporary low memory trampoline parts as those parts are
>> + * not guaranteed to persist.
>> + */
> 
> It would of course be nice if we had a way to enforce this
> restriction, but I can't seem to be able to think of a workable
> one.
> 
>> @@ -131,6 +151,12 @@ start64:
>>          movabs  $__high_start,%rax
>>          jmpq    *%rax
>>  
>> +#include "wakeup.S"
>> +
>> +ENTRY(trampoline_temp_start)
> 
> s/temp/boot/ ?

I don't mind, so I'll rename it.

> 
>> --- a/xen/arch/x86/boot/wakeup.S
>> +++ b/xen/arch/x86/boot/wakeup.S
>> @@ -173,6 +173,8 @@ bogus_saved_magic:
>>          movw    $0x0e00 + 'S', 0xb8014
>>          jmp     bogus_saved_magic
>>  
>> +/* Stack for wakeup: rest of first trampoline page. */
>>          .align  16
>> -        .fill   PAGE_SIZE,1,0
>> +.Lwakeup_stack_start:
>> +        .fill   PAGE_SIZE - (.Lwakeup_stack_start - trampoline_start),1,0
>>  wakeup_stack:
> 
> Is this stack being used at boot time? If not, what's the point of
> putting a gap in here? Instead it could simply be calculated by its
> users as trampoline_start + PAGE_SIZE, overwriting whatever
> was left there. All that would then be desirable would be a
> BUILD_BUG_ON()-like construct to make sure the stack won't
> grow too small.

Okay, I'll do it this way.


Juergen

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

[PATCH 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

[Juergen Gross]

Instead of using the E820 raw buffer for BIOS, EFI and multiboot based
memory map information use it for the BIOS interface only. This will
enable us to support more E820 entries than the limited trampoline
located buffer can.

Add a new raw e820 table for common purpose and copy the BIOS buffer
to it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/e820.c         | 22 +++++++++++-----------
 xen/arch/x86/efi/efi-boot.h | 10 +++++-----
 xen/arch/x86/setup.c        | 45 ++++++++++++++++++++++++---------------------
 xen/include/asm-x86/e820.h  |  7 ++++---
 4 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 76537ea..4b47ec9 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -33,6 +33,7 @@ static bool_t __initdata e820_verbose;
 boolean_param("e820-verbose", e820_verbose);
 
 struct e820map e820;
+struct e820map __initdata e820_raw;
 
 /*
  * This function checks if the entire range <start,end> is mapped with type.
@@ -133,7 +134,8 @@ static struct change_member *change_point[2*E820MAX] __initdata;
 static struct e820entry *overlap_list[E820MAX] __initdata;
 static struct e820entry new_bios[E820MAX] __initdata;
 
-static int __init sanitize_e820_map(struct e820entry * biosmap, char * pnr_map)
+static int __init sanitize_e820_map(struct e820entry * biosmap,
+                                    unsigned int * pnr_map)
 {
     struct change_member *change_tmp;
     unsigned long current_type, last_type;
@@ -508,17 +510,16 @@ static void __init reserve_dmi_region(void)
     }
 }
 
-static void __init machine_specific_memory_setup(
-    struct e820entry *raw, unsigned int *raw_nr)
+static void __init machine_specific_memory_setup(struct e820map *raw)
 {
     unsigned long mpt_limit, ro_mpt_limit;
     uint64_t top_of_ram, size;
     int i;
 
-    char nr = (char)*raw_nr;
-    sanitize_e820_map(raw, &nr);
-    *raw_nr = nr;
-    (void)copy_e820_map(raw, nr);
+    unsigned int nr = raw->nr_map;
+    sanitize_e820_map(raw->map, &nr);
+    raw->nr_map = nr;
+    (void)copy_e820_map(raw->map, nr);
 
     if ( opt_mem )
         clip_to_limit(opt_mem, NULL);
@@ -691,16 +692,15 @@ int __init reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e)
     return e820_change_range_type(e820, s, e, E820_RAM, E820_RESERVED);
 }
 
-unsigned long __init init_e820(
-    const char *str, struct e820entry *raw, unsigned int *raw_nr)
+unsigned long __init init_e820(const char *str, struct e820map *raw)
 {
     if ( e820_verbose )
     {
         printk("Initial %s RAM map:\n", str);
-        print_e820_memory_map(raw, *raw_nr);
+        print_e820_memory_map(raw->map, raw->nr_map);
     }
 
-    machine_specific_memory_setup(raw, raw_nr);
+    machine_specific_memory_setup(raw);
 
     printk("%s RAM map:\n", str);
     print_e820_memory_map(e820.map, e820.nr_map);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 0e1c190..1785381 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -156,8 +156,8 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
     unsigned int i;
 
     /* Populate E820 table and check trampoline area availability. */
-    e = e820map - 1;
-    for ( e820nr = i = 0; i < map_size; i += desc_size )
+    e = e820_raw.map - 1;
+    for ( e820_raw.nr_map = i = 0; i < map_size; i += desc_size )
     {
         EFI_MEMORY_DESCRIPTOR *desc = map + i;
         u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
@@ -194,10 +194,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
             type = E820_NVS;
             break;
         }
-        if ( e820nr && type == e->type &&
+        if ( e820_raw.nr_map && type == e->type &&
              desc->PhysicalStart == e->addr + e->size )
             e->size += len;
-        else if ( !len || e820nr >= E820MAX )
+        else if ( !len || e820_raw.nr_map >= E820MAX )
             continue;
         else
         {
@@ -205,7 +205,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
             e->addr = desc->PhysicalStart;
             e->size = len;
             e->type = type;
-            ++e820nr;
+            ++e820_raw.nr_map;
         }
     }
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1cd290e..d7ee678 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -782,14 +782,17 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     }
     else if ( efi_enabled(EFI_BOOT) )
         memmap_type = "EFI";
-    else if ( e820_raw_nr != 0 )
+    else if ( e820_bios_nr != 0 )
     {
         memmap_type = "Xen-e820";
+        for ( i = 0; i < e820_bios_nr && i < E820MAX; i++ )
+            e820_raw.map[i] = e820_bios[i];
+        e820_raw.nr_map = i;
     }
     else if ( mbi->flags & MBI_MEMMAP )
     {
         memmap_type = "Multiboot-e820";
-        while ( (bytes < mbi->mmap_length) && (e820_raw_nr < E820MAX) )
+        while ( (bytes < mbi->mmap_length) && (e820_raw.nr_map < E820MAX) )
         {
             memory_map_t *map = __va(mbi->mmap_addr + bytes);
 
@@ -813,12 +816,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                 map->length_high = 0;
             }
 
-            e820_raw[e820_raw_nr].addr = 
+            e820_raw.map[e820_raw.nr_map].addr =
                 ((u64)map->base_addr_high << 32) | (u64)map->base_addr_low;
-            e820_raw[e820_raw_nr].size = 
+            e820_raw.map[e820_raw.nr_map].size =
                 ((u64)map->length_high << 32) | (u64)map->length_low;
-            e820_raw[e820_raw_nr].type = map->type;
-            e820_raw_nr++;
+            e820_raw.map[e820_raw.nr_map].type = map->type;
+            e820_raw.nr_map++;
 
             bytes += map->size + 4;
         }
@@ -826,30 +829,30 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     else if ( bootsym(lowmem_kb) )
     {
         memmap_type = "Xen-e801";
-        e820_raw[0].addr = 0;
-        e820_raw[0].size = bootsym(lowmem_kb) << 10;
-        e820_raw[0].type = E820_RAM;
-        e820_raw[1].addr = 0x100000;
-        e820_raw[1].size = bootsym(highmem_kb) << 10;
-        e820_raw[1].type = E820_RAM;
-        e820_raw_nr = 2;
+        e820_raw.map[0].addr = 0;
+        e820_raw.map[0].size = bootsym(lowmem_kb) << 10;
+        e820_raw.map[0].type = E820_RAM;
+        e820_raw.map[1].addr = 0x100000;
+        e820_raw.map[1].size = bootsym(highmem_kb) << 10;
+        e820_raw.map[1].type = E820_RAM;
+        e820_raw.nr_map = 2;
     }
     else if ( mbi->flags & MBI_MEMLIMITS )
     {
         memmap_type = "Multiboot-e801";
-        e820_raw[0].addr = 0;
-        e820_raw[0].size = mbi->mem_lower << 10;
-        e820_raw[0].type = E820_RAM;
-        e820_raw[1].addr = 0x100000;
-        e820_raw[1].size = mbi->mem_upper << 10;
-        e820_raw[1].type = E820_RAM;
-        e820_raw_nr = 2;
+        e820_raw.map[0].addr = 0;
+        e820_raw.map[0].size = mbi->mem_lower << 10;
+        e820_raw.map[0].type = E820_RAM;
+        e820_raw.map[1].addr = 0x100000;
+        e820_raw.map[1].size = mbi->mem_upper << 10;
+        e820_raw.map[1].type = E820_RAM;
+        e820_raw.nr_map = 2;
     }
     else
         panic("Bootloader provided no memory information.");
 
     /* Sanitise the raw E820 map to produce a final clean version. */
-    max_page = raw_max_page = init_e820(memmap_type, e820_raw, &e820_raw_nr);
+    max_page = raw_max_page = init_e820(memmap_type, &e820_raw);
 
     /* Create a temporary copy of the E820 map. */
     memcpy(&boot_e820, &e820, sizeof(e820));
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index d9ff4eb..4a40d14 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -30,15 +30,16 @@ extern int e820_change_range_type(
     uint32_t orig_type, uint32_t new_type);
 extern int e820_add_range(
     struct e820map *, uint64_t s, uint64_t e, uint32_t type);
-extern unsigned long init_e820(const char *, struct e820entry *, unsigned int *);
+extern unsigned long init_e820(const char *, struct e820map *);
 extern struct e820map e820;
+extern struct e820map e820_raw;
 
 /* These symbols live in the boot trampoline. */
 extern struct e820entry e820map[];
 extern unsigned int e820nr;
 extern unsigned int lowmem_kb, highmem_kb;
 
-#define e820_raw bootsym(e820map)
-#define e820_raw_nr bootsym(e820nr)
+#define e820_bios bootsym(e820map)
+#define e820_bios_nr bootsym(e820nr)
 
 #endif /*__E820_HEADER*/
-- 
2.10.2


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

Re: [PATCH 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

[Jan Beulich]

>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
> @@ -133,7 +134,8 @@ static struct change_member *change_point[2*E820MAX] __initdata;
>  static struct e820entry *overlap_list[E820MAX] __initdata;
>  static struct e820entry new_bios[E820MAX] __initdata;
>  
> -static int __init sanitize_e820_map(struct e820entry * biosmap, char * pnr_map)
> +static int __init sanitize_e820_map(struct e820entry * biosmap,
> +                                    unsigned int * pnr_map)

Please correct style violations in code you touch anyway (here:
stray blanks after the *s).

> @@ -508,17 +510,16 @@ static void __init reserve_dmi_region(void)
>      }
>  }
>  
> -static void __init machine_specific_memory_setup(
> -    struct e820entry *raw, unsigned int *raw_nr)
> +static void __init machine_specific_memory_setup(struct e820map *raw)
>  {
>      unsigned long mpt_limit, ro_mpt_limit;
>      uint64_t top_of_ram, size;
>      int i;
>  
> -    char nr = (char)*raw_nr;
> -    sanitize_e820_map(raw, &nr);
> -    *raw_nr = nr;
> -    (void)copy_e820_map(raw, nr);
> +    unsigned int nr = raw->nr_map;
> +    sanitize_e820_map(raw->map, &nr);

And here: Missing blank line between declarations and
statements (in fact the blank line is there but misplaced). Or wait:
The variable "nr" doesn't appear to be needed at all:

    sanitize_e820_map(raw->map, &raw->nr_map);
    copy_e820_map(raw->map, raw->nr_map);

I guess it was there merely because field type and parameter
type didn't match up.

> +    raw->nr_map = nr;
> +    (void)copy_e820_map(raw->map, nr);

Stray cast.

> @@ -194,10 +194,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>              type = E820_NVS;
>              break;
>          }
> -        if ( e820nr && type == e->type &&
> +        if ( e820_raw.nr_map && type == e->type &&
>               desc->PhysicalStart == e->addr + e->size )
>              e->size += len;
> -        else if ( !len || e820nr >= E820MAX )
> +        else if ( !len || e820_raw.nr_map >= E820MAX )

ARRAY_SIZE() (also elsewhere)?

> --- a/xen/include/asm-x86/e820.h
> +++ b/xen/include/asm-x86/e820.h
> @@ -30,15 +30,16 @@ extern int e820_change_range_type(
>      uint32_t orig_type, uint32_t new_type);
>  extern int e820_add_range(
>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
> -extern unsigned long init_e820(const char *, struct e820entry *, unsigned int *);
> +extern unsigned long init_e820(const char *, struct e820map *);
>  extern struct e820map e820;
> +extern struct e820map e820_raw;
>  
>  /* These symbols live in the boot trampoline. */
>  extern struct e820entry e820map[];
>  extern unsigned int e820nr;

It would be nice to restrict the visibility of these two (to be certain
there are no stray references), but that may be difficult to achieve.
One option might be to have an accessor function at the bottom of
e820.c, placing their declarations right ahead of that function. That
way only that function can validly use them. Another option would
be to drop the declarations altogether, moving the copying to
e820_raw into assembly code.

Jan


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

Re: [PATCH 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

[Juergen Gross]

On 22/03/17 14:12, Jan Beulich wrote:
>>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
>> @@ -133,7 +134,8 @@ static struct change_member *change_point[2*E820MAX] __initdata;
>>  static struct e820entry *overlap_list[E820MAX] __initdata;
>>  static struct e820entry new_bios[E820MAX] __initdata;
>>  
>> -static int __init sanitize_e820_map(struct e820entry * biosmap, char * pnr_map)
>> +static int __init sanitize_e820_map(struct e820entry * biosmap,
>> +                                    unsigned int * pnr_map)
> 
> Please correct style violations in code you touch anyway (here:
> stray blanks after the *s).

Okay.

> 
>> @@ -508,17 +510,16 @@ static void __init reserve_dmi_region(void)
>>      }
>>  }
>>  
>> -static void __init machine_specific_memory_setup(
>> -    struct e820entry *raw, unsigned int *raw_nr)
>> +static void __init machine_specific_memory_setup(struct e820map *raw)
>>  {
>>      unsigned long mpt_limit, ro_mpt_limit;
>>      uint64_t top_of_ram, size;
>>      int i;
>>  
>> -    char nr = (char)*raw_nr;
>> -    sanitize_e820_map(raw, &nr);
>> -    *raw_nr = nr;
>> -    (void)copy_e820_map(raw, nr);
>> +    unsigned int nr = raw->nr_map;
>> +    sanitize_e820_map(raw->map, &nr);
> 
> And here: Missing blank line between declarations and
> statements (in fact the blank line is there but misplaced). Or wait:
> The variable "nr" doesn't appear to be needed at all:
> 
>     sanitize_e820_map(raw->map, &raw->nr_map);
>     copy_e820_map(raw->map, raw->nr_map);
> 
> I guess it was there merely because field type and parameter
> type didn't match up.

You are right. Will remove it.

> 
>> +    raw->nr_map = nr;
>> +    (void)copy_e820_map(raw->map, nr);
> 
> Stray cast.

Indeed.

> 
>> @@ -194,10 +194,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>>              type = E820_NVS;
>>              break;
>>          }
>> -        if ( e820nr && type == e->type &&
>> +        if ( e820_raw.nr_map && type == e->type &&
>>               desc->PhysicalStart == e->addr + e->size )
>>              e->size += len;
>> -        else if ( !len || e820nr >= E820MAX )
>> +        else if ( !len || e820_raw.nr_map >= E820MAX )
> 
> ARRAY_SIZE() (also elsewhere)?

Yes. Would you prefer a separate patch for the other places I don't
touch yet, or should I fold it into this one?

>> --- a/xen/include/asm-x86/e820.h
>> +++ b/xen/include/asm-x86/e820.h
>> @@ -30,15 +30,16 @@ extern int e820_change_range_type(
>>      uint32_t orig_type, uint32_t new_type);
>>  extern int e820_add_range(
>>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
>> -extern unsigned long init_e820(const char *, struct e820entry *, unsigned int *);
>> +extern unsigned long init_e820(const char *, struct e820map *);
>>  extern struct e820map e820;
>> +extern struct e820map e820_raw;
>>  
>>  /* These symbols live in the boot trampoline. */
>>  extern struct e820entry e820map[];
>>  extern unsigned int e820nr;
> 
> It would be nice to restrict the visibility of these two (to be certain
> there are no stray references), but that may be difficult to achieve.
> One option might be to have an accessor function at the bottom of
> e820.c, placing their declarations right ahead of that function. That
> way only that function can validly use them. Another option would
> be to drop the declarations altogether, moving the copying to
> e820_raw into assembly code.

Hmm, maybe a copy function in assembly code? Something like:

e820_raw.nr_map = copy_bios_e820(e820_raw.map);

This would hide struct e820 from assembly and e820map[] and e820nr from
C code.


Juergen

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

Re: [PATCH 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

[Jan Beulich]

>>> On 22.03.17 at 16:01, <jgross@suse.com> wrote:
> On 22/03/17 14:12, Jan Beulich wrote:
>>>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
>>> @@ -194,10 +194,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>>>              type = E820_NVS;
>>>              break;
>>>          }
>>> -        if ( e820nr && type == e->type &&
>>> +        if ( e820_raw.nr_map && type == e->type &&
>>>               desc->PhysicalStart == e->addr + e->size )
>>>              e->size += len;
>>> -        else if ( !len || e820nr >= E820MAX )
>>> +        else if ( !len || e820_raw.nr_map >= E820MAX )
>> 
>> ARRAY_SIZE() (also elsewhere)?
> 
> Yes. Would you prefer a separate patch for the other places I don't
> touch yet, or should I fold it into this one?

Either way should be fine.

>>> --- a/xen/include/asm-x86/e820.h
>>> +++ b/xen/include/asm-x86/e820.h
>>> @@ -30,15 +30,16 @@ extern int e820_change_range_type(
>>>      uint32_t orig_type, uint32_t new_type);
>>>  extern int e820_add_range(
>>>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
>>> -extern unsigned long init_e820(const char *, struct e820entry *, unsigned int *);
>>> +extern unsigned long init_e820(const char *, struct e820map *);
>>>  extern struct e820map e820;
>>> +extern struct e820map e820_raw;
>>>  
>>>  /* These symbols live in the boot trampoline. */
>>>  extern struct e820entry e820map[];
>>>  extern unsigned int e820nr;
>> 
>> It would be nice to restrict the visibility of these two (to be certain
>> there are no stray references), but that may be difficult to achieve.
>> One option might be to have an accessor function at the bottom of
>> e820.c, placing their declarations right ahead of that function. That
>> way only that function can validly use them. Another option would
>> be to drop the declarations altogether, moving the copying to
>> e820_raw into assembly code.
> 
> Hmm, maybe a copy function in assembly code? Something like:
> 
> e820_raw.nr_map = copy_bios_e820(e820_raw.map);
> 
> This would hide struct e820 from assembly and e820map[] and e820nr from
> C code.

Good idea, go ahead, albeit the hiding from assembly part is not
entirely true: Assembly code, with the single argument passed
above, would make the implication that the passed in array is no
smaller than the BIOS one.

Jan


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

Re: [PATCH 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

[Juergen Gross]

On 22/03/17 16:23, Jan Beulich wrote:
>>>> On 22.03.17 at 16:01, <jgross@suse.com> wrote:
>> On 22/03/17 14:12, Jan Beulich wrote:
>>>>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
>>>> @@ -194,10 +194,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>>>>              type = E820_NVS;
>>>>              break;
>>>>          }
>>>> -        if ( e820nr && type == e->type &&
>>>> +        if ( e820_raw.nr_map && type == e->type &&
>>>>               desc->PhysicalStart == e->addr + e->size )
>>>>              e->size += len;
>>>> -        else if ( !len || e820nr >= E820MAX )
>>>> +        else if ( !len || e820_raw.nr_map >= E820MAX )
>>>
>>> ARRAY_SIZE() (also elsewhere)?
>>
>> Yes. Would you prefer a separate patch for the other places I don't
>> touch yet, or should I fold it into this one?
> 
> Either way should be fine.
> 
>>>> --- a/xen/include/asm-x86/e820.h
>>>> +++ b/xen/include/asm-x86/e820.h
>>>> @@ -30,15 +30,16 @@ extern int e820_change_range_type(
>>>>      uint32_t orig_type, uint32_t new_type);
>>>>  extern int e820_add_range(
>>>>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
>>>> -extern unsigned long init_e820(const char *, struct e820entry *, unsigned int *);
>>>> +extern unsigned long init_e820(const char *, struct e820map *);
>>>>  extern struct e820map e820;
>>>> +extern struct e820map e820_raw;
>>>>  
>>>>  /* These symbols live in the boot trampoline. */
>>>>  extern struct e820entry e820map[];
>>>>  extern unsigned int e820nr;
>>>
>>> It would be nice to restrict the visibility of these two (to be certain
>>> there are no stray references), but that may be difficult to achieve.
>>> One option might be to have an accessor function at the bottom of
>>> e820.c, placing their declarations right ahead of that function. That
>>> way only that function can validly use them. Another option would
>>> be to drop the declarations altogether, moving the copying to
>>> e820_raw into assembly code.
>>
>> Hmm, maybe a copy function in assembly code? Something like:
>>
>> e820_raw.nr_map = copy_bios_e820(e820_raw.map);
>>
>> This would hide struct e820 from assembly and e820map[] and e820nr from
>> C code.
> 
> Good idea, go ahead, albeit the hiding from assembly part is not
> entirely true: Assembly code, with the single argument passed
> above, would make the implication that the passed in array is no
> smaller than the BIOS one.

Hmm, passing the maximum number of entries as a second parameter isn't
really rocket science. :-)


Juergen


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

[PATCH 3/3] xen/x86: support larger memory map from EFI

[Juergen Gross]

Use a larger e820 map buffer for non-BIOS memory map sources. This
requires to have different defines for the maximum number of E820 map
entries for the raw BIOS buffer and the later used struct e820map.

While at it use e820.h in mem.S in order to avoid having to define the
buffer size at two places.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/boot/mem.S    | 7 ++++---
 xen/arch/x86/setup.c       | 2 +-
 xen/include/asm-x86/e820.h | 7 ++++++-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index 602ab2c..17db546 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -1,7 +1,8 @@
+#include <asm/e820.h>
+
         .code16
 
 #define SMAP    0x534d4150
-#define E820MAX 128
 
 get_memory_map:
 
@@ -23,7 +24,7 @@ get_memory_map:
         jne     .Lmem88
 
         movb    bootsym(e820nr),%al             # up to 128 entries
-        cmpb    $E820MAX,%al
+        cmpb    $E820_BIOS_MAX,%al
         jae     .Lmem88
 
         incb    bootsym(e820nr)
@@ -69,7 +70,7 @@ get_memory_map:
 
         .align  4
 GLOBAL(e820map)
-        .fill   E820MAX*20,1,0
+        .fill   E820_BIOS_MAX*20,1,0
 GLOBAL(e820nr)
         .long   0
 GLOBAL(lowmem_kb)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d7ee678..36c73f4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -785,7 +785,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     else if ( e820_bios_nr != 0 )
     {
         memmap_type = "Xen-e820";
-        for ( i = 0; i < e820_bios_nr && i < E820MAX; i++ )
+        for ( i = 0; i < e820_bios_nr && i < E820_BIOS_MAX; i++ )
             e820_raw.map[i] = e820_bios[i];
         e820_raw.nr_map = i;
     }
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index 4a40d14..448338c 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -10,13 +10,14 @@
 #define E820_NVS          4
 #define E820_UNUSABLE     5
 
+#ifndef __ASSEMBLY__
 struct __packed e820entry {
     uint64_t addr;
     uint64_t size;
     uint32_t type;
 };
 
-#define E820MAX	128
+#define E820MAX	1024
 
 struct e820map {
     unsigned int nr_map;
@@ -42,4 +43,8 @@ extern unsigned int lowmem_kb, highmem_kb;
 #define e820_bios bootsym(e820map)
 #define e820_bios_nr bootsym(e820nr)
 
+#endif /* __ASSEMBLY__ */
+
+#define E820_BIOS_MAX 128
+
 #endif /*__E820_HEADER*/
-- 
2.10.2


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

Re: [PATCH 3/3] xen/x86: support larger memory map from EFI

[Jan Beulich]

>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
> Use a larger e820 map buffer for non-BIOS memory map sources. This
> requires to have different defines for the maximum number of E820 map
> entries for the raw BIOS buffer and the later used struct e820map.
> 
> While at it use e820.h in mem.S in order to avoid having to define the
> buffer size at two places.

I don't think you need to define it outside the assembly file at all,
since the use in setup.c seems unnecessary (instead you rather
want to keep E820MAX [or its ARRAY_SIZE() equivalent] there as
the output array bound; the input value can't exceed
E280_BIOS_MAX anyway). One reason to actually have the #define
in the header would be if you used it in the e820map[] declaration,
but since that array is bounded by e820nr I don't think that's
strictly necessary.

Jan


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

Re: [PATCH 3/3] xen/x86: support larger memory map from EFI

[Juergen Gross]

On 22/03/17 14:19, Jan Beulich wrote:
>>>> On 21.03.17 at 14:10, <jgross@suse.com> wrote:
>> Use a larger e820 map buffer for non-BIOS memory map sources. This
>> requires to have different defines for the maximum number of E820 map
>> entries for the raw BIOS buffer and the later used struct e820map.
>>
>> While at it use e820.h in mem.S in order to avoid having to define the
>> buffer size at two places.
> 
> I don't think you need to define it outside the assembly file at all,
> since the use in setup.c seems unnecessary (instead you rather
> want to keep E820MAX [or its ARRAY_SIZE() equivalent] there as
> the output array bound; the input value can't exceed
> E280_BIOS_MAX anyway). One reason to actually have the #define
> in the header would be if you used it in the e820map[] declaration,
> but since that array is bounded by e820nr I don't think that's
> strictly necessary.

Even more obvious with e820map[] visibility restricted to assembly code.

So I will just replace E820MAX with E820_BIOS_MAX in assembly without
adding E820_BIOS_MAX to e820.h.


Juergen


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

Re: [PATCH 0/3] xen: support of large memory maps

[Daniel Kiper]

On Tue, Mar 21, 2017 at 02:10:20PM +0100, Juergen Gross wrote:
> This patch series is the first part for adding support of large EFI
> memory maps (> the current limit of 128 entries) while reducing
> trampoline size.
>
> I'm not posting the final patch for making the trampoline size
> reduction effective in order not to add major rebase work to Daniel's
> multiboot2 series.

Thanks a lot Juergen!

Daniel

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

Re: [PATCH 0/3] xen: support of large memory maps

[Alex Thorlton]

On Tue, Mar 21, 2017 at 02:10:20PM +0100, Juergen Gross wrote:
> This patch series is the first part for adding support of large EFI
> memory maps (> the current limit of 128 entries) while reducing
> trampoline size.
> 
> I'm not posting the final patch for making the trampoline size
> reduction effective in order not to add major rebase work to Daniel's
> multiboot2 series.
> 
> Juergen Gross (3):
>   xen/x86: split boot trampoline into permanent and temporary part
>   xen/x86: use trampoline e820 buffer for BIOS interface only
>   xen/x86: support larger memory map from EFI

I have tested these patches on a system with a large E820 table (~700
entries) and everything appears to work fine.

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

Re: [PATCH 0/3] xen: support of large memory maps

[Juergen Gross]

On 22/03/17 16:17, Alex Thorlton wrote:
> On Tue, Mar 21, 2017 at 02:10:20PM +0100, Juergen Gross wrote:
>> This patch series is the first part for adding support of large EFI
>> memory maps (> the current limit of 128 entries) while reducing
>> trampoline size.
>>
>> I'm not posting the final patch for making the trampoline size
>> reduction effective in order not to add major rebase work to Daniel's
>> multiboot2 series.
>>
>> Juergen Gross (3):
>>   xen/x86: split boot trampoline into permanent and temporary part
>>   xen/x86: use trampoline e820 buffer for BIOS interface only
>>   xen/x86: support larger memory map from EFI
> 
> I have tested these patches on a system with a large E820 table (~700
> entries) and everything appears to work fine.
> 

Thank you very much for the test and reporting the result!


Juergen

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