| Message ID | 20221123154525.63068-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gfx: improvements when using multiboot2 and EFI + misc | expand |
On 23.11.2022 16:45, Roger Pau Monne wrote: > Only set the GOP mode if vga is selected in the console option, > otherwise just fetch the information from the current mode in order to > make it available to dom0. > > Introduce support for passing the command line to the efi_multiboot2() > helper, and parse the console= option if present. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I'm unsure why the parsing of the multiboot2 tags is done in assembly, > it could very well be done in efi_multiboot2() in C, but I don't want > to switch that code now. I guess that's mainly mirroring the non-EFI boot path, where the amount of work needed to eventually enter C land is quite a bit larger? Anything beyond that Daniel may want to point out. > @@ -265,6 +266,15 @@ __efi64_mb2_start: > cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) > je .Lrun_bs > > + /* > + * Get command line from Multiboot2 information. > + * Must be last parsed tag. Why? And how do you guarantee this? > + */ > + cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx) > + jne .Lefi_mb2_next_tag > + mov %rcx,%rdx > + add $(MB2_tag_string),%rdx Simply "lea MB2_tag_string(%rcx),%rdx"? > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -786,7 +786,22 @@ 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 the last occurrence of opt in cmd. */ Is this sufficient in the general case (it may be for "console=", but perhaps not for "vga=", which may also need finding as per below)? > +static const char __init *get_option(const char *cmd, const char *opt) Nit: The first * wants to move earlier. > +{ > + const char *s = cmd, *o = NULL; > + > + while ( (s = strstr(s, opt)) != NULL ) I'm afraid this is too easy to break without considering separators as well. If I'm not mistaken you'd also match e.g. "sync_console=1" for the sole present caller. > + { > + s += strlen(opt); > + o = s; > + } > + > + 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; > @@ -807,7 +822,21 @@ 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 *opt = get_option(cmdline, "console="); > + bool vga = false; > + > + if ( opt ) > + { > + const char *s = strstr(opt, "vga"); > + > + if ( s && s < strpbrk(opt, " \0")) > + vga = true; > + } Don't you also want to find a "vga=gfx-..." option, to avoid ... > + if ( vga ) > + { > + gop_mode = efi_find_gop_mode(gop, 0, 0, 0); ... requesting a "random" mode here? > + } Nit: No need for the braces in cases like this one. Jan
On 05.12.2022 16:10, Jan Beulich wrote: > On 23.11.2022 16:45, Roger Pau Monne wrote: >> @@ -807,7 +822,21 @@ 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 *opt = get_option(cmdline, "console="); >> + bool vga = false; >> + >> + if ( opt ) >> + { >> + const char *s = strstr(opt, "vga"); >> + >> + if ( s && s < strpbrk(opt, " \0")) >> + vga = true; >> + } > > Don't you also want to find a "vga=gfx-..." option, to avoid ... Apologies - I should have looked at the title of the next patch before writing this. Jan
Sorry for late reply... On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote: > On 23.11.2022 16:45, Roger Pau Monne wrote: > > Only set the GOP mode if vga is selected in the console option, > > otherwise just fetch the information from the current mode in order to > > make it available to dom0. > > > > Introduce support for passing the command line to the efi_multiboot2() > > helper, and parse the console= option if present. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > I'm unsure why the parsing of the multiboot2 tags is done in assembly, > > it could very well be done in efi_multiboot2() in C, but I don't want > > to switch that code now. > > I guess that's mainly mirroring the non-EFI boot path, where the amount > of work needed to eventually enter C land is quite a bit larger? > Anything beyond that Daniel may want to point out. Yeah, you are right, its mainly mirroring the non-EFI boot path and it evolved from that code. However, if you want to move it to C go for it... Daniel
On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote: > On 23.11.2022 16:45, Roger Pau Monne wrote: > > @@ -265,6 +266,15 @@ __efi64_mb2_start: > > cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) > > je .Lrun_bs > > > > + /* > > + * Get command line from Multiboot2 information. > > + * Must be last parsed tag. > > Why? And how do you guarantee this? I think the comment is misleading, must be the last checked for tag in the loop that does this in assembly, because it's not using cmove. I've adjusted to: /* 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: Maybe there's some instruction I'm missing similar to a conditional lea? > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -786,7 +786,22 @@ 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 the last occurrence of opt in cmd. */ > > Is this sufficient in the general case (it may be for "console=", but > perhaps not for "vga=", which may also need finding as per below)? I guess for vga= it's possible to have something like: vga=current vga=keep And in that case we do care about the non-last option. > > +static const char __init *get_option(const char *cmd, const char *opt) > > Nit: The first * wants to move earlier. > > > +{ > > + const char *s = cmd, *o = NULL; > > + > > + while ( (s = strstr(s, opt)) != NULL ) > > I'm afraid this is too easy to break without considering separators as > well. If I'm not mistaken you'd also match e.g. "sync_console=1" for > the sole present caller. Right, wasn't taking into account that sync_console can also be a boolean (and thus assigned). > > + } > > Nit: No need for the braces in cases like this one. I've added the braces here because it will be expended in the next patch. Since you asked for both patches to be merged this will go away. Thanks, Roger.
On 29.03.2023 18:29, Roger Pau Monné wrote: > On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote: >> On 23.11.2022 16:45, Roger Pau Monne wrote: >>> @@ -265,6 +266,15 @@ __efi64_mb2_start: >>> cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) >>> je .Lrun_bs >>> >>> + /* >>> + * Get command line from Multiboot2 information. >>> + * Must be last parsed tag. >> >> Why? And how do you guarantee this? > > I think the comment is misleading, must be the last checked for tag in > the loop that does this in assembly, because it's not using cmove. > I've adjusted to: > > /* 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: > > Maybe there's some instruction I'm missing similar to a conditional > lea? There isn't. If you want to get away without conditional branch, then you'll need to LEA to a scratch register (unconditional) and then CMOVcc from that scratch register. Jan
On Thu, Mar 30, 2023 at 08:24:20AM +0200, Jan Beulich wrote: > On 29.03.2023 18:29, Roger Pau Monné wrote: > > On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote: > >> On 23.11.2022 16:45, Roger Pau Monne wrote: > >>> @@ -265,6 +266,15 @@ __efi64_mb2_start: > >>> cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) > >>> je .Lrun_bs > >>> > >>> + /* > >>> + * Get command line from Multiboot2 information. > >>> + * Must be last parsed tag. > >> > >> Why? And how do you guarantee this? > > > > I think the comment is misleading, must be the last checked for tag in > > the loop that does this in assembly, because it's not using cmove. > > I've adjusted to: > > > > /* 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: > > > > Maybe there's some instruction I'm missing similar to a conditional > > lea? > > There isn't. If you want to get away without conditional branch, then > you'll need to LEA to a scratch register (unconditional) and then > CMOVcc from that scratch register. Likely not worth it, unless you dislike the extra conditional branch. Thanks, Roger.
On 30.03.2023 10:11, Roger Pau Monné wrote: > On Thu, Mar 30, 2023 at 08:24:20AM +0200, Jan Beulich wrote: >> On 29.03.2023 18:29, Roger Pau Monné wrote: >>> On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote: >>>> On 23.11.2022 16:45, Roger Pau Monne wrote: >>>>> @@ -265,6 +266,15 @@ __efi64_mb2_start: >>>>> cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) >>>>> je .Lrun_bs >>>>> >>>>> + /* >>>>> + * Get command line from Multiboot2 information. >>>>> + * Must be last parsed tag. >>>> >>>> Why? And how do you guarantee this? >>> >>> I think the comment is misleading, must be the last checked for tag in >>> the loop that does this in assembly, because it's not using cmove. >>> I've adjusted to: >>> >>> /* 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: >>> >>> Maybe there's some instruction I'm missing similar to a conditional >>> lea? >> >> There isn't. If you want to get away without conditional branch, then >> you'll need to LEA to a scratch register (unconditional) and then >> CMOVcc from that scratch register. > > Likely not worth it, unless you dislike the extra conditional branch. Entirely up to you - I'm fine either way. Jan
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 0fb7dd3029..6920ad08d1 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -221,9 +221,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 @@ -265,6 +266,15 @@ __efi64_mb2_start: cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) je .Lrun_bs + /* + * Get command line from Multiboot2 information. + * Must be last parsed tag. + */ + cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx) + jne .Lefi_mb2_next_tag + mov %rcx,%rdx + add $(MB2_tag_string),%rdx + .Lefi_mb2_next_tag: /* Go to next Multiboot2 information tag. */ add MB2_tag_size(%rcx),%ecx @@ -324,7 +334,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 27f928ed3c..695491a5b7 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -786,7 +786,22 @@ 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 the last occurrence of opt in cmd. */ +static const char __init *get_option(const char *cmd, const char *opt) +{ + const char *s = cmd, *o = NULL; + + while ( (s = strstr(s, opt)) != NULL ) + { + s += strlen(opt); + o = s; + } + + 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; @@ -807,7 +822,21 @@ 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 *opt = get_option(cmdline, "console="); + bool vga = false; + + if ( opt ) + { + const char *s = strstr(opt, "vga"); + + if ( s && s < strpbrk(opt, " \0")) + vga = true; + } + + if ( vga ) + { + gop_mode = efi_find_gop_mode(gop, 0, 0, 0); + } 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 287dac101a..fbd6c54188 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);
Only set the GOP mode if vga is selected in the console option, otherwise just fetch the information from the current mode in order to make it available to dom0. Introduce support for passing the command line to the efi_multiboot2() helper, and parse the console= option if present. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- I'm unsure why the parsing of the multiboot2 tags is done in assembly, it could very well be done in efi_multiboot2() in C, but I don't want to switch that code now. --- xen/arch/x86/boot/head.S | 15 ++++++++++++-- xen/arch/x86/efi/efi-boot.h | 33 +++++++++++++++++++++++++++++-- xen/arch/x86/x86_64/asm-offsets.c | 1 + 3 files changed, 45 insertions(+), 4 deletions(-)