| Message ID | 1410310325-4509-13-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: > - cols = rows = depth = 0; > - if ( !base_video ) > - { > - name.cs = get_value(&cfg, section.s, "video"); > - if ( !name.cs ) > - name.cs = get_value(&cfg, "global", "video"); > - if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) > + cols = rows = depth = 0; > + if ( !base_video ) > { > - cols = simple_strtoul(name.cs + 4, &name.cs, 10); > - if ( *name.cs == 'x' ) > - rows = simple_strtoul(name.cs + 1, &name.cs, 10); > - if ( *name.cs == 'x' ) > - depth = simple_strtoul(name.cs + 1, &name.cs, 10); > - if ( *name.cs ) > - cols = rows = depth = 0; > + name.cs = get_value(&cfg, section.s, "video"); > + if ( !name.cs ) > + name.cs = get_value(&cfg, "global", "video"); > + if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) > + { > + cols = simple_strtoul(name.cs + 4, &name.cs, 10); > + if ( *name.cs == 'x' ) > + rows = simple_strtoul(name.cs + 1, &name.cs, 10); > + if ( *name.cs == 'x' ) > + depth = simple_strtoul(name.cs + 1, &name.cs, 10); > + if ( *name.cs ) > + cols = rows = depth = 0; > + } So how is this video mode selection being represented then without config file? Don't you need to at least add a command line option for that? Jan
On Thu, 11 Sep 2014, Jan Beulich wrote: > >>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: > > - cols = rows = depth = 0; > > - if ( !base_video ) > > - { > > - name.cs = get_value(&cfg, section.s, "video"); > > - if ( !name.cs ) > > - name.cs = get_value(&cfg, "global", "video"); > > - if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) > > + cols = rows = depth = 0; > > + if ( !base_video ) > > { > > - cols = simple_strtoul(name.cs + 4, &name.cs, 10); > > - if ( *name.cs == 'x' ) > > - rows = simple_strtoul(name.cs + 1, &name.cs, 10); > > - if ( *name.cs == 'x' ) > > - depth = simple_strtoul(name.cs + 1, &name.cs, 10); > > - if ( *name.cs ) > > - cols = rows = depth = 0; > > + name.cs = get_value(&cfg, section.s, "video"); > > + if ( !name.cs ) > > + name.cs = get_value(&cfg, "global", "video"); > > + if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) > > + { > > + cols = simple_strtoul(name.cs + 4, &name.cs, 10); > > + if ( *name.cs == 'x' ) > > + rows = simple_strtoul(name.cs + 1, &name.cs, 10); > > + if ( *name.cs == 'x' ) > > + depth = simple_strtoul(name.cs + 1, &name.cs, 10); > > + if ( *name.cs ) > > + cols = rows = depth = 0; > > + } > > So how is this video mode selection being represented then without > config file? Don't you need to at least add a command line option for > that? The scenario without config file is the one where Xen is loaded by GRUB. Do we actually need to pass a video mode option in that case? Wouldn't GRUB take care of changing video mode itself to the value specified by the user before booting Xen? Xen can query the current video mode afterwards.
>>> On 12.09.14 at 01:54, <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 11 Sep 2014, Jan Beulich wrote: >> >>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: >> > - cols = rows = depth = 0; >> > - if ( !base_video ) >> > - { >> > - name.cs = get_value(&cfg, section.s, "video"); >> > - if ( !name.cs ) >> > - name.cs = get_value(&cfg, "global", "video"); >> > - if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) >> > + cols = rows = depth = 0; >> > + if ( !base_video ) >> > { >> > - cols = simple_strtoul(name.cs + 4, &name.cs, 10); >> > - if ( *name.cs == 'x' ) >> > - rows = simple_strtoul(name.cs + 1, &name.cs, 10); >> > - if ( *name.cs == 'x' ) >> > - depth = simple_strtoul(name.cs + 1, &name.cs, 10); >> > - if ( *name.cs ) >> > - cols = rows = depth = 0; >> > + name.cs = get_value(&cfg, section.s, "video"); >> > + if ( !name.cs ) >> > + name.cs = get_value(&cfg, "global", "video"); >> > + if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) >> > + { >> > + cols = simple_strtoul(name.cs + 4, &name.cs, 10); >> > + if ( *name.cs == 'x' ) >> > + rows = simple_strtoul(name.cs + 1, &name.cs, 10); >> > + if ( *name.cs == 'x' ) >> > + depth = simple_strtoul(name.cs + 1, &name.cs, 10); >> > + if ( *name.cs ) >> > + cols = rows = depth = 0; >> > + } >> >> So how is this video mode selection being represented then without >> config file? Don't you need to at least add a command line option for >> that? > > The scenario without config file is the one where Xen is loaded by GRUB. > Do we actually need to pass a video mode option in that case? > Wouldn't GRUB take care of changing video mode itself to the value > specified by the user before booting Xen? Xen can query the current > video mode afterwards. Ah, right, that's a good point. Jan
On Fri, Sep 12, 2014 at 12:15 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 12.09.14 at 01:54, <stefano.stabellini@eu.citrix.com> wrote: > > On Thu, 11 Sep 2014, Jan Beulich wrote: > >> >>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: > >> > - cols = rows = depth = 0; > >> > - if ( !base_video ) > >> > - { > >> > - name.cs = get_value(&cfg, section.s, "video"); > >> > - if ( !name.cs ) > >> > - name.cs = get_value(&cfg, "global", "video"); > >> > - if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) > >> > + cols = rows = depth = 0; > >> > + if ( !base_video ) > >> > { > >> > - cols = simple_strtoul(name.cs + 4, &name.cs, 10); > >> > - if ( *name.cs == 'x' ) > >> > - rows = simple_strtoul(name.cs + 1, &name.cs, 10); > >> > - if ( *name.cs == 'x' ) > >> > - depth = simple_strtoul(name.cs + 1, &name.cs, 10); > >> > - if ( *name.cs ) > >> > - cols = rows = depth = 0; > >> > + name.cs = get_value(&cfg, section.s, "video"); > >> > + if ( !name.cs ) > >> > + name.cs = get_value(&cfg, "global", "video"); > >> > + if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) > >> > + { > >> > + cols = simple_strtoul(name.cs + 4, &name.cs, 10); > >> > + if ( *name.cs == 'x' ) > >> > + rows = simple_strtoul(name.cs + 1, &name.cs, 10); > >> > + if ( *name.cs == 'x' ) > >> > + depth = simple_strtoul(name.cs + 1, &name.cs, > 10); > >> > + if ( *name.cs ) > >> > + cols = rows = depth = 0; > >> > + } > >> > >> So how is this video mode selection being represented then without > >> config file? Don't you need to at least add a command line option for > >> that? > > > > The scenario without config file is the one where Xen is loaded by GRUB. > > Do we actually need to pass a video mode option in that case? > > Wouldn't GRUB take care of changing video mode itself to the value > > specified by the user before booting Xen? Xen can query the current > > video mode afterwards. > > Ah, right, that's a good point. > > Jan > > I think we can leave this to GRUB in this case. It seems to have the capability to set video modes use EFI calls so it should be able to take care of this as well as the EFI boot portion of XEN. One of the difficulties with arm64 development right now is that most of it is being done on platforms without video, so any video code is rarely used or tested. Roy
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 1630dde..36bcfe4 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -574,8 +574,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; - EFI_FILE_HANDLE dir_handle; - union string section = { NULL }, name, file_name, cfg_file_name = {NULL}; + union string section = { NULL }, name = { NULL }, file_name, cfg_file_name = {NULL}; bool_t base_video = 0; UINT32 mmap_desc_ver = 0; UINTN mmap_size, mmap_desc_size, mmap_key = 0; @@ -594,9 +593,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_load_addr_check(loaded_image); - /* Get the file system interface. */ - dir_handle = get_parent_handle(loaded_image, &file_name.w); - argc = get_argv(0, NULL, loaded_image->LoadOptions, loaded_image->LoadOptionsSize, &options); if ( argc > 0 && @@ -688,104 +684,111 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) if ( EFI_ERROR(status) ) gop = NULL; - /* Read and parse the config file. */ - if ( !cfg_file_name.w ) + if ( efi_arch_use_config_file(SystemTable) ) { - CHAR16 *tail; + EFI_FILE_HANDLE dir_handle; - while ( (tail = point_tail(file_name.w)) != NULL ) + /* Get the file system interface. */ + dir_handle = get_parent_handle(loaded_image, &file_name.w); + /* Read and parse the config file. */ + if ( !cfg_file_name.w ) { - wstrcpy(tail, L".cfg"); - if ( read_file(dir_handle, &cfg, w2s(&file_name)) ) - break; - *tail = 0; + CHAR16 *tail; + + while ( (tail = point_tail(file_name.w)) != NULL ) + { + wstrcpy(tail, L".cfg"); + if ( read_file(dir_handle, &cfg, w2s(&file_name)) ) + break; + *tail = 0; + } + if ( !tail ) + blexit(L"No configuration file found."); + PrintStr(L"Using configuration file '"); + s2w(&file_name); + PrintStr(file_name.w); + PrintStr(L"'\r\n"); + efi_bs->FreePool(file_name.w); } - if ( !tail ) - blexit(L"No configuration file found."); - PrintStr(L"Using configuration file '"); - s2w(&file_name); - PrintStr(file_name.w); - PrintStr(L"'\r\n"); - efi_bs->FreePool(file_name.w); - } - else if ( !read_file(dir_handle, &cfg, w2s(&cfg_file_name)) ) - blexit(L"Configuration file not found."); - pre_parse(&cfg); + else if ( !read_file(dir_handle, &cfg, w2s(&cfg_file_name)) ) + blexit(L"Configuration file not found."); + pre_parse(&cfg); - if ( section.w ) - w2s(§ion); - else - section.s = get_value(&cfg, "global", "default"); + if ( section.w ) + w2s(§ion); + else + section.s = get_value(&cfg, "global", "default"); - for ( ; ; ) - { - name.s = get_value(&cfg, section.s, "kernel"); - if ( name.s ) - break; - name.s = get_value(&cfg, "global", "chain"); - if ( !name.s ) - break; - efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - cfg.addr = 0; - if ( !read_file(dir_handle, &cfg, name.s) ) + for ( ; ; ) { - PrintStr(L"Chained configuration file '"); - PrintStr(name.w); + name.s = get_value(&cfg, section.s, "kernel"); + if ( name.s ) + break; + name.s = get_value(&cfg, "global", "chain"); + if ( !name.s ) + break; + efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); + cfg.addr = 0; + if ( !read_file(dir_handle, &cfg, name.s) ) + { + PrintStr(L"Chained configuration file '"); + PrintStr(name.w); + efi_bs->FreePool(name.w); + blexit(L"'not found."); + } + pre_parse(&cfg); efi_bs->FreePool(name.w); - blexit(L"'not found."); } - pre_parse(&cfg); - efi_bs->FreePool(name.w); - } - if ( !name.s ) - blexit(L"No Dom0 kernel image specified."); - - efi_arch_cfg_file(dir_handle, section.s); + if ( !name.s ) + blexit(L"No Dom0 kernel image specified."); - read_file(dir_handle, &kernel, name.s); + efi_arch_cfg_file(dir_handle, section.s); - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) + read_file(dir_handle, &kernel, name.s); + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) && + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) PrintErrMesg(L"Dom0 kernel image could not be verified", status); - name.s = get_value(&cfg, section.s, "ramdisk"); - if ( name.s ) - { - read_file(dir_handle, &ramdisk, name.s); - } + name.s = get_value(&cfg, section.s, "ramdisk"); + if ( name.s ) + { + read_file(dir_handle, &ramdisk, name.s); + } - name.s = get_value(&cfg, section.s, "xsm"); - if ( name.s ) - { - read_file(dir_handle, &xsm, name.s); - } + name.s = get_value(&cfg, section.s, "xsm"); + if ( name.s ) + { + read_file(dir_handle, &xsm, name.s); + } - cols = rows = depth = 0; - if ( !base_video ) - { - name.cs = get_value(&cfg, section.s, "video"); - if ( !name.cs ) - name.cs = get_value(&cfg, "global", "video"); - if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) + cols = rows = depth = 0; + if ( !base_video ) { - cols = simple_strtoul(name.cs + 4, &name.cs, 10); - if ( *name.cs == 'x' ) - rows = simple_strtoul(name.cs + 1, &name.cs, 10); - if ( *name.cs == 'x' ) - depth = simple_strtoul(name.cs + 1, &name.cs, 10); - if ( *name.cs ) - cols = rows = depth = 0; + name.cs = get_value(&cfg, section.s, "video"); + if ( !name.cs ) + name.cs = get_value(&cfg, "global", "video"); + if ( name.cs && !strncmp(name.cs, "gfx-", 4) ) + { + cols = simple_strtoul(name.cs + 4, &name.cs, 10); + if ( *name.cs == 'x' ) + rows = simple_strtoul(name.cs + 1, &name.cs, 10); + if ( *name.cs == 'x' ) + depth = simple_strtoul(name.cs + 1, &name.cs, 10); + if ( *name.cs ) + cols = rows = depth = 0; + } } - } + name.s = get_value(&cfg, section.s, "options"); - name.s = get_value(&cfg, section.s, "options"); - efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s); + efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); + cfg.addr = 0; + + dir_handle->Close(dir_handle); - efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - cfg.addr = 0; + efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s); + } - dir_handle->Close(dir_handle); efi_arch_edd(); /* XXX Collect EDID info. */ @@ -818,3 +821,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) for( ; ; ); /* not reached */ } +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h index e7e9b15..65483d3 100644 --- a/xen/include/asm-x86/efi-boot.h +++ b/xen/include/asm-x86/efi-boot.h @@ -1063,3 +1063,17 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image) blexit(L"Xen must be loaded at a 2Mb boundary."); trampoline_xen_phys_start = xen_phys_start; } + +static __init bool_t efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) +{ + return 1; /* x86 always uses a config file */ +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
The x86 EFI build of Xen always uses a configuration file to load modules, but the ARM version can either use a config file to specify the modules, or be loaded by GRUB in which case GRUB loads the modules and adds them to the DTB that is passed to Xen. Add the efi_arch_use_config_file() to indicate if a configuration file is required. For x86, this will always be true. ARM will examine the DTB passed via EFI configuration table (if any), and if it contains module information will use that that not use the configuration file at all. Add Emacs footer to efi-boot.h and boot.c Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/common/efi/boot.c | 177 ++++++++++++++++++++++------------------- xen/include/asm-x86/efi-boot.h | 14 ++++ 2 files changed, 108 insertions(+), 83 deletions(-)