diff mbox

[Xen-devel,V5,03/15] create arch functions to allocate memory for and process EFI memory map.

Message ID 1411080607-32365-4-git-send-email-roy.franz@linaro.org
State New
Headers show

Commit Message

Roy Franz Sept. 18, 2014, 10:49 p.m. UTC 
The memory used to store the EFI memory map is allocated in an architecture
specific way, and the processing of the memory map itself uses x86 specific
data structures. This patch adds architecture specific funtions so each
architecture can provide its own implementation.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/common/efi/boot.c          | 79 ++++++++----------------------------------
 xen/include/asm-x86/efi-boot.h | 76 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 64 deletions(-)

Comments

Jan Beulich Sept. 19, 2014, 8:47 a.m. UTC | #1 
>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote:
> @@ -657,16 +655,18 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      EFI_STATUS status;
>      unsigned int i, argc;
>      CHAR16 **argv, *file_name, *cfg_file_name = NULL;
> -    UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0;
> +    UINTN cols, rows, depth, size, info_size, gop_mode = ~0;
>      EFI_HANDLE *handles = NULL;
>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>      EFI_FILE_HANDLE dir_handle;
>      union string section = { NULL }, name;
> -    struct e820entry *e;
>      u64 efer;
>      bool_t base_video = 0;
> +    UINT32 mmap_desc_ver = 0;
> +    UINTN mmap_size, mmap_desc_size, mmap_key = 0;

Are these initializers really needed?

Also I previously commented about mdesc_ver having been static
rather than automatic for a reason, yet you still don't retain that.

Similarly for efi_memmap_size, efi_mdesc_size, and efi_memmap:
With runtime.c moved and (presumably later in the series) also
built for ARM, there's no point in moving the setting of these into
efi_arch_process_memory_map().

> @@ -1262,67 +1260,20 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          }
>      }
>  
> -    efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
> -                         &efi_mdesc_size, &mdesc_ver);
> -    mbi.mem_upper -= efi_memmap_size;
> -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> -    if ( mbi.mem_upper < xen_phys_start )
> -        blexit(L"Out of static memory");
> -    efi_memmap = (void *)(long)mbi.mem_upper;
> -    status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
> -                                  &efi_mdesc_size, &mdesc_ver);
> +    efi_bs->GetMemoryMap(&mmap_size, NULL, &mmap_key,
> +                         &mmap_desc_size, &mmap_desc_ver);
> +    mmap = efi_arch_allocate_mmap_buffer(mmap_size);
> +    if ( !mmap )
> +        blexit(L"ERROR Unable to allocate memory for EFI memory map\r\n");

blexit() appends a newline itself.

Jan
Roy Franz Sept. 23, 2014, 1:14 a.m. UTC | #2 
On Fri, Sep 19, 2014 at 1:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote:
>> @@ -657,16 +655,18 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      EFI_STATUS status;
>>      unsigned int i, argc;
>>      CHAR16 **argv, *file_name, *cfg_file_name = NULL;
>> -    UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0;
>> +    UINTN cols, rows, depth, size, info_size, gop_mode = ~0;
>>      EFI_HANDLE *handles = NULL;
>>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>>      EFI_FILE_HANDLE dir_handle;
>>      union string section = { NULL }, name;
>> -    struct e820entry *e;
>>      u64 efer;
>>      bool_t base_video = 0;
>> +    UINT32 mmap_desc_ver = 0;
>> +    UINTN mmap_size, mmap_desc_size, mmap_key = 0;
>
> Are these initializers really needed?
The base_video is, some of the others aren't in the current form.  I will
audit the initialized local variables here.
>
> Also I previously commented about mdesc_ver having been static
> rather than automatic for a reason, yet you still don't retain that.

Sorry, I had missed that bit in the virtual mapping issues.  I'll return this
to being static.
>
> Similarly for efi_memmap_size, efi_mdesc_size, and efi_memmap:
> With runtime.c moved and (presumably later in the series) also
> built for ARM, there's no point in moving the setting of these into
> efi_arch_process_memory_map().

Yes, I missed undoing this as part of pulling the in the global
runtime variables.  These should
all be able to be reverted.

>
>> @@ -1262,67 +1260,20 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>          }
>>      }
>>
>> -    efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>> -                         &efi_mdesc_size, &mdesc_ver);
>> -    mbi.mem_upper -= efi_memmap_size;
>> -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
>> -    if ( mbi.mem_upper < xen_phys_start )
>> -        blexit(L"Out of static memory");
>> -    efi_memmap = (void *)(long)mbi.mem_upper;
>> -    status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
>> -                                  &efi_mdesc_size, &mdesc_ver);
>> +    efi_bs->GetMemoryMap(&mmap_size, NULL, &mmap_key,
>> +                         &mmap_desc_size, &mmap_desc_ver);
>> +    mmap = efi_arch_allocate_mmap_buffer(mmap_size);
>> +    if ( !mmap )
>> +        blexit(L"ERROR Unable to allocate memory for EFI memory map\r\n");
>
> blexit() appends a newline itself.

I'll fix this and review the other blexit() calls.
>
> Jan
>
diff mbox

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1a6cd7c..307740b 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -61,8 +61,6 @@  static EFI_HANDLE __initdata efi_ih;
 static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
 static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
 
-static UINT32 __initdata mdesc_ver;
-
 static struct file __initdata cfg;
 static struct file __initdata kernel;
 static struct file __initdata ramdisk;
@@ -657,16 +655,18 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     EFI_STATUS status;
     unsigned int i, argc;
     CHAR16 **argv, *file_name, *cfg_file_name = NULL;
-    UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0;
+    UINTN cols, rows, depth, size, info_size, gop_mode = ~0;
     EFI_HANDLE *handles = NULL;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
     EFI_FILE_HANDLE dir_handle;
     union string section = { NULL }, name;
-    struct e820entry *e;
     u64 efer;
     bool_t base_video = 0;
+    UINT32 mmap_desc_ver = 0;
+    UINTN mmap_size, mmap_desc_size, mmap_key = 0;
+    void *mmap;
 
     efi_ih = ImageHandle;
     efi_bs = SystemTable->BootServices;
@@ -966,8 +966,6 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     mbi.boot_loader_name = (long)"EFI";
     mbi.mods_addr = (long)mb_modules;
 
-    place_string(&mbi.mem_upper, NULL);
-
     /* Collect EDD info. */
     BUILD_BUG_ON(offsetof(struct edd_info, edd_device_params) != EDDEXTSIZE);
     BUILD_BUG_ON(sizeof(struct edd_device_params) != EDDPARMSIZE);
@@ -1262,67 +1260,20 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         }
     }
 
-    efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
-                         &efi_mdesc_size, &mdesc_ver);
-    mbi.mem_upper -= efi_memmap_size;
-    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
-    if ( mbi.mem_upper < xen_phys_start )
-        blexit(L"Out of static memory");
-    efi_memmap = (void *)(long)mbi.mem_upper;
-    status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
-                                  &efi_mdesc_size, &mdesc_ver);
+    efi_bs->GetMemoryMap(&mmap_size, NULL, &mmap_key,
+                         &mmap_desc_size, &mmap_desc_ver);
+    mmap = efi_arch_allocate_mmap_buffer(mmap_size);
+    if ( !mmap )
+        blexit(L"ERROR Unable to allocate memory for EFI memory map\r\n");
+
+    status = efi_bs->GetMemoryMap(&mmap_size, mmap, &mmap_key,
+                         &mmap_desc_size, &mmap_desc_ver);
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Cannot obtain memory map", status);
 
-    /* Populate E820 table and check trampoline area availability. */
-    e = e820map - 1;
-    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
-    {
-        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
-        u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
-        u32 type;
+    efi_arch_process_memory_map(SystemTable, mmap, mmap_size,
+                                mmap_desc_size, mmap_desc_ver);
 
-        switch ( desc->Type )
-        {
-        default:
-            type = E820_RESERVED;
-            break;
-        case EfiConventionalMemory:
-        case EfiBootServicesCode:
-        case EfiBootServicesData:
-            if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
-                 len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
-                cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
-            /* fall through */
-        case EfiLoaderCode:
-        case EfiLoaderData:
-            if ( desc->Attribute & EFI_MEMORY_WB )
-                type = E820_RAM;
-            else
-        case EfiUnusableMemory:
-                type = E820_UNUSABLE;
-            break;
-        case EfiACPIReclaimMemory:
-            type = E820_ACPI;
-            break;
-        case EfiACPIMemoryNVS:
-            type = E820_NVS;
-            break;
-        }
-        if ( e820nr && type == e->type &&
-             desc->PhysicalStart == e->addr + e->size )
-            e->size += len;
-        else if ( !len || e820nr >= E820MAX )
-            continue;
-        else
-        {
-            ++e;
-            e->addr = desc->PhysicalStart;
-            e->size = len;
-            e->type = type;
-            ++e820nr;
-        }
-    }
     if ( !trampoline_phys )
     {
         if ( !cfg.addr )
@@ -1330,7 +1281,7 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         relocate_trampoline(cfg.addr);
     }
 
-    status = efi_bs->ExitBootServices(ImageHandle, map_key);
+    status = efi_bs->ExitBootServices(ImageHandle, mmap_key);
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Cannot exit boot services", status);
 
diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h
index f250941..8832b21 100644
--- a/xen/include/asm-x86/efi-boot.h
+++ b/xen/include/asm-x86/efi-boot.h
@@ -133,3 +133,79 @@  static void __init place_string(u32 *addr, const char *s)
     }
     *addr = (long)alloc;
 }
+
+static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
+                                               void *map,
+                                               UINTN map_size,
+                                               UINTN desc_size,
+                                               UINT32 desc_ver)
+{
+    struct e820entry *e;
+    unsigned int i;
+
+    /* Set global EFI memory map variables */
+    efi_memmap_size = map_size;
+    efi_mdesc_size = desc_size;
+    efi_memmap = map;
+
+    /* Populate E820 table and check trampoline area availability. */
+    e = e820map - 1;
+    for ( i = 0; i < map_size; i += desc_size )
+    {
+        EFI_MEMORY_DESCRIPTOR *desc = map + i;
+        u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+        u32 type;
+
+        switch ( desc->Type )
+        {
+        default:
+            type = E820_RESERVED;
+            break;
+        case EfiConventionalMemory:
+        case EfiBootServicesCode:
+        case EfiBootServicesData:
+            if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
+                 len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
+                cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
+            /* fall through */
+        case EfiLoaderCode:
+        case EfiLoaderData:
+            if ( desc->Attribute & EFI_MEMORY_WB )
+                type = E820_RAM;
+            else
+        case EfiUnusableMemory:
+                type = E820_UNUSABLE;
+            break;
+        case EfiACPIReclaimMemory:
+            type = E820_ACPI;
+            break;
+        case EfiACPIMemoryNVS:
+            type = E820_NVS;
+            break;
+        }
+        if ( e820nr && type == e->type &&
+             desc->PhysicalStart == e->addr + e->size )
+            e->size += len;
+        else if ( !len || e820nr >= E820MAX )
+            continue;
+        else
+        {
+            ++e;
+            e->addr = desc->PhysicalStart;
+            e->size = len;
+            e->type = type;
+            ++e820nr;
+        }
+    }
+
+}
+
+static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
+{
+    place_string(&mbi.mem_upper, NULL);
+    mbi.mem_upper -= map_size;
+    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
+    if ( mbi.mem_upper < xen_phys_start )
+        return NULL;
+    return (void *)(long)mbi.mem_upper;
+}