diff mbox

[Xen-devel,V4,05/15] Add efi_arch_cfg_file() to handle arch specific cfg file fields

Message ID 1410310325-4509-6-git-send-email-roy.franz@linaro.org
State New
Headers show

Commit Message

Roy Franz Sept. 10, 2014, 12:51 a.m. UTC 
Different architectures have some different configuration file
fields that need to be handled.  In particular, x86 has ucode
and ARM has device tree files to be loaded.  This arch specific
function is used to allow each architecture to implement these
features in arch specific code.

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

Comments

Jan Beulich Sept. 11, 2014, 2:16 p.m. UTC | #1 
>>> On 10.09.14 at 02:51, <roy.franz@linaro.org> wrote:
> @@ -752,6 +758,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      }
>      if ( !name.s )
>          blexit(L"No Dom0 kernel image specified.");
> +
> +    efi_arch_cfg_file(dir_handle, section.s);
> +
>      split_value(name.s);
>      read_file(dir_handle, s2w(&name), &kernel);
>      efi_bs->FreePool(name.w);
> @@ -769,17 +778,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          efi_bs->FreePool(name.w);
>      }
>  
> -    name.s = get_value(&cfg, section.s, "ucode");
> -    if ( !name.s )
> -        name.s = get_value(&cfg, "global", "ucode");
> -    if ( name.s )
> -    {
> -        microcode_set_module(mbi.mods_count);
> -        split_value(name.s);
> -        read_file(dir_handle, s2w(&name), &ucode);
> -        efi_bs->FreePool(name.w);
> -    }
> -
>      name.s = get_value(&cfg, section.s, "xsm");
>      if ( name.s )
>      {

While the ordering shouldn't matter that much, is it intentional that
you move this up ahead of the loading of the kernel? If anything,
I'd see this move down after the XSM loading.

Jan
Roy Franz Sept. 11, 2014, 6:11 p.m. UTC | #2 
On Thu, Sep 11, 2014 at 7:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.09.14 at 02:51, <roy.franz@linaro.org> wrote:
>> @@ -752,6 +758,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      }
>>      if ( !name.s )
>>          blexit(L"No Dom0 kernel image specified.");
>> +
>> +    efi_arch_cfg_file(dir_handle, section.s);
>> +
>>      split_value(name.s);
>>      read_file(dir_handle, s2w(&name), &kernel);
>>      efi_bs->FreePool(name.w);
>> @@ -769,17 +778,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>          efi_bs->FreePool(name.w);
>>      }
>>
>> -    name.s = get_value(&cfg, section.s, "ucode");
>> -    if ( !name.s )
>> -        name.s = get_value(&cfg, "global", "ucode");
>> -    if ( name.s )
>> -    {
>> -        microcode_set_module(mbi.mods_count);
>> -        split_value(name.s);
>> -        read_file(dir_handle, s2w(&name), &ucode);
>> -        efi_bs->FreePool(name.w);
>> -    }
>> -
>>      name.s = get_value(&cfg, section.s, "xsm");
>>      if ( name.s )
>>      {
>
> While the ordering shouldn't matter that much, is it intentional that
> you move this up ahead of the loading of the kernel? If anything,
> I'd see this move down after the XSM loading.
>
> Jan
>
Yes, this is intentional, as in the ARM case the configuration file specifies
the device tree to use, and the kernel and other modules are added to this,
so we need this set up before we actually read the other module files.  For x86
the ordering didn't seem to have any dependencies. I can add a comment
explaining this.

Roy
Jan Beulich Sept. 12, 2014, 7:10 a.m. UTC | #3 
>>> On 11.09.14 at 20:11, <roy.franz@linaro.org> wrote:
> On Thu, Sep 11, 2014 at 7:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 10.09.14 at 02:51, <roy.franz@linaro.org> wrote:
>>> @@ -752,6 +758,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>>>      }
>>>      if ( !name.s )
>>>          blexit(L"No Dom0 kernel image specified.");
>>> +
>>> +    efi_arch_cfg_file(dir_handle, section.s);
>>> +
>>>      split_value(name.s);
>>>      read_file(dir_handle, s2w(&name), &kernel);
>>>      efi_bs->FreePool(name.w);
>>> @@ -769,17 +778,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>>>          efi_bs->FreePool(name.w);
>>>      }
>>>
>>> -    name.s = get_value(&cfg, section.s, "ucode");
>>> -    if ( !name.s )
>>> -        name.s = get_value(&cfg, "global", "ucode");
>>> -    if ( name.s )
>>> -    {
>>> -        microcode_set_module(mbi.mods_count);
>>> -        split_value(name.s);
>>> -        read_file(dir_handle, s2w(&name), &ucode);
>>> -        efi_bs->FreePool(name.w);
>>> -    }
>>> -
>>>      name.s = get_value(&cfg, section.s, "xsm");
>>>      if ( name.s )
>>>      {
>>
>> While the ordering shouldn't matter that much, is it intentional that
>> you move this up ahead of the loading of the kernel? If anything,
>> I'd see this move down after the XSM loading.
>>
> Yes, this is intentional, as in the ARM case the configuration file 
> specifies
> the device tree to use, and the kernel and other modules are added to this,
> so we need this set up before we actually read the other module files.  For 
> x86
> the ordering didn't seem to have any dependencies. I can add a comment
> explaining this.

Thing is that (not just here) I prefer allocating big chunks of
memory before smaller ones (i.e. kernel+initrd before ucode+xsm).
Hence maybe we'd be better of with a pre and a post arch hook
here.

Jan
diff mbox

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ca86beb..a33a8f6 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -31,11 +31,6 @@  typedef struct {
     EFI_SHIM_LOCK_VERIFY Verify;
 } EFI_SHIM_LOCK_PROTOCOL;
 
-static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer);
-static CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer);
-static void __init DisplayUint(UINT64 Val, INTN Width);
-static CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s);
-
 union string {
     CHAR16 *w;
     char *s;
@@ -50,6 +45,17 @@  struct file {
     };
 };
 
+static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer);
+static CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer);
+static void __init DisplayUint(UINT64 Val, INTN Width);
+static CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s);
+static char *__init get_value(const struct file *cfg, const char *section,
+                              const char *item);
+static void __init split_value(char *s);
+static CHAR16 *__init s2w(union string *str);
+static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+                               struct file *file);
+
 static EFI_BOOT_SERVICES *__initdata efi_bs;
 static EFI_HANDLE __initdata efi_ih;
 
@@ -752,6 +758,9 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     }
     if ( !name.s )
         blexit(L"No Dom0 kernel image specified.");
+
+    efi_arch_cfg_file(dir_handle, section.s);
+
     split_value(name.s);
     read_file(dir_handle, s2w(&name), &kernel);
     efi_bs->FreePool(name.w);
@@ -769,17 +778,6 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         efi_bs->FreePool(name.w);
     }
 
-    name.s = get_value(&cfg, section.s, "ucode");
-    if ( !name.s )
-        name.s = get_value(&cfg, "global", "ucode");
-    if ( name.s )
-    {
-        microcode_set_module(mbi.mods_count);
-        split_value(name.s);
-        read_file(dir_handle, s2w(&name), &ucode);
-        efi_bs->FreePool(name.w);
-    }
-
     name.s = get_value(&cfg, section.s, "xsm");
     if ( name.s )
     {
diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h
index 518c319..3937955 100644
--- a/xen/include/asm-x86/efi-boot.h
+++ b/xen/include/asm-x86/efi-boot.h
@@ -592,3 +592,18 @@  static void __init efi_arch_post_exit_boot(void)
                      "D" (&mbi)
                    : "memory" );
 }
+
+static void __init efi_arch_cfg_file(EFI_FILE_HANDLE dir_handle, char *section)
+{
+    union string name;
+    name.s = get_value(&cfg, section, "ucode");
+    if ( !name.s )
+        name.s = get_value(&cfg, "global", "ucode");
+    if ( name.s )
+    {
+        microcode_set_module(mbi.mods_count);
+        split_value(name.s);
+        read_file(dir_handle, s2w(&name), &ucode);
+        efi_bs->FreePool(name.w);
+    }
+}