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

[PATCH v3 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.

Tested and verified the Xen-e820 has been used in the test. Suspend and
resume have been verified to work, too.

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/mem.S        | 27 +++++++++++++++++++-----
 xen/arch/x86/boot/trampoline.S | 32 ++++++++++++++++++++++++++--
 xen/arch/x86/boot/wakeup.S     |  5 ++---
 xen/arch/x86/e820.c            | 24 ++++++++++-----------
 xen/arch/x86/efi/efi-boot.h    | 10 ++++-----
 xen/arch/x86/setup.c           | 47 ++++++++++++++++++++++--------------------
 xen/arch/x86/xen.lds.S         |  2 ++
 xen/include/asm-x86/config.h   |  1 +
 xen/include/asm-x86/e820.h     | 11 +++++-----
 9 files changed, 103 insertions(+), 56 deletions(-)

-- 
2.10.2


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

[PATCH v3 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 code in order for the permanent
trampoline to fit in a single page. 4k of stack seems excessive, about
3k should be more than enough.

Add an ASSERT() to the linker script to ensure the wakeup stack is
always at least 3k.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3: - drop trampoline_boot_start label, use wakeup_stack instead
      (Jan Beulich)
V2: - don't reserve space for the wakeup stack, just use the rest of
      the permanent trampoline page (Jan Beulich)
    - add an build time assertion for wakeup stack size (Jan Beulich)
    - some minor fixes (Jan Beulich)
---
 xen/arch/x86/boot/trampoline.S | 32 ++++++++++++++++++++++++++++++--
 xen/arch/x86/boot/wakeup.S     |  5 ++---
 xen/arch/x86/xen.lds.S         |  2 ++
 xen/include/asm-x86/config.h   |  1 +
 4 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 2715d17..a5d7b08 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,15 @@ start64:
         movabs  $__high_start,%rax
         jmpq    *%rax
 
+#include "wakeup.S"
+
+/* The first page of trampoline is permanent, the rest boot-time only. */
+/* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
+        .equ    wakeup_stack, trampoline_start + PAGE_SIZE
+        .global wakeup_stack
+
+/* From here on early boot only. */
+
         .code32
 trampoline_boot_cpu_entry:
         cmpb    $0,bootsym_rel(skip_realmode,5)
@@ -246,4 +275,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..b901cb1 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -173,6 +173,5 @@ bogus_saved_magic:
         movw    $0x0e00 + 'S', 0xb8014
         jmp     bogus_saved_magic
 
-        .align  16
-        .fill   PAGE_SIZE,1,0
-wakeup_stack:
+/* Stack for wakeup: rest of first trampoline page. */
+ENTRY(wakeup_stack_start)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 2d0ee8e..1462194 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -335,3 +335,5 @@ ASSERT(IS_ALIGNED(__bss_end,        8), "__bss_end misaligned")
 
 ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
     "not enough room for trampoline and mbi data")
+ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
+    "wakeup stack too small")
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index b9a6d94..d3ec2c3 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -75,6 +75,7 @@
 
 #define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
 #define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
+#define WAKEUP_STACK_MIN        3072
 
 #define MBI_SPACE_MIN           (2 * PAGE_SIZE)
 
-- 
2.10.2


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

[PATCH v3 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. Doing the copying in assembly avoids the need to export the
symbols for the BIOS E820 buffer and number of entries.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3: - rework of e820map_copy (Jan Beulich)
    - again ARRAY_SIZE() (Jan Beulich)
V2: - move copying the BIOS E820 table to the common one into assembly
      code avoiding the need to expose the BIOS table to C code
      (Jan Beulich)
    - use ARRAY_SIZE() instead of E820MAX where appropriate
      (Jan Beulich)
    - some style fixes (Jan Beulich)
---
 xen/arch/x86/boot/mem.S     | 21 ++++++++++++++++++--
 xen/arch/x86/e820.c         | 24 +++++++++++------------
 xen/arch/x86/efi/efi-boot.h | 10 +++++-----
 xen/arch/x86/setup.c        | 47 ++++++++++++++++++++++++---------------------
 xen/include/asm-x86/e820.h  |  9 ++++-----
 5 files changed, 64 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index 602ab2c..14dff95 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -67,10 +67,27 @@ get_memory_map:
 
         ret
 
+/*
+ * Copy E820 map obtained from BIOS to a buffer allocated by Xen.
+ * Input: %rdi: target address of e820 entry array
+ *        %esi: maximum number of entries to copy
+ * Output: %eax: number of entries copied
+ */
+        .code64
+ENTRY(e820map_copy)
+        mov     %esi, %eax
+        lea     e820map(%rip), %rsi
+        mov     e820nr(%rip), %ecx
+        cmp     %ecx, %eax
+        cmova   %ecx, %eax                      # number of entries to move
+        imul    $5, %eax, %ecx
+        rep movsl                               # do the move
+        ret
+
         .align  4
-GLOBAL(e820map)
+e820map:
         .fill   E820MAX*20,1,0
-GLOBAL(e820nr)
+e820nr:
         .long   0
 GLOBAL(lowmem_kb)
         .long   0
diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 76537ea..bc1544a 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.
@@ -75,7 +76,7 @@ static void __init add_memory_region(unsigned long long start,
 
     x = e820.nr_map;
 
-    if (x == E820MAX) {
+    if (x == ARRAY_SIZE(e820.map)) {
         printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
         return;
     }
@@ -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;
@@ -266,7 +268,7 @@ static int __init sanitize_e820_map(struct e820entry * biosmap, char * pnr_map)
                     change_point[chgidx]->addr - last_addr;
 				/* move forward only if the new size was non-zero */
                 if (new_bios[new_bios_entry].size != 0)
-                    if (++new_bios_entry >= E820MAX)
+                    if (++new_bios_entry >= ARRAY_SIZE(new_bios))
                         break; 	/* no more space left for new bios entries */
             }
             if (current_type != 0)	{
@@ -508,17 +510,14 @@ 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);
+    sanitize_e820_map(raw->map, &raw->nr_map);
+    copy_e820_map(raw->map, raw->nr_map);
 
     if ( opt_mem )
         clip_to_limit(opt_mem, NULL);
@@ -691,16 +690,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..34537d4 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 >= ARRAY_SIZE(e820_raw.map) )
             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..ed957e0 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -636,7 +636,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 {
     char *memmap_type = NULL;
     char *cmdline, *kextra, *loader;
-    unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
+    unsigned int nr_e820, initrdidx, domcr_flags = DOMCRF_s3_integrity;
     multiboot_info_t *mbi = __va(mbi_p);
     module_t *mod = (module_t *)__va(mbi->mods_addr);
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
@@ -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 ( (nr_e820 = copy_bios_e820(e820_raw.map,
+                                        ARRAY_SIZE(e820_raw.map))) != 0 )
     {
         memmap_type = "Xen-e820";
+        e820_raw.nr_map = nr_e820;
     }
     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 < ARRAY_SIZE(e820_raw.map) )
         {
             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..a2d468f 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -30,15 +30,14 @@ 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;
+unsigned int e820map_copy(struct e820entry *map, unsigned int limit);
 
-#define e820_raw bootsym(e820map)
-#define e820_raw_nr bootsym(e820nr)
+#define copy_bios_e820 bootsym(e820map_copy)
 
 #endif /*__E820_HEADER*/
-- 
2.10.2


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

[PATCH v3 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.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2: - define E820_BIOS_MAX in assembly code only (Jan Beulich)
---
 xen/arch/x86/boot/mem.S    | 6 +++---
 xen/include/asm-x86/e820.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index 14dff95..4b1fa4d 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -1,7 +1,7 @@
         .code16
 
 #define SMAP    0x534d4150
-#define E820MAX 128
+#define E820_BIOS_MAX 128
 
 get_memory_map:
 
@@ -23,7 +23,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)
@@ -86,7 +86,7 @@ ENTRY(e820map_copy)
 
         .align  4
 e820map:
-        .fill   E820MAX*20,1,0
+        .fill   E820_BIOS_MAX*20,1,0
 e820nr:
         .long   0
 GLOBAL(lowmem_kb)
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index a2d468f..28defa8 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -16,7 +16,7 @@ struct __packed e820entry {
     uint32_t type;
 };
 
-#define E820MAX	128
+#define E820MAX	1024
 
 struct e820map {
     unsigned int nr_map;
-- 
2.10.2


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

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

[Jan Beulich]

>>> On 24.03.17 at 08:48, <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.
> 
> Reduce the stack for wakeup code in order for the permanent
> trampoline to fit in a single page. 4k of stack seems excessive, about
> 3k should be more than enough.
> 
> Add an ASSERT() to the linker script to ensure the wakeup stack is
> always at least 3k.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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



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

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

[Jan Beulich]

>>> On 24.03.17 at 08:48, <jgross@suse.com> wrote:
> 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. Doing the copying in assembly avoids the need to export the
> symbols for the BIOS E820 buffer and number of entries.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -636,7 +636,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  {
>      char *memmap_type = NULL;
>      char *cmdline, *kextra, *loader;
> -    unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
> +    unsigned int nr_e820, initrdidx, domcr_flags = DOMCRF_s3_integrity;

Is this variable really needed?

> @@ -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 ( (nr_e820 = copy_bios_e820(e820_raw.map,
> +                                        ARRAY_SIZE(e820_raw.map))) != 0 )
>      {
>          memmap_type = "Xen-e820";
> +        e820_raw.nr_map = nr_e820;
>      }

I.e. can't we store the result of the function call in e820_raw.nr_map
right away? (The change could of course be done while committing.)

Jan


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

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

[Juergen Gross]

On 24/03/17 12:24, Jan Beulich wrote:
>>>> On 24.03.17 at 08:48, <jgross@suse.com> wrote:
>> 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. Doing the copying in assembly avoids the need to export the
>> symbols for the BIOS E820 buffer and number of entries.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one question:
> 
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -636,7 +636,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>  {
>>      char *memmap_type = NULL;
>>      char *cmdline, *kextra, *loader;
>> -    unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
>> +    unsigned int nr_e820, initrdidx, domcr_flags = DOMCRF_s3_integrity;
> 
> Is this variable really needed?
> 
>> @@ -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 ( (nr_e820 = copy_bios_e820(e820_raw.map,
>> +                                        ARRAY_SIZE(e820_raw.map))) != 0 )
>>      {
>>          memmap_type = "Xen-e820";
>> +        e820_raw.nr_map = nr_e820;
>>      }
> 
> I.e. can't we store the result of the function call in e820_raw.nr_map
> right away? (The change could of course be done while committing.)

You are right, the variable can be omitted.


Juergen


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