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

[PATCH v2 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/mem.S        | 32 ++++++++++++++++++++++++-----
 xen/arch/x86/boot/trampoline.S | 31 ++++++++++++++++++++++++++--
 xen/arch/x86/boot/wakeup.S     |  6 +++---
 xen/arch/x86/e820.c            | 24 ++++++++++------------
 xen/arch/x86/efi/efi-boot.h    | 10 ++++-----
 xen/arch/x86/setup.c           | 46 ++++++++++++++++++++++--------------------
 xen/arch/x86/xen.lds.S         |  2 ++
 xen/include/asm-x86/config.h   |  1 +
 xen/include/asm-x86/e820.h     | 11 +++++-----
 9 files changed, 107 insertions(+), 56 deletions(-)

-- 
2.10.2


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

[PATCH v2 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>
---
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 | 31 +++++++++++++++++++++++++++++--
 xen/arch/x86/boot/wakeup.S     |  6 +++---
 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..6585f12 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,14 @@ start64:
         movabs  $__high_start,%rax
         jmpq    *%rax
 
+#include "wakeup.S"
+
+/* The first page of trampoline is permanent, the rest boot-time only. */
+        .equ    trampoline_boot_start, trampoline_start + PAGE_SIZE
+        .global trampoline_boot_start
+
+/* From here on early boot only. */
+
         .code32
 trampoline_boot_cpu_entry:
         cmpb    $0,bootsym_rel(skip_realmode,5)
@@ -246,4 +274,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..fa120a9 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -1,6 +1,7 @@
         .code16
 
 #define wakesym(sym) (sym - wakeup_start)
+#define wakeup_stack trampoline_start + PAGE_SIZE
 
         .align 16
 ENTRY(wakeup_start)
@@ -173,6 +174,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..7b87bdb 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((trampoline_boot_start - 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 v2 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>
---
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     | 26 +++++++++++++++++++++++--
 xen/arch/x86/e820.c         | 24 +++++++++++------------
 xen/arch/x86/efi/efi-boot.h | 10 +++++-----
 xen/arch/x86/setup.c        | 46 +++++++++++++++++++++++----------------------
 xen/include/asm-x86/e820.h  |  9 ++++-----
 5 files changed, 68 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index 602ab2c..ed2a190 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -67,10 +67,32 @@ get_memory_map:
 
         ret
 
+/*
+ * Copy E820 map obtained from BIOS to a buffer allocated by Xen.
+ * Input: %rdi: target address of e820 entry array
+ *        %rsi: maximum number of entries to copy
+ * Output: %rax: number of entries copied
+ */
+        .code64
+ENTRY(e820map_copy)
+        mov     %rsi, %rax
+        movq    $bootsym(e820map), %rsi
+        movl    bootsym(e820nr), %ecx
+        cmpl    %ecx, %eax
+        cmova   %ecx, %eax                      # number of entries to move
+        movl    %eax, %ecx
+        shll    $2, %ecx
+        jz      .Lcopyexit                      # early exit: nothing to move
+        addl    %eax, %ecx                      # number of 4-byte moves
+        cld                                     # (5 times of entries)
+        rep movsd                               # do the move
+.Lcopyexit:
+        retq
+
         .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..0e35fb8 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,16 @@ 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, E820MAX)) != 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 +815,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 +828,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 v2 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>
---
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 ed2a190..518fbcd 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)
@@ -91,7 +91,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 v2 1/3] xen/x86: split boot trampoline into permanent and temporary part

[Jan Beulich]

>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
> @@ -131,6 +151,14 @@ start64:
>          movabs  $__high_start,%rax
>          jmpq    *%rax
>  
> +#include "wakeup.S"
> +
> +/* The first page of trampoline is permanent, the rest boot-time only. */
> +        .equ    trampoline_boot_start, trampoline_start + PAGE_SIZE
> +        .global trampoline_boot_start

The name is at least ambiguous - boot only code starts right here,
not at the next page boundary. Would it not work to use wakeup_stack
here, ...

> --- a/xen/arch/x86/boot/wakeup.S
> +++ b/xen/arch/x86/boot/wakeup.S
> @@ -1,6 +1,7 @@
>          .code16
>  
>  #define wakesym(sym) (sym - wakeup_start)
> +#define wakeup_stack trampoline_start + PAGE_SIZE

... omit this #define, and ...

> --- 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((trampoline_boot_start - wakeup_stack_start) >= WAKEUP_STACK_MIN,
> +    "wakeup stack too small")

... use wakeup_stack here too?

Jan


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

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

[Jan Beulich]

>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/boot/mem.S
> +++ b/xen/arch/x86/boot/mem.S
> @@ -67,10 +67,32 @@ get_memory_map:
>  
>          ret
>  
> +/*
> + * Copy E820 map obtained from BIOS to a buffer allocated by Xen.
> + * Input: %rdi: target address of e820 entry array
> + *        %rsi: maximum number of entries to copy

%esi (and also needs to be used that way below)

> + * Output: %rax: number of entries copied
> + */
> +        .code64
> +ENTRY(e820map_copy)
> +        mov     %rsi, %rax
> +        movq    $bootsym(e820map), %rsi

%rip-relative leaq? Even more - is bootsym() usable here at all? The
comment next to its definition says otherwise. Otoh I'm sure you've
tested this, so it must be working despite me not seeing how it would
do so.

> +        movl    bootsym(e820nr), %ecx
> +        cmpl    %ecx, %eax
> +        cmova   %ecx, %eax                      # number of entries to move
> +        movl    %eax, %ecx
> +        shll    $2, %ecx
> +        jz      .Lcopyexit                      # early exit: nothing to move
> +        addl    %eax, %ecx                      # number of 4-byte moves
> +        cld                                     # (5 times of entries)

I don't think you need this - the function is being called from C
code, which assume EFLAGS.DF to be always clear. And to make
the "5 times" more obvious, how about

        cmova   %ecx, %eax                      # number of entries to move
        imul    $5, %eax, %ecx
        jrcxz   .Lcopyexit                      # early exit: nothing to move

And the branch isn't even needed - REP MOVS does the right
thing when %rcx is zero.

> +        rep movsd                               # do the move
> +.Lcopyexit:
> +        retq

Please be consistent: Either suffix all insns, or only those where the
suffix is really needed. And in no case should you use an Intel
mnemonic (movsd) in AT&T syntax code (should be movsl).

> @@ -782,14 +782,16 @@ 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, E820MAX)) != 0 )

ARRAY_SIZE()?

Jan


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

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

[Jan Beulich]

>>> On 23.03.17 at 07:25, <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.
> 
> 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 v2 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

[Andrew Cooper]

On 23/03/17 15:35, Jan Beulich wrote:
>>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
>>>>
>> +        movl    bootsym(e820nr), %ecx
>> +        cmpl    %ecx, %eax
>> +        cmova   %ecx, %eax                      # number of entries to move
>> +        movl    %eax, %ecx
>> +        shll    $2, %ecx
>> +        jz      .Lcopyexit                      # early exit: nothing to move
>> +        addl    %eax, %ecx                      # number of 4-byte moves
>> +        cld                                     # (5 times of entries)
> I don't think you need this - the function is being called from C
> code, which assume EFLAGS.DF to be always clear. And to make
> the "5 times" more obvious, how about
>
>         cmova   %ecx, %eax                      # number of entries to move
>         imul    $5, %eax, %ecx
>         jrcxz   .Lcopyexit                      # early exit: nothing to move
>
> And the branch isn't even needed - REP MOVS does the right
> thing when %rcx is zero.
>
>> +        rep movsd                               # do the move
>> +.Lcopyexit:
>> +        retq
> Please be consistent: Either suffix all insns, or only those where the
> suffix is really needed.

None of these instructions need suffixes, so far as I can tell.  I would
certainly prefer if we opted for only using suffixes when necessary,
because the resulting code is neater.

~Andrew

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

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

[Jan Beulich]

>>> On 23.03.17 at 16:40, <andrew.cooper3@citrix.com> wrote:
> On 23/03/17 15:35, Jan Beulich wrote:
>>>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
>>>>>
>>> +        movl    bootsym(e820nr), %ecx
>>> +        cmpl    %ecx, %eax
>>> +        cmova   %ecx, %eax                      # number of entries to move
>>> +        movl    %eax, %ecx
>>> +        shll    $2, %ecx
>>> +        jz      .Lcopyexit                      # early exit: nothing to 
> move
>>> +        addl    %eax, %ecx                      # number of 4-byte moves
>>> +        cld                                     # (5 times of entries)
>> I don't think you need this - the function is being called from C
>> code, which assume EFLAGS.DF to be always clear. And to make
>> the "5 times" more obvious, how about
>>
>>         cmova   %ecx, %eax                      # number of entries to move
>>         imul    $5, %eax, %ecx
>>         jrcxz   .Lcopyexit                      # early exit: nothing to 
> move
>>
>> And the branch isn't even needed - REP MOVS does the right
>> thing when %rcx is zero.
>>
>>> +        rep movsd                               # do the move
>>> +.Lcopyexit:
>>> +        retq
>> Please be consistent: Either suffix all insns, or only those where the
>> suffix is really needed.
> 
> None of these instructions need suffixes, so far as I can tell.  I would
> certainly prefer if we opted for only using suffixes when necessary,
> because the resulting code is neater.

Same here - I had actually meant to add a "(here: none)" remark,
but then forgot.

Jan


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

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

[Juergen Gross]

On 23/03/17 16:04, Jan Beulich wrote:
>>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
>> @@ -131,6 +151,14 @@ start64:
>>          movabs  $__high_start,%rax
>>          jmpq    *%rax
>>  
>> +#include "wakeup.S"
>> +
>> +/* The first page of trampoline is permanent, the rest boot-time only. */
>> +        .equ    trampoline_boot_start, trampoline_start + PAGE_SIZE
>> +        .global trampoline_boot_start
> 
> The name is at least ambiguous - boot only code starts right here,
> not at the next page boundary. Would it not work to use wakeup_stack
> here, ...
> 
>> --- a/xen/arch/x86/boot/wakeup.S
>> +++ b/xen/arch/x86/boot/wakeup.S
>> @@ -1,6 +1,7 @@
>>          .code16
>>  
>>  #define wakesym(sym) (sym - wakeup_start)
>> +#define wakeup_stack trampoline_start + PAGE_SIZE
> 
> ... omit this #define, and ...
> 
>> --- 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((trampoline_boot_start - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>> +    "wakeup stack too small")
> 
> ... use wakeup_stack here too?

It would work, yes. With my pending patch for releasing the memory of
the boot-only trampoline code this would look a little bit strange:
I'd have to free the memory named "wakeup_stack" and the label
"trampoline_boot_start" would be unused.

In case you'd prefer it I can just place trampoline_boot_start at the
same location as wakeup_stack_start and free the boot trampoline
memory by using the next page boundary.


Juergen

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

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

[Jan Beulich]

>>> On 23.03.17 at 17:44, <jgross@suse.com> wrote:
> On 23/03/17 16:04, Jan Beulich wrote:
>>>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
>>> --- 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((trampoline_boot_start - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>>> +    "wakeup stack too small")
>> 
>> ... use wakeup_stack here too?
> 
> It would work, yes. With my pending patch for releasing the memory of
> the boot-only trampoline code this would look a little bit strange:
> I'd have to free the memory named "wakeup_stack" and the label
> "trampoline_boot_start" would be unused.

Well, the implication from my reply was that trampoline_boot_start
likely can be dropped. Freeing from wakeup_stack onwards seems
quite reasonable, since the stack is the last thing you want to keep.

> In case you'd prefer it I can just place trampoline_boot_start at the
> same location as wakeup_stack_start and free the boot trampoline
> memory by using the next page boundary.

That's an option too. What I'd like to avoid is a mix of things like
in the ASSERT() above - the expression should be in wakeup stack
terms only.

Jan


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

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

[Juergen Gross]

On 23/03/17 16:35, Jan Beulich wrote:
>>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/boot/mem.S
>> +++ b/xen/arch/x86/boot/mem.S
>> @@ -67,10 +67,32 @@ get_memory_map:
>>  
>>          ret
>>  
>> +/*
>> + * Copy E820 map obtained from BIOS to a buffer allocated by Xen.
>> + * Input: %rdi: target address of e820 entry array
>> + *        %rsi: maximum number of entries to copy
> 
> %esi (and also needs to be used that way below)

Okay.

> 
>> + * Output: %rax: number of entries copied
>> + */
>> +        .code64
>> +ENTRY(e820map_copy)
>> +        mov     %rsi, %rax
>> +        movq    $bootsym(e820map), %rsi
> 
> %rip-relative leaq? Even more - is bootsym() usable here at all? The
> comment next to its definition says otherwise. Otoh I'm sure you've
> tested this, so it must be working despite me not seeing how it would
> do so.

Hmm, let me double ckeck. I'm sure to have tested it: the system came up
and the memory map looked as usual.

Oh wait - don't know whether I should cry or laugh: the system is really
fault tolerant! Seems as if e820map_copy returned zero and Xen used the
Multiboot-e820 map!

Thank you very much for questioning using bootsym!

>> +        movl    bootsym(e820nr), %ecx
>> +        cmpl    %ecx, %eax
>> +        cmova   %ecx, %eax                      # number of entries to move
>> +        movl    %eax, %ecx
>> +        shll    $2, %ecx
>> +        jz      .Lcopyexit                      # early exit: nothing to move
>> +        addl    %eax, %ecx                      # number of 4-byte moves
>> +        cld                                     # (5 times of entries)
> 
> I don't think you need this - the function is being called from C
> code, which assume EFLAGS.DF to be always clear. And to make
> the "5 times" more obvious, how about
> 
>         cmova   %ecx, %eax                      # number of entries to move
>         imul    $5, %eax, %ecx
>         jrcxz   .Lcopyexit                      # early exit: nothing to move
> 
> And the branch isn't even needed - REP MOVS does the right
> thing when %rcx is zero.

Okay, I'll change it.

> 
>> +        rep movsd                               # do the move
>> +.Lcopyexit:
>> +        retq
> 
> Please be consistent: Either suffix all insns, or only those where the
> suffix is really needed. And in no case should you use an Intel
> mnemonic (movsd) in AT&T syntax code (should be movsl).

No suffixes then in general and movsl here.

> 
>> @@ -782,14 +782,16 @@ 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, E820MAX)) != 0 )
> 
> ARRAY_SIZE()?

Of course!


Juergen


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

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

[Juergen Gross]

On 23/03/17 17:54, Jan Beulich wrote:
>>>> On 23.03.17 at 17:44, <jgross@suse.com> wrote:
>> On 23/03/17 16:04, Jan Beulich wrote:
>>>>>> On 23.03.17 at 07:25, <jgross@suse.com> wrote:
>>>> --- 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((trampoline_boot_start - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>>>> +    "wakeup stack too small")
>>>
>>> ... use wakeup_stack here too?
>>
>> It would work, yes. With my pending patch for releasing the memory of
>> the boot-only trampoline code this would look a little bit strange:
>> I'd have to free the memory named "wakeup_stack" and the label
>> "trampoline_boot_start" would be unused.
> 
> Well, the implication from my reply was that trampoline_boot_start
> likely can be dropped. Freeing from wakeup_stack onwards seems
> quite reasonable, since the stack is the last thing you want to keep.

Okay, lets do it this way.


Juergen



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