Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[PATCH 4/5] multiboot2: parse console= option when setting GOP mode
roger.pau at citrix
Nov 23, 2022, 7:45 AM
Post #1 of 8 (36 views)
Permalink
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(-)

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);
--
2.37.3
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
jbeulich at suse
Dec 5, 2022, 7:10 AM
Post #2 of 8 (35 views)
Permalink
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
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
jbeulich at suse
Dec 5, 2022, 8:01 AM
Post #3 of 8 (35 views)
Permalink
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
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
daniel.kiper at oracle
Dec 13, 2022, 3:41 AM
Post #4 of 8 (34 views)
Permalink
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
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
roger.pau at citrix
Mar 29, 2023, 9:29 AM
Post #5 of 8 (29 views)
Permalink
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.
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
jbeulich at suse
Mar 29, 2023, 11:24 PM
Post #6 of 8 (29 views)
Permalink
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
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
roger.pau at citrix
Mar 30, 2023, 1:11 AM
Post #7 of 8 (29 views)
Permalink
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.
Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode [ In reply to ]
jbeulich at suse
Mar 30, 2023, 1:52 AM
Post #8 of 8 (29 views)
Permalink
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

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal