Re: [PATCH for-xen-4.5 v4 11/18] x86: move legacy BIOS memory map stuff to boot_info

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/10/2014 15:12, Daniel Kiper wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>   xen/arch/x86/boot_info.c        |  105 +++++++++++++++++++++++++++++++++++----
>   xen/arch/x86/efi/efi-boot.h     |   18 +++----
>   xen/arch/x86/setup.c            |   73 ++-------------------------
>   xen/common/efi/runtime.c        |    7 +++
>   xen/include/asm-x86/boot_info.h |   22 ++++++++
>   xen/include/asm-x86/e820.h      |    8 ---
>   6 files changed, 135 insertions(+), 98 deletions(-)
>
> diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c
> index 7d8b0e5..9e4af78 100644
> --- a/xen/arch/x86/boot_info.c
> +++ b/xen/arch/x86/boot_info.c
> @@ -16,45 +16,126 @@
>    * with this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +/*
> + * Some ideas are taken (out) from xen/arch/x86/boot/reloc.c,
> + * xen/arch/x86/efi/boot.c and xen/arch/x86/setup.c.
> + */
> +

I don't think this comment adds any productive value.  I would just 
discard it.

>   #include <xen/types.h>
>   #include <xen/cache.h>
>   #include <xen/init.h>
>   #include <xen/multiboot.h>
>   
>   #include <asm/boot_info.h>
> +#include <asm/e820.h>
>   #include <asm/mbd.h>
>   #include <asm/page.h>
>   #include <asm/setup.h>
>   
> +/* These symbols live in the boot trampoline. Access via bootsym(). */
> +extern struct e820entry e820map[];
> +extern unsigned int e820nr;
> +extern unsigned int lowmem_kb, highmem_kb;

Strictly speaking, these are uint32_t's rather than unsigned ints.

> +
>   static multiboot_info_t __read_mostly mbi;
>   
>   static boot_info_t __read_mostly boot_info_mb = {
>       .boot_loader_name = "UNKNOWN",
>       .cmdline = NULL,
> +    .mmap_src = NULL,
> +    .mem_upper = 0,
> +    .e820map_nr = 0,
> +    .e820map = NULL,
>       .warn_msg = NULL,
>       .err_msg = NULL
>   };
>   
> -unsigned long __init __init_mbi(u32 mbd_pa)

This is a rather peculiar way for git/diff to have split the patch. Does 
the patient algorithm yield a more intelligible patch?

> +#define e820_raw bootsym(e820map)
> +#define e820_raw_nr bootsym(e820nr)
> +
> +static void __init init_mmap(boot_info_t *boot_info, mbd_t *mbd)
>   {
> -    mbd_t *mbd = __va(mbd_pa);
> +    int bytes = 0;
> +    memory_map_t *map;
>   
> -    enable_bsp_exception_support();
> +    if ( e820_raw_nr )
> +        boot_info->mmap_src = "Xen-e820";
> +    else if ( mbd->mmap_size )
> +    {
> +        boot_info->mmap_src = "Multiboot-e820";
> +
> +        while ( (bytes < mbd->mmap_size) && (e820_raw_nr < E820MAX) )
> +        {
> +            /*
> +             * This is a gross workaround for a BIOS bug. Some bootloaders do
> +             * not write e820 map entries into pre-zeroed memory. This is
> +             * okay if the BIOS fills in all fields of the map entry, but
> +             * some broken BIOSes do not bother to write the high word of
> +             * the length field if the length is smaller than 4GB. We
> +             * detect and fix this by flagging sections below 4GB that
> +             * appear to be larger than 4GB in size.
> +             */
> +            map = __va(mbd->mmap + bytes);
>   
> -    if ( mbd->mem_lower || mbd->mem_upper )
> +            if ( !map->base_addr_high && map->length_high )
> +            {
> +                map->length_high = 0;
> +                boot_info->warn_msg = "WARNING: Buggy e820 map detected and fixed "
> +                                "(truncated length fields).\n";
> +            }
> +
> +            e820_raw[e820_raw_nr].addr =
> +                ((u64)map->base_addr_high << 32) | (u64)map->base_addr_low;
> +            e820_raw[e820_raw_nr].size =
> +                ((u64)map->length_high << 32) | (u64)map->length_low;
> +            e820_raw[e820_raw_nr].type = map->type;
> +            e820_raw_nr++;
> +
> +            bytes += map->size + 4;
> +        }
> +    }
> +    else if ( bootsym(lowmem_kb) )
>       {
> -        mbi.flags |= MBI_MEMLIMITS;
> -        mbi.mem_lower = mbd->mem_lower;
> -        mbi.mem_upper = mbd->mem_upper;
> +        boot_info->mmap_src = "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;
>       }
> +    else if ( mbd->mem_lower || mbd->mem_upper )
> +    {
> +        boot_info->mmap_src = "Multiboot-e801";
>   
> -    if ( mbd->mmap_size )
> +        e820_raw[0].addr = 0;
> +        e820_raw[0].size = mbd->mem_lower << 10;
> +        e820_raw[0].type = E820_RAM;
> +        e820_raw[1].addr = 0x100000;
> +        e820_raw[1].size = mbd->mem_upper << 10;
> +        e820_raw[1].type = E820_RAM;
> +        e820_raw_nr = 2;
> +    }
> +    else
>       {
> -        mbi.flags |= MBI_MEMMAP;
> -        mbi.mmap_length = mbd->mmap_size;
> -        mbi.mmap_addr = mbd->mmap;
> +        boot_info->err_msg = "Bootloader provided no memory information.\n";
> +        return;
>       }
>   
> +    boot_info->mem_upper = mbd->mem_upper;
> +
> +    boot_info->e820map_nr = e820_raw_nr;
> +    boot_info->e820map = e820_raw;
> +}
> +
> +unsigned long __init __init_mbi(u32 mbd_pa)
> +{
> +    mbd_t *mbd = __va(mbd_pa);
> +
> +    enable_bsp_exception_support();
> +
>       if ( mbd->mods_nr )
>       {
>           mbi.flags |= MBI_MODULES;
> @@ -75,5 +156,7 @@ boot_info_t __init *__init_boot_info(u32 mbd_pa)
>       if ( mbd->cmdline )
>           boot_info_mb.cmdline = __va(mbd->cmdline);
>   
> +    init_mmap(&boot_info_mb, mbd);
> +
>       return &boot_info_mb;
>   }
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index f02e604..96e758c 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -148,7 +148,7 @@ 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;
> +    e = boot_info_efi.e820map - 1;

Having a variable by the name of "e820map" of type "e820entry *" is 
confusing alongside the type "e320map", but that is a fault of the 
original code.

However, how can this code ever have worked?  the symbol e820map is a 
bootsym and not valid to be used as a regular pointer like this.

>       for ( i = 0; i < map_size; i += desc_size )
>       {
>           EFI_MEMORY_DESCRIPTOR *desc = map + i;
> @@ -182,10 +182,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>               type = E820_NVS;
>               break;
>           }
> -        if ( e820nr && type == e->type &&
> +        if ( boot_info_efi.e820map_nr && type == e->type &&
>                desc->PhysicalStart == e->addr + e->size )
>               e->size += len;
> -        else if ( !len || e820nr >= E820MAX )
> +        else if ( !len || boot_info_efi.e820map_nr >= E820MAX )
>               continue;
>           else
>           {
> @@ -193,7 +193,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>               e->addr = desc->PhysicalStart;
>               e->size = len;
>               e->type = type;
> -            ++e820nr;
> +            ++boot_info_efi.e820map_nr;
>           }
>       }
>   
> @@ -201,12 +201,12 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>   
>   static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
>   {
> -    place_string_u32(&mbi.mem_upper, NULL);
> -    mbi.mem_upper -= map_size;
> -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> -    if ( mbi.mem_upper < xen_phys_start )
> +    place_string_u32(&boot_info_efi.mem_upper, NULL);
> +    boot_info_efi.mem_upper -= map_size;
> +    boot_info_efi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> +    if ( boot_info_efi.mem_upper < xen_phys_start )
>           return NULL;
> -    return (void *)(long)mbi.mem_upper;
> +    return (void *)(long)boot_info_efi.mem_upper;
>   }
>   
>   static void __init efi_arch_pre_exit_boot(void)
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 8f83969..32d9a3a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -551,13 +551,12 @@ void __init enable_bsp_exception_support(void)
>   
>   void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr)
>   {
> -    char *memmap_type = NULL;
>       char *cmdline, *kextra;
>       unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
>       multiboot_info_t *mbi = (multiboot_info_t *)mbi_p;
>       module_t *mod = (module_t *)__va(mbi->mods_addr);
>       unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
> -    int i, j, e820_warn = 0, bytes = 0;
> +    int i, j;
>       bool_t acpi_boot_table_init_done = 0;
>       struct domain *dom0;
>       struct ns16550_defaults ns16550 = {
> @@ -691,77 +690,11 @@ void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr
>           /* Make boot page tables match non-EFI boot. */
>           l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>               l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> -
> -        memmap_type = boot_info->boot_loader_name;
> -    }
> -    else if ( e820_raw_nr != 0 )
> -    {
> -        memmap_type = "Xen-e820";
>       }
> -    else if ( mbi->flags & MBI_MEMMAP )
> -    {
> -        memmap_type = "Multiboot-e820";
> -        while ( (bytes < mbi->mmap_length) && (e820_raw_nr < E820MAX) )
> -        {
> -            memory_map_t *map = __va(mbi->mmap_addr + bytes);
> -
> -            /*
> -             * This is a gross workaround for a BIOS bug. Some bootloaders do
> -             * not write e820 map entries into pre-zeroed memory. This is
> -             * okay if the BIOS fills in all fields of the map entry, but
> -             * some broken BIOSes do not bother to write the high word of
> -             * the length field if the length is smaller than 4GB. We
> -             * detect and fix this by flagging sections below 4GB that
> -             * appear to be larger than 4GB in size.
> -             */
> -            if ( (map->base_addr_high == 0) && (map->length_high != 0) )
> -            {
> -                if ( !e820_warn )
> -                {
> -                    printk("WARNING: Buggy e820 map detected and fixed "
> -                           "(truncated length fields).\n");
> -                    e820_warn = 1;
> -                }
> -                map->length_high = 0;
> -            }
> -
> -            e820_raw[e820_raw_nr].addr =
> -                ((u64)map->base_addr_high << 32) | (u64)map->base_addr_low;
> -            e820_raw[e820_raw_nr].size =
> -                ((u64)map->length_high << 32) | (u64)map->length_low;
> -            e820_raw[e820_raw_nr].type = map->type;
> -            e820_raw_nr++;
> -
> -            bytes += map->size + 4;
> -        }
> -    }
> -    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;
> -    }
> -    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;
> -    }
> -    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(boot_info->mmap_src, boot_info->e820map,
> +                                        &boot_info->e820map_nr);

A possible consideration for cleanup.  As this is the single caller of 
init_e820, and all the information is derived from boot_info, init_e820 
could drop all 3 of its parameters, and read boot_info straight.

~Andrew

>   
>       /* Create a temporary copy of the E820 map. */
>       memcpy(&boot_e820, &e820, sizeof(e820));
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index ee2ee2d..03c6658 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -7,6 +7,7 @@
>   #include <xen/time.h>
>   #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
>   #include <asm/boot_info.h>
> +#include <asm/e820.h>
>   #endif
>   
>   DEFINE_XEN_GUEST_HANDLE(CHAR16);
> @@ -53,9 +54,15 @@ struct efi __read_mostly efi = {
>   const struct efi_pci_rom *__read_mostly efi_pci_roms;
>   
>   #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> +extern struct e820entry e820map[];
> +
>   boot_info_t __read_mostly boot_info_efi = {
>       .boot_loader_name = "EFI",
>       .cmdline = NULL,
> +    .mmap_src = "EFI",
> +    .mem_upper = 0,
> +    .e820map_nr = 0,
> +    .e820map = e820map,
>       .warn_msg = NULL,
>       .err_msg = NULL
>   };
> diff --git a/xen/include/asm-x86/boot_info.h b/xen/include/asm-x86/boot_info.h
> index 44d1674..a882c0c 100644
> --- a/xen/include/asm-x86/boot_info.h
> +++ b/xen/include/asm-x86/boot_info.h
> @@ -19,6 +19,10 @@
>   #ifndef __BOOT_INFO_H__
>   #define __BOOT_INFO_H__
>   
> +#include <xen/types.h>
> +
> +#include <asm/e820.h>
> +
>   /*
>    * Define boot_info type. It will be used to define variable which in turn
>    * will store data collected by bootloader and preloader. This way it will
> @@ -36,6 +40,24 @@ typedef struct {
>       /* Xen command line. */
>       char *cmdline;
>   
> +    /* Memory map source name. */
> +    const char *mmap_src;
> +
> +    /*
> +     * Amount of upper memory (in KiB) accordingly to The Multiboot
> +     * Specification version 0.6.96.
> +     */
> +    u32 mem_upper;
> +
> +    /* Number of memory map entries provided by Xen preloader. */
> +    unsigned int e820map_nr;
> +
> +    /*
> +     * Memory map provided by Xen preloader. It should always point
> +     * to an area able to accommodate at least E820MAX entries.
> +     */
> +    struct e820entry *e820map;
> +
>       /*
>        * Info about warning occurred during boot_info initialization.
>        * NULL if everything went OK.
> diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
> index d9ff4eb..8727afb 100644
> --- a/xen/include/asm-x86/e820.h
> +++ b/xen/include/asm-x86/e820.h
> @@ -33,12 +33,4 @@ extern int e820_add_range(
>   extern unsigned long init_e820(const char *, struct e820entry *, unsigned int *);
>   extern struct e820map e820;
>   
> -/* 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)
> -
>   #endif /*__E820_HEADER*/