[PATCH v3 0/3] x86/gfx: early boot improvements

[PATCH v3 0/3] x86/gfx: early boot improvements

[Roger Pau Monne]

Hello,

The following series contains some fixes and improvements related to
graphics usage when booting Xen.

Proposed patches fix some shortcomings when using multiboot2, like the
ignoring of the vga= parameter and forcefully switching the console to
the maximum supported resolution.

Thanks, Roger.

Roger Pau Monne (3):
  multiboot2: parse vga= option when setting GOP mode
  multiboot2: do not set StdOut mode unconditionally
  cmdline: parse multiple instances of the vga option

 docs/misc/xen-command-line.pandoc |  3 ++
 xen/arch/x86/boot/cmdline.c       | 85 +++++++++++++++----------------
 xen/arch/x86/boot/head.S          | 13 ++++-
 xen/arch/x86/efi/efi-boot.h       | 69 +++++++++++++++++++++++--
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 5 files changed, 123 insertions(+), 48 deletions(-)

-- 
2.40.0

[PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Roger Pau Monne]

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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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       | 61 ++++++++++++++++++++++++++++++-
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 09bebf8635d0..aa443574d26f 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -226,9 +226,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
@@ -266,6 +267,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
@@ -329,7 +337,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 c94e53d139a3..003ef037bf07 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -786,7 +786,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 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) == ' ' )
+        {
+            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;
@@ -807,7 +830,41 @@ 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 *last = cmdline;
+        unsigned int width = 0, height = 0, depth = 0;
+        bool keep_current = false;
+
+        while ( (last = get_option(last, "vga=")) != NULL )
+        {
+            if ( !strncmp(last, "gfx-", 4) )
+            {
+                width = simple_strtoul(last + 4, &last, 10);
+                if ( *last == 'x' )
+                    height = simple_strtoul(last + 1, &last, 10);
+                if ( *last == 'x' )
+                    depth = simple_strtoul(last + 1, &last, 10);
+                if ( *last != ' ' && *last != '\t' && *last != '\0' &&
+                     *last != ',' )
+                    width = height = depth = 0;
+                keep_current = false;
+            }
+            else if ( !strncmp(last, "current", 7) )
+                keep_current = true;
+            else if ( !strncmp(last, "keep", 4) )
+            {
+                /* Ignore. */
+            }
+            else
+            {
+                /* Fallback to defaults if unimplemented. */
+                width = height = depth = 0;
+                keep_current = false;
+                PrintStr(L"Warning: Cannot use selected vga option.\r\n");
+            }
+        }
+
+        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);
-- 
2.40.0

[PATCH v3 2/3] multiboot2: do not set StdOut mode unconditionally

[Roger Pau Monne]

Only initialize StdOut if the current StdOut mode is unusable.  This
avoids forcefully switching StdOut to the maximum supported
resolution, and thus very likely changing the GOP mode without having
first parsed the command line options.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The code is very similar to the approach suggested by Jan, please let
me know if you would be OK with your suggested-by tag added.
---
Changes since v2:
 - Use approach suggested by Jan.

Changes since v1:
 - New in this version.
---
 xen/arch/x86/efi/efi-boot.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 003ef037bf07..5314f4293b12 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -820,7 +820,13 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
 
     efi_init(ImageHandle, SystemTable);
 
-    efi_console_set_mode();
+    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
+                           &cols, &rows) != EFI_SUCCESS )
+        /*
+         * If active StdOut mode is invalid init ConOut (StdOut) to the max
+         * supported size.
+         */
+        efi_console_set_mode();
 
     if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
                            &cols, &rows) == EFI_SUCCESS )
-- 
2.40.0

[PATCH v3 3/3] cmdline: parse multiple instances of the vga option

[Roger Pau Monne]

Parse all instances of the vga= option on the command line, in order
to always enforce the last selection on the command line.

Note that it's not safe to parse just the last occurrence of the vga=
option, as then a command line with `vga=current vga=keep` would
result in current being ignored.

Adjust the command line documentation to describe the new behavior.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
Build tested only, as I don't have a system that does legacy boot and
has VGA output I can check.

It's mostly encapsulating the current code inside of a while loop and
adding an extra else if for the "ask" option, there's a lot of
indentation changes.
---
 docs/misc/xen-command-line.pandoc |  3 ++
 xen/arch/x86/boot/cmdline.c       | 85 +++++++++++++++----------------
 2 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index e0b89b7d3319..8cf2f3423d47 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2628,6 +2628,9 @@ with the specified width, height and depth.
 `ask` option.  (N.B menu modes are displayed in hex, so `<mode>`
 should be a hexadecimal number)
 
+Note that all the occurrences of the vga option in the command line are parsed,
+and hence later occurrences can overwrite selections done by prior ones.
+
 The optional `keep` parameter causes Xen to continue using the vga
 console even after dom0 has been started.  The default behaviour is to
 relinquish control to dom0.
diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
index fc11c6d3c5c4..511e77e0c2b5 100644
--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -277,59 +277,58 @@ static u16 rows2vmode(unsigned int rows)
 
 static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
 {
-    const char *c;
-    unsigned int tmp, vesa_depth, vesa_height, vesa_width;
-
-    c = find_opt(cmdline, "vga=", true);
-
-    if ( !c )
-        return;
+    const char *c = cmdline;
 
-    ebo->boot_vid_mode = ASK_VGA;
-
-    if ( !strmaxcmp(c, "current", delim_chars_comma) )
-        ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
-    else if ( !strsubcmp(c, "text-80x") )
-    {
-        c += strlen("text-80x");
-        ebo->boot_vid_mode = rows2vmode(strtoui(c, delim_chars_comma, NULL));
-    }
-    else if ( !strsubcmp(c, "gfx-") )
+    while ( (c = find_opt(c, "vga=", true)) != NULL )
     {
-        vesa_width = strtoui(c + strlen("gfx-"), "x", &c);
+        unsigned int tmp, vesa_depth, vesa_height, vesa_width;
 
-        if ( vesa_width > U16_MAX )
-            return;
+        if ( !strmaxcmp(c, "current", delim_chars_comma) )
+            ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
+        else if ( !strsubcmp(c, "text-80x") )
+        {
+            c += strlen("text-80x");
+            ebo->boot_vid_mode = rows2vmode(strtoui(c, delim_chars_comma, NULL));
+        }
+        else if ( !strsubcmp(c, "gfx-") )
+        {
+            vesa_width = strtoui(c + strlen("gfx-"), "x", &c);
 
-        /*
-         * Increment c outside of strtoui() because otherwise some
-         * compiler may complain with following message:
-         * warning: operation on 'c' may be undefined.
-         */
-        ++c;
-        vesa_height = strtoui(c, "x", &c);
+            if ( vesa_width > U16_MAX )
+                return;
 
-        if ( vesa_height > U16_MAX )
-            return;
+            /*
+             * Increment c outside of strtoui() because otherwise some
+             * compiler may complain with following message:
+             * warning: operation on 'c' may be undefined.
+             */
+            ++c;
+            vesa_height = strtoui(c, "x", &c);
 
-        vesa_depth = strtoui(++c, delim_chars_comma, NULL);
+            if ( vesa_height > U16_MAX )
+                return;
 
-        if ( vesa_depth > U16_MAX )
-            return;
+            vesa_depth = strtoui(++c, delim_chars_comma, NULL);
 
-        ebo->vesa_width = vesa_width;
-        ebo->vesa_height = vesa_height;
-        ebo->vesa_depth = vesa_depth;
-        ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;
-    }
-    else if ( !strsubcmp(c, "mode-") )
-    {
-        tmp = strtoui(c + strlen("mode-"), delim_chars_comma, NULL);
+            if ( vesa_depth > U16_MAX )
+                return;
 
-        if ( tmp > U16_MAX )
-            return;
+            ebo->vesa_width = vesa_width;
+            ebo->vesa_height = vesa_height;
+            ebo->vesa_depth = vesa_depth;
+            ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;
+        }
+        else if ( !strsubcmp(c, "mode-") )
+        {
+            tmp = strtoui(c + strlen("mode-"), delim_chars_comma, NULL);
 
-        ebo->boot_vid_mode = tmp;
+            if ( tmp > U16_MAX )
+                return;
+
+            ebo->boot_vid_mode = tmp;
+        }
+        else if ( !strsubcmp(c, "ask") )
+            ebo->boot_vid_mode = ASK_VGA;
     }
 }
 #endif
-- 
2.40.0

Re: [PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Jan Beulich]

On 01.06.2023 15:05, Roger Pau Monne wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -226,9 +226,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

While perhaps better to leave this as you have it, ...

> @@ -266,6 +267,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:

... in new blocks of code I think it would be nice if commas were
followed by visually separating blanks.

> @@ -329,7 +337,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

All you obtain is a pointer to the string, which is fine from what I
was able to establish, but that's not entirely obvious: The MBI
structure used has a size field, so it could have been that the
string isn't nul-terminated, and hence the size would also need
passing. May I ask that this be mentioned at least in the description?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -786,7 +786,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 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) == ' ' )

Iirc I had asked before: Not allowing for at least tab? (See
cmdline.c:delim_chars_comma[] for what the non-EFI parsing permits,
which in turn might be going a little too far especially with
permitting comma as well.)

> +        {
> +            o = s + strlen(opt);

I don't think the comment ahead of the function describes this
behavior, i.e. in particular the adding of the length of the
option.

> @@ -807,7 +830,41 @@ 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 *last = cmdline;

Nit: Maybe better "cur" than "last"?

> +        unsigned int width = 0, height = 0, depth = 0;
> +        bool keep_current = false;
> +
> +        while ( (last = get_option(last, "vga=")) != NULL )
> +        {
> +            if ( !strncmp(last, "gfx-", 4) )
> +            {
> +                width = simple_strtoul(last + 4, &last, 10);
> +                if ( *last == 'x' )
> +                    height = simple_strtoul(last + 1, &last, 10);
> +                if ( *last == 'x' )
> +                    depth = simple_strtoul(last + 1, &last, 10);
> +                if ( *last != ' ' && *last != '\t' && *last != '\0' &&
> +                     *last != ',' )
> +                    width = height = depth = 0;
> +                keep_current = false;
> +            }
> +            else if ( !strncmp(last, "current", 7) )
> +                keep_current = true;
> +            else if ( !strncmp(last, "keep", 4) )
> +            {
> +                /* Ignore. */

Maybe "Ignore here, handled in ..."?

Jan

Re: [PATCH v3 2/3] multiboot2: do not set StdOut mode unconditionally

[Jan Beulich]

On 01.06.2023 15:05, Roger Pau Monne wrote:
> Only initialize StdOut if the current StdOut mode is unusable.  This
> avoids forcefully switching StdOut to the maximum supported
> resolution, and thus very likely changing the GOP mode without having
> first parsed the command line options.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> The code is very similar to the approach suggested by Jan, please let
> me know if you would be OK with your suggested-by tag added.

I'm okay either way; I only suggested the "how", not the "that" after all.
In any event
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v3 3/3] cmdline: parse multiple instances of the vga option

[Jan Beulich]

On 01.06.2023 15:05, Roger Pau Monne wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2628,6 +2628,9 @@ with the specified width, height and depth.
>  `ask` option.  (N.B menu modes are displayed in hex, so `<mode>`
>  should be a hexadecimal number)
>  
> +Note that all the occurrences of the vga option in the command line are parsed,
> +and hence later occurrences can overwrite selections done by prior ones.

I'm not a native speaker, but is it perhaps more "override" that you
mean?

> --- a/xen/arch/x86/boot/cmdline.c
> +++ b/xen/arch/x86/boot/cmdline.c
> @@ -277,59 +277,58 @@ static u16 rows2vmode(unsigned int rows)
>  
>  static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
>  {
> -    const char *c;
> -    unsigned int tmp, vesa_depth, vesa_height, vesa_width;
> -
> -    c = find_opt(cmdline, "vga=", true);
> -
> -    if ( !c )
> -        return;
> +    const char *c = cmdline;
>  
> -    ebo->boot_vid_mode = ASK_VGA;

I think this needs to stay here along with the addition of the related
"else if" below. Otherwise I expect behavior for e.g. a sole "vga=keep"
on the command line would change (in no longer prompting for the mode
to use).

Jan

Re: [PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Jan Beulich]

On 01.06.2023 15:05, Roger Pau Monne wrote:
> @@ -807,7 +830,41 @@ 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 *last = cmdline;
> +        unsigned int width = 0, height = 0, depth = 0;
> +        bool keep_current = false;
> +
> +        while ( (last = get_option(last, "vga=")) != NULL )
> +        {
> +            if ( !strncmp(last, "gfx-", 4) )
> +            {
> +                width = simple_strtoul(last + 4, &last, 10);
> +                if ( *last == 'x' )
> +                    height = simple_strtoul(last + 1, &last, 10);
> +                if ( *last == 'x' )
> +                    depth = simple_strtoul(last + 1, &last, 10);
> +                if ( *last != ' ' && *last != '\t' && *last != '\0' &&
> +                     *last != ',' )

You check for an appropriate terminator here.

> +                    width = height = depth = 0;
> +                keep_current = false;
> +            }
> +            else if ( !strncmp(last, "current", 7) )

Don't you also need to do so here, and maybe even ...

> +                keep_current = true;
> +            else if ( !strncmp(last, "keep", 4) )
> +            {
> +                /* Ignore. */

... here?

Jan

Re: [PATCH v3 3/3] cmdline: parse multiple instances of the vga option

[Andrew Cooper]

On 07/06/2023 11:07 am, Jan Beulich wrote:
> On 01.06.2023 15:05, Roger Pau Monne wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2628,6 +2628,9 @@ with the specified width, height and depth.
>>  `ask` option.  (N.B menu modes are displayed in hex, so `<mode>`
>>  should be a hexadecimal number)
>>  
>> +Note that all the occurrences of the vga option in the command line are parsed,
>> +and hence later occurrences can overwrite selections done by prior ones.
> I'm not a native speaker, but is it perhaps more "override" that you
> mean?

I don't think this is appropriate to say in the cmdline docs.  Absent
bugs, all command line options get parsed left->right with
latest-takes-precedence.

It is a bug that vga= didn't behave like this.

It might be ok to say "Note, prior to Xen 4.18, only the first instance
of vga= got parsed, with subsequent instances getting ignored".

Explicitly calling out the default behaviour in one option does nothing
but confuse things.

~Andrew

Re: [PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Roger Pau Monné]

On Wed, Jun 07, 2023 at 11:41:24AM +0200, Jan Beulich wrote:
> On 01.06.2023 15:05, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -329,7 +337,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
> 
> All you obtain is a pointer to the string, which is fine from what I
> was able to establish, but that's not entirely obvious: The MBI
> structure used has a size field, so it could have been that the
> string isn't nul-terminated, and hence the size would also need
> passing. May I ask that this be mentioned at least in the description?

Sure.  The multiboot2 specification already states that the string
must be zero terminated.  It's my understanding the size field is part
of all the tags, so that a parser can find the next tag even if it
doesn't know how to parse the current one.

> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -786,7 +786,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 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) == ' ' )
> 
> Iirc I had asked before: Not allowing for at least tab? (See
> cmdline.c:delim_chars_comma[] for what the non-EFI parsing permits,
> which in turn might be going a little too far especially with
> permitting comma as well.)

I've fixed this when parsing the gfx- option but not here, sorry.

> 
> > +        {
> > +            o = s + strlen(opt);
> 
> I don't think the comment ahead of the function describes this
> behavior, i.e. in particular the adding of the length of the
> option.

I've changed the comment to:

Return a pointer to the character after the first occurrence of opt in cmd.

Thanks, Roger.

Re: [PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Roger Pau Monné]

On Wed, Jun 07, 2023 at 12:10:28PM +0200, Jan Beulich wrote:
> On 01.06.2023 15:05, Roger Pau Monne wrote:
> > @@ -807,7 +830,41 @@ 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 *last = cmdline;
> > +        unsigned int width = 0, height = 0, depth = 0;
> > +        bool keep_current = false;
> > +
> > +        while ( (last = get_option(last, "vga=")) != NULL )
> > +        {
> > +            if ( !strncmp(last, "gfx-", 4) )
> > +            {
> > +                width = simple_strtoul(last + 4, &last, 10);
> > +                if ( *last == 'x' )
> > +                    height = simple_strtoul(last + 1, &last, 10);
> > +                if ( *last == 'x' )
> > +                    depth = simple_strtoul(last + 1, &last, 10);
> > +                if ( *last != ' ' && *last != '\t' && *last != '\0' &&
> > +                     *last != ',' )
> 
> You check for an appropriate terminator here.
> 
> > +                    width = height = depth = 0;
> > +                keep_current = false;
> > +            }
> > +            else if ( !strncmp(last, "current", 7) )
> 
> Don't you also need to do so here, and maybe even ...
> 
> > +                keep_current = true;
> > +            else if ( !strncmp(last, "keep", 4) )
> > +            {
> > +                /* Ignore. */
> 
> ... here?

Hm, quite possibly for correctness.  I felt it was relevant in gfx- as
to avoid things like: gfx-1024x786x32x64 being handled, but the same
could apply to passing on option like current-bar.

Will try to generalize the terminator parsing so it applies to all
options.

Thanks, Roger.

Re: [PATCH v3 3/3] cmdline: parse multiple instances of the vga option

[Roger Pau Monné]

On Wed, Jun 07, 2023 at 12:07:54PM +0200, Jan Beulich wrote:
> On 01.06.2023 15:05, Roger Pau Monne wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -2628,6 +2628,9 @@ with the specified width, height and depth.
> >  `ask` option.  (N.B menu modes are displayed in hex, so `<mode>`
> >  should be a hexadecimal number)
> >  
> > +Note that all the occurrences of the vga option in the command line are parsed,
> > +and hence later occurrences can overwrite selections done by prior ones.
> 
> I'm not a native speaker, but is it perhaps more "override" that you
> mean?

TBH I always get confused with overwrite vs override.  I will remove
the line as requested by Andrew.

> > --- a/xen/arch/x86/boot/cmdline.c
> > +++ b/xen/arch/x86/boot/cmdline.c
> > @@ -277,59 +277,58 @@ static u16 rows2vmode(unsigned int rows)
> >  
> >  static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
> >  {
> > -    const char *c;
> > -    unsigned int tmp, vesa_depth, vesa_height, vesa_width;
> > -
> > -    c = find_opt(cmdline, "vga=", true);
> > -
> > -    if ( !c )
> > -        return;
> > +    const char *c = cmdline;
> >  
> > -    ebo->boot_vid_mode = ASK_VGA;
> 
> I think this needs to stay here along with the addition of the related
> "else if" below. Otherwise I expect behavior for e.g. a sole "vga=keep"
> on the command line would change (in no longer prompting for the mode
> to use).

Done.

Thanks, Roger.