| Message ID | 20230705114741.11449-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/gfx: early boot improvements | expand |
On 05.07.2023 13:47, Roger Pau Monne wrote: > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -795,7 +795,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > +/* Return a pointer to the character after the first occurrence of opt in cmd */ > +static const char __init *get_option(const char *cmd, const char *opt) Nit: __init and * want to change places. > @@ -816,7 +839,54 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > > if ( gop ) > { > - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > + const char *cur = cmdline; > + unsigned int width = 0, height = 0, depth = 0; > + bool keep_current = false; > + > + while ( (cur = get_option(cur, "vga=")) != NULL ) > + { > +#define VALID_TERMINATOR(c) \ > + (*(c) == ' ' || *(c) == '\t' || *(c) == '\0' || *(c) == ',') > + if ( !strncmp(cur, "gfx-", 4) ) > + { > + width = simple_strtoul(cur + 4, &cur, 10); > + > + if ( *cur == 'x' ) > + height = simple_strtoul(cur + 1, &cur, 10); > + else > + goto error; > + > + if ( *cur == 'x' ) > + depth = simple_strtoul(cur + 1, &cur, 10); > + else > + goto error; > + > + if ( !VALID_TERMINATOR(cur) ) > + { > +error: Nit: Labels want to be indented by at least one blank. Here I'm inclined to suggest indenting to the level of the enclosing curly braces. > + PrintStr(L"Warning: Invalid gfx- option detected.\r\n"); Maybe better PrintErr() and no trailing full stop? > + width = height = depth = 0; > + } > + keep_current = false; > + } > + else if ( !strncmp(cur, "current", 7) && VALID_TERMINATOR(cur + 7) ) > + keep_current = true; > + else if ( !strncmp(cur, "keep", 4) && VALID_TERMINATOR(cur + 4) ) > + { > + /* Ignore, handled in later vga= parsing. */ > + } > + else > + { > + /* Fallback to defaults if unimplemented. */ > + width = height = depth = 0; > + keep_current = false; > + PrintStr(L"Warning: Cannot use selected vga option.\r\n"); Same here then? With these addressed (which are all mechanical and hence can probably be done while committing, as long as we can reach agreement) Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Thu, Jul 06, 2023 at 12:41:58PM +0200, Jan Beulich wrote: > On 05.07.2023 13:47, Roger Pau Monne wrote: > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -795,7 +795,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > > > > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > > +/* Return a pointer to the character after the first occurrence of opt in cmd */ > > +static const char __init *get_option(const char *cmd, const char *opt) > > Nit: __init and * want to change places. Hm, yes. I assume that placing it before the return type is not OK? (static const __init char ...) > > @@ -816,7 +839,54 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > > > > if ( gop ) > > { > > - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > > + const char *cur = cmdline; > > + unsigned int width = 0, height = 0, depth = 0; > > + bool keep_current = false; > > + > > + while ( (cur = get_option(cur, "vga=")) != NULL ) > > + { > > +#define VALID_TERMINATOR(c) \ > > + (*(c) == ' ' || *(c) == '\t' || *(c) == '\0' || *(c) == ',') > > + if ( !strncmp(cur, "gfx-", 4) ) > > + { > > + width = simple_strtoul(cur + 4, &cur, 10); > > + > > + if ( *cur == 'x' ) > > + height = simple_strtoul(cur + 1, &cur, 10); > > + else > > + goto error; > > + > > + if ( *cur == 'x' ) > > + depth = simple_strtoul(cur + 1, &cur, 10); > > + else > > + goto error; > > + > > + if ( !VALID_TERMINATOR(cur) ) > > + { > > +error: > > Nit: Labels want to be indented by at least one blank. Here I'm > inclined to suggest indenting to the level of the enclosing curly > braces. > > > + PrintStr(L"Warning: Invalid gfx- option detected.\r\n"); > > Maybe better PrintErr() and no trailing full stop? Yes, please adjust if you don't mind. > > > + width = height = depth = 0; > > + } > > + keep_current = false; > > + } > > + else if ( !strncmp(cur, "current", 7) && VALID_TERMINATOR(cur + 7) ) > > + keep_current = true; > > + else if ( !strncmp(cur, "keep", 4) && VALID_TERMINATOR(cur + 4) ) > > + { > > + /* Ignore, handled in later vga= parsing. */ > > + } > > + else > > + { > > + /* Fallback to defaults if unimplemented. */ > > + width = height = depth = 0; > > + keep_current = false; > > + PrintStr(L"Warning: Cannot use selected vga option.\r\n"); > > Same here then? > > With these addressed (which are all mechanical and hence can probably > be done while committing, as long as we can reach agreement) > Reviewed-by: Jan Beulich <jbeulich@suse.com> LGTM, please adjust if you don't mind, otherwise I can send an adjusted version. Thanks, Roger.
On 07.07.2023 12:13, Roger Pau Monné wrote: > On Thu, Jul 06, 2023 at 12:41:58PM +0200, Jan Beulich wrote: >> On 05.07.2023 13:47, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/efi/efi-boot.h >>> +++ b/xen/arch/x86/efi/efi-boot.h >>> @@ -795,7 +795,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) >>> >>> static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } >>> >>> -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >>> +/* Return a pointer to the character after the first occurrence of opt in cmd */ >>> +static const char __init *get_option(const char *cmd, const char *opt) >> >> Nit: __init and * want to change places. > > Hm, yes. I assume that placing it before the return type is not OK? > (static const __init char ...) That's still in the middle of the return type then. Technically gcc accepts it being placed anywhere, but they reserve the right to change meaning when not placed appropriately. Recall that you may alter both attributes of a function (or variable) and attributes of types. Hence to disambiguate both, proper placement may become necessary down the road. And while it might be that static __init const char *... would also be okay-ish (albeit I'm not certain), that's still against how we do things commonly (i.e. a not written down style aspect). >>>[...] > > LGTM, please adjust if you don't mind, otherwise I can send an > adjusted version. No need to send an update. Jan
On Fri, Jul 07, 2023 at 12:24:10PM +0200, Jan Beulich wrote: > On 07.07.2023 12:13, Roger Pau Monné wrote: > > On Thu, Jul 06, 2023 at 12:41:58PM +0200, Jan Beulich wrote: > >> On 05.07.2023 13:47, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/efi/efi-boot.h > >>> +++ b/xen/arch/x86/efi/efi-boot.h > >>> @@ -795,7 +795,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > >>> > >>> static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > >>> > >>> -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > >>> +/* Return a pointer to the character after the first occurrence of opt in cmd */ > >>> +static const char __init *get_option(const char *cmd, const char *opt) > >> > >> Nit: __init and * want to change places. > > > > Hm, yes. I assume that placing it before the return type is not OK? > > (static const __init char ...) > > That's still in the middle of the return type then. Technically gcc > accepts it being placed anywhere, but they reserve the right to change > meaning when not placed appropriately. Recall that you may alter both > attributes of a function (or variable) and attributes of types. Hence > to disambiguate both, proper placement may become necessary down the > road. And while it might be that > > static __init const char *... > > would also be okay-ish (albeit I'm not certain), that's still against > how we do things commonly (i.e. a not written down style aspect). Thanks for the explanation. Roger.
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index e03f52c75535..1eb829ab419f 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -237,9 +237,10 @@ __efi64_mb2_start: jmp x86_32_switch .Lefi_multiboot2_proto: - /* Zero EFI SystemTable and EFI ImageHandle addresses. */ + /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */ xor %esi,%esi xor %edi,%edi + xor %edx,%edx /* Skip Multiboot2 information fixed part. */ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx @@ -277,6 +278,13 @@ __efi64_mb2_start: cmove MB2_efi64_ih(%rcx),%rdi je .Lefi_mb2_next_tag + /* Get command line from Multiboot2 information. */ + cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE, MB2_tag_type(%rcx) + jne .Lno_cmdline + lea MB2_tag_string(%rcx), %rdx + jmp .Lefi_mb2_next_tag +.Lno_cmdline: + /* Is it the end of Multiboot2 information? */ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) je .Lrun_bs @@ -340,7 +348,8 @@ __efi64_mb2_start: /* * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, + * %rdx - MB2 cmdline */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 64c1a02cf10a..4394c7276aa3 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -795,7 +795,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) +/* Return a pointer to the character after the first occurrence of opt in cmd */ +static const char __init *get_option(const char *cmd, const char *opt) +{ + const char *s = cmd, *o = NULL; + + if ( !cmd || !opt ) + return NULL; + + while ( (s = strstr(s, opt)) != NULL ) + { + if ( s == cmd || *(s - 1) == ' ' || *(s - 1) == '\t' ) + { + o = s + strlen(opt); + break; + } + + s += strlen(opt); + } + + return o; +} + +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, + const char *cmdline) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; EFI_HANDLE gop_handle; @@ -816,7 +839,54 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable if ( gop ) { - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); + const char *cur = cmdline; + unsigned int width = 0, height = 0, depth = 0; + bool keep_current = false; + + while ( (cur = get_option(cur, "vga=")) != NULL ) + { +#define VALID_TERMINATOR(c) \ + (*(c) == ' ' || *(c) == '\t' || *(c) == '\0' || *(c) == ',') + if ( !strncmp(cur, "gfx-", 4) ) + { + width = simple_strtoul(cur + 4, &cur, 10); + + if ( *cur == 'x' ) + height = simple_strtoul(cur + 1, &cur, 10); + else + goto error; + + if ( *cur == 'x' ) + depth = simple_strtoul(cur + 1, &cur, 10); + else + goto error; + + if ( !VALID_TERMINATOR(cur) ) + { +error: + PrintStr(L"Warning: Invalid gfx- option detected.\r\n"); + width = height = depth = 0; + } + keep_current = false; + } + else if ( !strncmp(cur, "current", 7) && VALID_TERMINATOR(cur + 7) ) + keep_current = true; + else if ( !strncmp(cur, "keep", 4) && VALID_TERMINATOR(cur + 4) ) + { + /* Ignore, handled in later vga= parsing. */ + } + else + { + /* Fallback to defaults if unimplemented. */ + width = height = depth = 0; + keep_current = false; + PrintStr(L"Warning: Cannot use selected vga option.\r\n"); + } +#undef VALID_TERMINATOR + } + + if ( !keep_current ) + gop_mode = efi_find_gop_mode(gop, width, height, depth); efi_arch_edid(gop_handle); } diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index 287dac101ad4..fbd6c54188db 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -175,6 +175,7 @@ void __dummy__(void) OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower); OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer); OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer); + OFFSET(MB2_tag_string, multiboot2_tag_string_t, string); BLANK(); OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
Introduce support for passing the command line to the efi_multiboot2() helper, and parse the vga= option if present. Add support for the 'gfx' and 'current' vga options, ignore the 'keep' option, and print a warning message about other options not being currently implemented. Note that the multboot2 command line string must always be zero-terminated according to the multiboot2 specification, and hence there's no need to pass the string size to efi_multiboot2(). Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v3: - s/last/cur/ - asm style fixes. - Add note about cmdline always being 0 terminated. - Also allow \t as valid option separator (as space). - Expand comment about expected get_option() behavior. - Always check for valid option terminator. Changes since v2: - Do not parse console=. - Allow width or height to be 0 as long as the gfx- option is well formed. Changes since v1: - Do not return the last occurrence of a command line. - Rearrange the code for assembly processing of the cmdline and use lea. - Merge patches handling console= and vga= together. --- xen/arch/x86/boot/head.S | 13 +++++- xen/arch/x86/efi/efi-boot.h | 74 ++++++++++++++++++++++++++++++- xen/arch/x86/x86_64/asm-offsets.c | 1 + 3 files changed, 84 insertions(+), 4 deletions(-)