From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH for-xen-4.5 v4 11/18] x86: move legacy BIOS memory map stuff to boot_info Date: Fri, 17 Oct 2014 23:08:25 +0100 Message-ID: <54419359.1080005@citrix.com> References: <1413555132-22138-1-git-send-email-daniel.kiper@oracle.com> <1413555132-22138-12-git-send-email-daniel.kiper@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XfFhG-0007hG-BX for xen-devel@lists.xenproject.org; Fri, 17 Oct 2014 22:08:30 +0000 In-Reply-To: <1413555132-22138-12-git-send-email-daniel.kiper@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel Kiper , xen-devel@lists.xenproject.org Cc: jgross@suse.com, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ross.philipson@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, jbeulich@suse.com, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org On 17/10/2014 15:12, Daniel Kiper wrote: > Signed-off-by: Daniel Kiper > --- > 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 . > */ > > +/* > + * 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 > #include > #include > #include > > #include > +#include > #include > #include > #include > > +/* 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 > #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */ > #include > +#include > #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 > + > +#include > + > /* > * 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*/