| Message ID | 1414109782-22961-1-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
>>> 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
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. >
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 ) {
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(-)