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

[PATCH v4 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

 xen/arch/x86/boot/cmdline.c       | 83 ++++++++++++++++---------------
 xen/arch/x86/boot/head.S          | 13 ++++-
 xen/arch/x86/efi/efi-boot.h       | 82 ++++++++++++++++++++++++++++--
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 4 files changed, 133 insertions(+), 46 deletions(-)

-- 
2.41.0

[PATCH v4 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.

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(-)

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);
-- 
2.41.0

[PATCH v4 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
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 4394c7276aa3..a13a335d24a9 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -829,7 +829,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.41.0

[PATCH v4 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 v3:
 - Remove xen-command-line doc addition.
 - Set mode to 'ask' by default.

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.
---
 xen/arch/x86/boot/cmdline.c | 83 +++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
index fc11c6d3c5c4..10dcc6142c85 100644
--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -277,59 +277,60 @@ 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 ( !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);
 
-        if ( vesa_width > U16_MAX )
-            return;
+            if ( vesa_width > 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);
+            /*
+             * 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_height > U16_MAX )
-            return;
+            if ( vesa_height > U16_MAX )
+                return;
 
-        vesa_depth = strtoui(++c, delim_chars_comma, NULL);
+            vesa_depth = strtoui(++c, delim_chars_comma, NULL);
 
-        if ( vesa_depth > U16_MAX )
-            return;
+            if ( vesa_depth > 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->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 ( tmp > U16_MAX )
-            return;
+            if ( tmp > U16_MAX )
+                return;
 
-        ebo->boot_vid_mode = tmp;
+            ebo->boot_vid_mode = tmp;
+        }
+        else if ( !strmaxcmp(c, "ask", delim_chars_comma) )
+            ebo->boot_vid_mode = ASK_VGA;
     }
 }
 #endif
-- 
2.41.0

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

[Jan Beulich]

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

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

[Jan Beulich]

On 05.07.2023 13:47, Roger Pau Monne wrote:
> 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.

This was likely meant to be dropped along with ...

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v3:
>  - Remove xen-command-line doc addition.

... this? Beyond that (easy to adjust while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Roger Pau Monné]

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.

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

[Roger Pau Monné]

On Thu, Jul 06, 2023 at 12:45:41PM +0200, Jan Beulich wrote:
> On 05.07.2023 13:47, Roger Pau Monne wrote:
> > 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.
> 
> This was likely meant to be dropped along with ...
> 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v3:
> >  - Remove xen-command-line doc addition.
> 
> ... this? Beyond that (easy to adjust while committing)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Oh, indeed, I've added the changelog and didn't adjust the commit
message.

Thanks, Roger.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.