[PATCH for-4.5] EFI: Always use EFI command line

[PATCH for-4.5] EFI: Always use EFI command line

[Roy Franz]

This patch changes the ARM EFI boot code to always use the EFI commandline,
even when loaded by GRUB, which makes it consistent with Linux EFI booting.
The code previously incorrectly skipped processing of the EFI command line when
modules are present in the loader supplied FDT and the config file is not used.
There is no change in behavior for x86 since it unconditionally uses the config
file.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/common/efi/boot.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4257341..c0d6768 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -702,7 +702,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
-    union string section = { NULL }, name;
+    union string section = { NULL }, name, cfg_options = { NULL };
     bool_t base_video = 0;
     char *option_str;
 
@@ -904,8 +904,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
             efi_bs->FreePool(name.w);
         }
 
-        name.s = get_value(&cfg, section.s, "options");
-        efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
+        cfg_options.s = get_value(&cfg, section.s, "options");
 
         if ( !base_video )
         {
@@ -930,8 +929,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         cfg.addr = 0;
 
         dir_handle->Close(dir_handle);
-
     }
+    efi_arch_handle_cmdline(argc ? *argv : NULL, options, cfg_options.s);
 
     if ( gop && !base_video )
     {
-- 
2.1.0

Re: [PATCH for-4.5] EFI: Always use EFI command line

[Ian Campbell]

On Thu, 2014-10-23 at 17:16 -0700, Roy Franz wrote:
> This patch changes the ARM EFI boot code to always use the EFI commandline,
> even when loaded by GRUB, which makes it consistent with Linux EFI booting.
> The code previously incorrectly skipped processing of the EFI command line when
> modules are present in the loader supplied FDT and the config file is not used.
> There is no change in behavior for x86 since it unconditionally uses the config
> file.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

Please can you update whichever of the docs are relevant as part of this
patch i.e. One or more of misc/arm/booting.txt,
misc/arm/device-tree/booting.txt or misc/efi.markdown might plausibly
need updates to describe the new precedence for where a command line
will be taken from.

http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot will also need updating at some point.

How does this interact with the presence/absence of a config file and
any potential options= in that?

What about the "efi options" like "-cfg=" which are currently handled
from the UEFI command line, are they still correctly handled? (looks
like the others are -help and -basevideo)

Ian.

Re: [PATCH for-4.5] EFI: Always use EFI command line

[Roy Franz]

On Thu, Oct 23, 2014 at 11:43 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-10-23 at 17:16 -0700, Roy Franz wrote:
>> This patch changes the ARM EFI boot code to always use the EFI commandline,
>> even when loaded by GRUB, which makes it consistent with Linux EFI booting.
>> The code previously incorrectly skipped processing of the EFI command line when
>> modules are present in the loader supplied FDT and the config file is not used.
>> There is no change in behavior for x86 since it unconditionally uses the config
>> file.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>
> Please can you update whichever of the docs are relevant as part of this
> patch i.e. One or more of misc/arm/booting.txt,
> misc/arm/device-tree/booting.txt or misc/efi.markdown might plausibly
> need updates to describe the new precedence for where a command line
> will be taken from.
(Adding Leif who has done the GRUB arm64 Linux EFI boot support.)

I am reviewing those and will update to clarify.

>
> http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot will also need updating at some point.
>

Yes, we will need to update this to indicate that the Xen commandline
can/should be passed using the EFI
command line when booting using EFI.  For the "Image' boot protocol
there is no 'native' commandline,
so the FDT is the only way.  For the EFI case, using the EFI
commandline to pass the commandline to
Xen brings us into alignment with Linux, and allows more shared code in GRUB.

> How does this interact with the presence/absence of a config file and
> any potential options= in that?

If a config file is present, the Xen options on the EFI command line
will be appended
to the options in the config file "options" field.  This behavior is
unchanged by my changes
to add arm64 support, and I will update the efi.markdown file to state this.

>
> What about the "efi options" like "-cfg=" which are currently handled
> from the UEFI command line, are they still correctly handled? (looks
> like the others are -help and -basevideo)

I think that "-basevideo" is the only one (that currently exists) that
would be useful from GRUB.  -cfg
is explicitly ignored if GRUB loads modules, and while -help is
supported, I don't think it's that useful.

These are currently processed from the EFI command line, the problem
is that the rest of the EFI
command line is ignored in the GRUB case, and required to be put in
the DTB by GRUB, which makes
Xen different that Linux in this regard.


>
> Ian.
>

Re: [PATCH for-4.5] EFI: Always use EFI command line

[Jan Beulich]

>>> On 24.10.14 at 02:16, <roy.franz@linaro.org> wrote:
> There is no change in behavior for x86 since it unconditionally uses the 
> config file.

I'm afraid there is:

> @@ -904,8 +904,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>              efi_bs->FreePool(name.w);
>          }
>  
> -        name.s = get_value(&cfg, section.s, "options");
> -        efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
> +        cfg_options.s = get_value(&cfg, section.s, "options");
>  
>          if ( !base_video )
>          {

Between this and the below code fragments there is

        efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));

> @@ -930,8 +929,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          cfg.addr = 0;
>  
>          dir_handle->Close(dir_handle);
> -
>      }
> +    efi_arch_handle_cmdline(argc ? *argv : NULL, options, cfg_options.s);

i.e. you're accessing freed data here.

Jan