| Message ID | 1415075851-8987-1-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 03, 2014 at 08:37:31PM -0800, Roy Franz wrote: > Update EFI code to completely ignore the EFI comnandline when booted from GRUB. > Previusly it was parsed of EFI boot specific options, but these aren't used > when booted from GRUB. > > Don't do EFI console or video configuration when booted by GRUB. The EFI boot > code does some console and video initialization to support native EFI boot from > the EFI boot manager or EFI shell. This initlization should not be done when > booted using GRUB. > > Update EFI documentation to indicate that it describes EFI native boot, and > does not apply at all when Xen is booted using GRUB. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> but... > --- > This patch implements what I understand to be the desired behavior when booting > an EFI Xen image via GRUB based on the thread last week. The EFI command line > is not used, and the Xen commandline comes via the multiboot protocol (and > in the ARM case the multiboot FDT bindings). This brings the x86 and arm64 > GRUB EFI boot cases into alignment in not using the EFI commandline. > > The current state of the arm64 code takes the Xen commandline from the FDT, > but still looks in the EFI commandline for EFI boot specific options. If > unexpected options are passed in the EFI commandline, it will generate > "unrecognized option" ouput for all unexpected options. > > I think this should be considered for 4.5. > > Changes from v1: > * Fixed style, and removed blank line changes > * Reviewed scope of variables now that more code is in if ( use_cfg_file ) block > > > docs/misc/efi.markdown | 5 ++ > xen/common/efi/boot.c | 240 +++++++++++++++++++++++++------------------------ > 2 files changed, 128 insertions(+), 117 deletions(-) > > diff --git a/docs/misc/efi.markdown b/docs/misc/efi.markdown > index 5e48aa3..f435ec7 100644 > --- a/docs/misc/efi.markdown > +++ b/docs/misc/efi.markdown > @@ -29,6 +29,11 @@ separators will be tried) to be present in the same directory as the binary. > (To illustrate the name handling, a binary named `xen-4.2-unstable.efi` would > try `xen-4.2-unstable.cfg`, `xen-4.2.cfg`, `xen-4.cfg`, and `xen.cfg` in > order.) One can override this with a command line option (`-cfg=<filename>`). > +This configuration file and EFI commandline are only used for booting directly > +from EFI firmware, or when using an EFI loader that does not support > +the multiboot2 protocol. When booting using GRUB or another multiboot aware > +loader the EFI commandline is ignored and all information is passed from > +the loader to Xen using the multiboot protocol. First you are referring to multiboot2 and later you are talking about multiboot. It makes some confusion. Could you put full stop after "... using an EFI loader" or rephrase this sentences in another way to make them clear? Daniel
On Tue, Nov 4, 2014 at 3:57 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote: > On Mon, Nov 03, 2014 at 08:37:31PM -0800, Roy Franz wrote: >> Update EFI code to completely ignore the EFI comnandline when booted from GRUB. >> Previusly it was parsed of EFI boot specific options, but these aren't used >> when booted from GRUB. >> >> Don't do EFI console or video configuration when booted by GRUB. The EFI boot >> code does some console and video initialization to support native EFI boot from >> the EFI boot manager or EFI shell. This initlization should not be done when >> booted using GRUB. >> >> Update EFI documentation to indicate that it describes EFI native boot, and >> does not apply at all when Xen is booted using GRUB. >> >> Signed-off-by: Roy Franz <roy.franz@linaro.org> > > In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> but... > >> --- >> This patch implements what I understand to be the desired behavior when booting >> an EFI Xen image via GRUB based on the thread last week. The EFI command line >> is not used, and the Xen commandline comes via the multiboot protocol (and >> in the ARM case the multiboot FDT bindings). This brings the x86 and arm64 >> GRUB EFI boot cases into alignment in not using the EFI commandline. >> >> The current state of the arm64 code takes the Xen commandline from the FDT, >> but still looks in the EFI commandline for EFI boot specific options. If >> unexpected options are passed in the EFI commandline, it will generate >> "unrecognized option" ouput for all unexpected options. >> >> I think this should be considered for 4.5. >> >> Changes from v1: >> * Fixed style, and removed blank line changes >> * Reviewed scope of variables now that more code is in if ( use_cfg_file ) block >> >> >> docs/misc/efi.markdown | 5 ++ >> xen/common/efi/boot.c | 240 +++++++++++++++++++++++++------------------------ >> 2 files changed, 128 insertions(+), 117 deletions(-) >> >> diff --git a/docs/misc/efi.markdown b/docs/misc/efi.markdown >> index 5e48aa3..f435ec7 100644 >> --- a/docs/misc/efi.markdown >> +++ b/docs/misc/efi.markdown >> @@ -29,6 +29,11 @@ separators will be tried) to be present in the same directory as the binary. >> (To illustrate the name handling, a binary named `xen-4.2-unstable.efi` would >> try `xen-4.2-unstable.cfg`, `xen-4.2.cfg`, `xen-4.cfg`, and `xen.cfg` in >> order.) One can override this with a command line option (`-cfg=<filename>`). >> +This configuration file and EFI commandline are only used for booting directly >> +from EFI firmware, or when using an EFI loader that does not support >> +the multiboot2 protocol. When booting using GRUB or another multiboot aware >> +loader the EFI commandline is ignored and all information is passed from >> +the loader to Xen using the multiboot protocol. > > First you are referring to multiboot2 and later you are talking about > multiboot. It makes some confusion. Could you put full stop after "... > using an EFI loader" or rephrase this sentences in another way to make > them clear? > > Daniel I think all references should be multiboot2. I do really want something that is clear to most readers. How about: "This configuration file and EFI commandline are only used for booting directly from EFI firmware, or when using an EFI loader. When booting using GRUB or another multiboot2 aware loader the EFI commandline is ignored and all information is passed from the loader to Xen using the multiboot protocol." Would it be helpful to list an example EFI loader, such as gummiboot? "... or when using an EFI loader (such as gummiboot.) When booting using GRUB" gummiboot and other EFI loaders just execute another EFI application, so as far as Xen is concerned this would be like booting from the EFI shell. Thanks, Roy
On Tue, Nov 04, 2014 at 10:46:15AM -0800, Roy Franz wrote: > On Tue, Nov 4, 2014 at 3:57 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote: > > On Mon, Nov 03, 2014 at 08:37:31PM -0800, Roy Franz wrote: > >> Update EFI code to completely ignore the EFI comnandline when booted from GRUB. > >> Previusly it was parsed of EFI boot specific options, but these aren't used > >> when booted from GRUB. > >> > >> Don't do EFI console or video configuration when booted by GRUB. The EFI boot > >> code does some console and video initialization to support native EFI boot from > >> the EFI boot manager or EFI shell. This initlization should not be done when > >> booted using GRUB. > >> > >> Update EFI documentation to indicate that it describes EFI native boot, and > >> does not apply at all when Xen is booted using GRUB. > >> > >> Signed-off-by: Roy Franz <roy.franz@linaro.org> > > > > In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> but... > > > >> --- > >> This patch implements what I understand to be the desired behavior when booting > >> an EFI Xen image via GRUB based on the thread last week. The EFI command line > >> is not used, and the Xen commandline comes via the multiboot protocol (and > >> in the ARM case the multiboot FDT bindings). This brings the x86 and arm64 > >> GRUB EFI boot cases into alignment in not using the EFI commandline. > >> > >> The current state of the arm64 code takes the Xen commandline from the FDT, > >> but still looks in the EFI commandline for EFI boot specific options. If > >> unexpected options are passed in the EFI commandline, it will generate > >> "unrecognized option" ouput for all unexpected options. > >> > >> I think this should be considered for 4.5. > >> > >> Changes from v1: > >> * Fixed style, and removed blank line changes > >> * Reviewed scope of variables now that more code is in if ( use_cfg_file ) block > >> > >> > >> docs/misc/efi.markdown | 5 ++ > >> xen/common/efi/boot.c | 240 +++++++++++++++++++++++++------------------------ > >> 2 files changed, 128 insertions(+), 117 deletions(-) > >> > >> diff --git a/docs/misc/efi.markdown b/docs/misc/efi.markdown > >> index 5e48aa3..f435ec7 100644 > >> --- a/docs/misc/efi.markdown > >> +++ b/docs/misc/efi.markdown > >> @@ -29,6 +29,11 @@ separators will be tried) to be present in the same directory as the binary. > >> (To illustrate the name handling, a binary named `xen-4.2-unstable.efi` would > >> try `xen-4.2-unstable.cfg`, `xen-4.2.cfg`, `xen-4.cfg`, and `xen.cfg` in > >> order.) One can override this with a command line option (`-cfg=<filename>`). > >> +This configuration file and EFI commandline are only used for booting directly > >> +from EFI firmware, or when using an EFI loader that does not support > >> +the multiboot2 protocol. When booting using GRUB or another multiboot aware > >> +loader the EFI commandline is ignored and all information is passed from > >> +the loader to Xen using the multiboot protocol. > > > > First you are referring to multiboot2 and later you are talking about > > multiboot. It makes some confusion. Could you put full stop after "... > > using an EFI loader" or rephrase this sentences in another way to make > > them clear? > > > > Daniel > I think all references should be multiboot2. I do really want > something that is clear > to most readers. > > How about: > > "This configuration file and EFI commandline are only used for booting directly > from EFI firmware, or when using an EFI loader. When booting using GRUB or > another multiboot2 aware loader the EFI commandline is ignored and all information > is passed from the loader to Xen using the multiboot protocol." Much better but right now it indirectly suggest that multiboot2 protocol is available on all platforms including x86 which is not true (at least now). Additionally, I am a bit confused about multiboot protocol naming on ARM64. It looks that http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot calls it multiboot. You say multiboot2. Which one is correct? Could you enlighten me? > Would it be helpful to list an example EFI loader, such as gummiboot? > > "... or when using an EFI loader (such as gummiboot.) When booting using GRUB" Yes, please. Hmmm... Should not we use "simple EFI application loader" instead of "EFI loader"? Daniel
diff --git a/docs/misc/efi.markdown b/docs/misc/efi.markdown index 5e48aa3..f435ec7 100644 --- a/docs/misc/efi.markdown +++ b/docs/misc/efi.markdown @@ -29,6 +29,11 @@ separators will be tried) to be present in the same directory as the binary. (To illustrate the name handling, a binary named `xen-4.2-unstable.efi` would try `xen-4.2-unstable.cfg`, `xen-4.2.cfg`, `xen-4.cfg`, and `xen.cfg` in order.) One can override this with a command line option (`-cfg=<filename>`). +This configuration file and EFI commandline are only used for booting directly +from EFI firmware, or when using an EFI loader that does not support +the multiboot2 protocol. When booting using GRUB or another multiboot aware +loader the EFI commandline is ignored and all information is passed from +the loader to Xen using the multiboot protocol. The configuration file consists of one or more sections headed by a section name enclosed in square brackets, with individual values specified in each diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4257341..471dff7 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -697,7 +697,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) EFI_STATUS status; unsigned int i, argc; CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; - UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0; + UINTN map_key, info_size, gop_mode = ~0; EFI_HANDLE *handles = NULL; EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; @@ -705,6 +705,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) union string section = { NULL }, name; bool_t base_video = 0; char *option_str; + bool_t use_cfg_file; efi_ih = ImageHandle; efi_bs = SystemTable->BootServices; @@ -717,6 +718,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) StdOut = SystemTable->ConOut; StdErr = SystemTable->StdErr ?: StdOut; + use_cfg_file = efi_arch_use_config_file(SystemTable); status = efi_bs->HandleProtocol(ImageHandle, &loaded_image_guid, (void **)&loaded_image); @@ -725,67 +727,71 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_load_addr_check(loaded_image); - argc = get_argv(0, NULL, loaded_image->LoadOptions, - loaded_image->LoadOptionsSize, NULL); - if ( argc > 0 && - efi_bs->AllocatePool(EfiLoaderData, - (argc + 1) * sizeof(*argv) + - loaded_image->LoadOptionsSize, - (void **)&argv) == EFI_SUCCESS ) - get_argv(argc, argv, loaded_image->LoadOptions, - loaded_image->LoadOptionsSize, &options); - else - argc = 0; - for ( i = 1; i < argc; ++i ) + if ( use_cfg_file ) { - CHAR16 *ptr = argv[i]; - - if ( !ptr ) - break; - if ( *ptr == L'/' || *ptr == L'-' ) + argc = get_argv(0, NULL, loaded_image->LoadOptions, + loaded_image->LoadOptionsSize, NULL); + if ( argc > 0 && + efi_bs->AllocatePool(EfiLoaderData, + (argc + 1) * sizeof(*argv) + + loaded_image->LoadOptionsSize, + (void **)&argv) == EFI_SUCCESS ) + get_argv(argc, argv, loaded_image->LoadOptions, + loaded_image->LoadOptionsSize, &options); + else + argc = 0; + for ( i = 1; i < argc; ++i ) { - if ( wstrcmp(ptr + 1, L"basevideo") == 0 ) - base_video = 1; - else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 ) - cfg_file_name = ptr + 5; - else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 ) - cfg_file_name = argv[++i]; - else if ( wstrcmp(ptr + 1, L"help") == 0 || - (ptr[1] == L'?' && !ptr[2]) ) + CHAR16 *ptr = argv[i]; + + if ( !ptr ) + break; + if ( *ptr == L'/' || *ptr == L'-' ) { - PrintStr(L"Xen EFI Loader options:\r\n"); - PrintStr(L"-basevideo retain current video mode\r\n"); - PrintStr(L"-cfg=<file> specify configuration file\r\n"); - PrintStr(L"-help, -? display this help\r\n"); - blexit(NULL); + if ( wstrcmp(ptr + 1, L"basevideo") == 0 ) + base_video = 1; + else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 ) + cfg_file_name = ptr + 5; + else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 ) + cfg_file_name = argv[++i]; + else if ( wstrcmp(ptr + 1, L"help") == 0 || + (ptr[1] == L'?' && !ptr[2]) ) + { + PrintStr(L"Xen EFI Loader options:\r\n"); + PrintStr(L"-basevideo retain current video mode\r\n"); + PrintStr(L"-cfg=<file> specify configuration file\r\n"); + PrintStr(L"-help, -? display this help\r\n"); + blexit(NULL); + } + else + { + PrintStr(L"WARNING: Unknown command line option '"); + PrintStr(ptr); + PrintStr(L"' ignored\r\n"); + } } else - { - PrintStr(L"WARNING: Unknown command line option '"); - PrintStr(ptr); - PrintStr(L"' ignored\r\n"); - } + section.w = ptr; } - else - section.w = ptr; - } - if ( !base_video ) - { - unsigned int best; - - for ( i = 0, size = 0, best = StdOut->Mode->Mode; - i < StdOut->Mode->MaxMode; ++i ) + if ( !base_video ) { - if ( StdOut->QueryMode(StdOut, i, &cols, &rows) == EFI_SUCCESS && - cols * rows > size ) + unsigned int best; + UINTN cols, rows, size; + + for ( i = 0, size = 0, best = StdOut->Mode->Mode; + i < StdOut->Mode->MaxMode; ++i ) { - size = cols * rows; - best = i; + if ( StdOut->QueryMode(StdOut, i, &cols, &rows) == EFI_SUCCESS && + cols * rows > size ) + { + size = cols * rows; + best = i; + } } + if ( best != StdOut->Mode->Mode ) + StdOut->SetMode(StdOut, best); } - if ( best != StdOut->Mode->Mode ) - StdOut->SetMode(StdOut, best); } PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION) @@ -793,37 +799,38 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_relocate_image(0); - if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, - &cols, &rows) == EFI_SUCCESS ) - efi_arch_console_init(cols, rows); - - size = 0; - status = efi_bs->LocateHandle(ByProtocol, &gop_guid, NULL, &size, NULL); - if ( status == EFI_BUFFER_TOO_SMALL ) - status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); - if ( !EFI_ERROR(status) ) - status = efi_bs->LocateHandle(ByProtocol, &gop_guid, NULL, &size, - handles); - if ( EFI_ERROR(status) ) - size = 0; - for ( i = 0; i < size / sizeof(*handles); ++i ) - { - status = efi_bs->HandleProtocol(handles[i], &gop_guid, (void **)&gop); - if ( EFI_ERROR(status) ) - continue; - status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info); - if ( !EFI_ERROR(status) ) - break; - } - if ( handles ) - efi_bs->FreePool(handles); - if ( EFI_ERROR(status) ) - gop = NULL; - - cols = rows = depth = 0; - if ( efi_arch_use_config_file(SystemTable) ) + if ( use_cfg_file ) { EFI_FILE_HANDLE dir_handle; + UINTN depth, cols, rows, size; + + size = cols = rows = depth = 0; + + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, + &cols, &rows) == EFI_SUCCESS ) + efi_arch_console_init(cols, rows); + + status = efi_bs->LocateHandle(ByProtocol, &gop_guid, NULL, &size, NULL); + if ( status == EFI_BUFFER_TOO_SMALL ) + status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles); + if ( !EFI_ERROR(status) ) + status = efi_bs->LocateHandle(ByProtocol, &gop_guid, NULL, &size, + handles); + if ( EFI_ERROR(status) ) + size = 0; + for ( i = 0; i < size / sizeof(*handles); ++i ) + { + status = efi_bs->HandleProtocol(handles[i], &gop_guid, (void **)&gop); + if ( EFI_ERROR(status) ) + continue; + status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info); + if ( !EFI_ERROR(status) ) + break; + } + if ( handles ) + efi_bs->FreePool(handles); + if ( EFI_ERROR(status) ) + gop = NULL; /* Get the file system interface. */ dir_handle = get_parent_handle(loaded_image, &file_name); @@ -931,45 +938,44 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) dir_handle->Close(dir_handle); - } - - if ( gop && !base_video ) - { - for ( i = size = 0; i < gop->Mode->MaxMode; ++i ) + if ( gop && !base_video ) { - unsigned int bpp = 0; - - status = gop->QueryMode(gop, i, &info_size, &mode_info); - if ( EFI_ERROR(status) ) - continue; - switch ( mode_info->PixelFormat ) - { - case PixelBitMask: - bpp = hweight32(mode_info->PixelInformation.RedMask | - mode_info->PixelInformation.GreenMask | - mode_info->PixelInformation.BlueMask); - break; - case PixelRedGreenBlueReserved8BitPerColor: - case PixelBlueGreenRedReserved8BitPerColor: - bpp = 24; - break; - default: - continue; - } - if ( cols == mode_info->HorizontalResolution && - rows == mode_info->VerticalResolution && - (!depth || bpp == depth) ) + for ( i = size = 0; i < gop->Mode->MaxMode; ++i ) { - gop_mode = i; - break; - } - if ( !cols && !rows && - mode_info->HorizontalResolution * - mode_info->VerticalResolution > size ) - { - size = mode_info->HorizontalResolution * - mode_info->VerticalResolution; - gop_mode = i; + unsigned int bpp = 0; + + status = gop->QueryMode(gop, i, &info_size, &mode_info); + if ( EFI_ERROR(status) ) + continue; + switch ( mode_info->PixelFormat ) + { + case PixelBitMask: + bpp = hweight32(mode_info->PixelInformation.RedMask | + mode_info->PixelInformation.GreenMask | + mode_info->PixelInformation.BlueMask); + break; + case PixelRedGreenBlueReserved8BitPerColor: + case PixelBlueGreenRedReserved8BitPerColor: + bpp = 24; + break; + default: + continue; + } + if ( cols == mode_info->HorizontalResolution && + rows == mode_info->VerticalResolution && + (!depth || bpp == depth) ) + { + gop_mode = i; + break; + } + if ( !cols && !rows && + mode_info->HorizontalResolution * + mode_info->VerticalResolution > size ) + { + size = mode_info->HorizontalResolution * + mode_info->VerticalResolution; + gop_mode = i; + } } } }
Update EFI code to completely ignore the EFI comnandline when booted from GRUB. Previusly it was parsed of EFI boot specific options, but these aren't used when booted from GRUB. Don't do EFI console or video configuration when booted by GRUB. The EFI boot code does some console and video initialization to support native EFI boot from the EFI boot manager or EFI shell. This initlization should not be done when booted using GRUB. Update EFI documentation to indicate that it describes EFI native boot, and does not apply at all when Xen is booted using GRUB. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- This patch implements what I understand to be the desired behavior when booting an EFI Xen image via GRUB based on the thread last week. The EFI command line is not used, and the Xen commandline comes via the multiboot protocol (and in the ARM case the multiboot FDT bindings). This brings the x86 and arm64 GRUB EFI boot cases into alignment in not using the EFI commandline. The current state of the arm64 code takes the Xen commandline from the FDT, but still looks in the EFI commandline for EFI boot specific options. If unexpected options are passed in the EFI commandline, it will generate "unrecognized option" ouput for all unexpected options. I think this should be considered for 4.5. Changes from v1: * Fixed style, and removed blank line changes * Reviewed scope of variables now that more code is in if ( use_cfg_file ) block docs/misc/efi.markdown | 5 ++ xen/common/efi/boot.c | 240 +++++++++++++++++++++++++------------------------ 2 files changed, 128 insertions(+), 117 deletions(-)