[PATCH v2 0/3] gfx: improvements when using multiboot2 and EFI

[PATCH v2 0/3] gfx: improvements when using multiboot2 and EFI

[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, mostly the
ignoring of the console=vga (or lack of) and the vga=gfx- parameters.
It also switches default Xen behaviour from trying to reuse the
currently set GOP mode instead of attempting to set the maximum
supported resolution.

Thanks, Roger.

Roger Pau Monne (3):
  efi: try to use the currently set GOP mode
  multiboot2: parse console= and vga= options when setting GOP mode
  multiboot2: do not set StdOut mode unconditionally

 xen/arch/x86/boot/head.S          | 13 ++++-
 xen/arch/x86/efi/efi-boot.h       | 95 ++++++++++++++++++++++++++++---
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 xen/common/efi/boot.c             | 21 +++++++
 4 files changed, 120 insertions(+), 10 deletions(-)

-- 
2.40.0

[PATCH v2 1/3] efi: try to use the currently set GOP mode

[Roger Pau Monne]

Modify efi_find_gop_mode() so that passing cols or rows as 0 is
interpreted as a request to attempt to keep the currently set mode,
and do so if the mode query for information is successful and the depth
is supported.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Only update cols or rows if the value is 0.
 - Leave depth alone.
---
 xen/common/efi/boot.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b69c83e354..766a2553be 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -930,6 +930,27 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
     UINTN gop_mode = ~0, info_size, size;
     unsigned int i;
 
+    if ( (!cols || !rows) && gop->Mode->Mode < gop->Mode->MaxMode )
+    {
+        /* If no (valid) resolution suggested, try to use the current mode. */
+        status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
+        if ( EFI_ERROR(status) )
+            PrintErr(L"Invalid current graphics mode\r\n");
+        else if ( mode_info->PixelFormat < PixelBltOnly )
+            return gop->Mode->Mode;
+        else
+        {
+            /*
+             * Try to find a mode with the same resolution and a valid pixel
+             * format.
+             */
+            if ( !cols )
+                cols = mode_info->HorizontalResolution;
+            if ( !rows )
+                rows = mode_info->VerticalResolution;
+        }
+    }
+
     for ( i = size = 0; i < gop->Mode->MaxMode; ++i )
     {
         unsigned int bpp = 0;
-- 
2.40.0

Re: [PATCH v2 1/3] efi: try to use the currently set GOP mode

[Jan Beulich]

On 31.03.2023 11:59, Roger Pau Monne wrote:
> Modify efi_find_gop_mode() so that passing cols or rows as 0 is
> interpreted as a request to attempt to keep the currently set mode,
> and do so if the mode query for information is successful and the depth
> is supported.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Only update cols or rows if the value is 0.
>  - Leave depth alone.
> ---
>  xen/common/efi/boot.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

While I realize that docs/misc/efi.pandoc currently doesn't describe
what rows/cols being zero means, I think that file needs updating in
the course of what you're doing. Irrespective of this I'm uncertain
about the change in behavior: Presently both values being 0 means
"find biggest resolution mode", in an attempt to have as much info on
the screen at a time as possible (in particular in case of problems).
I certainly appreciate the desire to have a way to say "keep the
current mode", but I don't think this should alter behavior of what's
presently considered valid config settings.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -930,6 +930,27 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>      UINTN gop_mode = ~0, info_size, size;
>      unsigned int i;
>  
> +    if ( (!cols || !rows) && gop->Mode->Mode < gop->Mode->MaxMode )
> +    {
> +        /* If no (valid) resolution suggested, try to use the current mode. */
> +        status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
> +        if ( EFI_ERROR(status) )
> +            PrintErr(L"Invalid current graphics mode\r\n");
> +        else if ( mode_info->PixelFormat < PixelBltOnly )
> +            return gop->Mode->Mode;

What if one of cols/rows was non-zero? You then wouldn't fulfill the request.
"depth", if non-zero, is also entirely ignored. (We don't fulfill such a
request right now either, but not doing so becomes more odd now imo. In fact
right now in this case we leave the screen alone, if I'm not mistaken, so
there already looks to be a way to achieve what you're after.)

> +        else
> +        {
> +            /*
> +             * Try to find a mode with the same resolution and a valid pixel
> +             * format.
> +             */

Is "valid" the right word here? I think you mean "usable for us" or some
such?

> +            if ( !cols )
> +                cols = mode_info->HorizontalResolution;
> +            if ( !rows )
> +                rows = mode_info->VerticalResolution;
> +        }
> +    }

Overall with the resulting behavior I'm not sure the title really describes
what's being done. You "try" only in certain cases.

Jan

[PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Roger Pau Monne]

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= and vga= options 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 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       | 80 ++++++++++++++++++++++++++++++-
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 3 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0fb7dd3029..9d13eee50b 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
@@ -261,6 +262,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
@@ -324,7 +332,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 c94e53d139..f46c1f021e 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 next 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,60 @@ 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 = NULL, *last = cmdline;
+        /* Default console selection is "com1,vga". */
+        bool vga = true;
+
+        /* For the console option the last occurrence is the enforced one. */
+        while ( (last = get_option(last, "console=")) != NULL )
+            opt = last;
+
+        if ( opt )
+        {
+            const char *s = strstr(opt, "vga");
+
+            if ( !s || s > strpbrk(opt, " ") )
+                vga = false;
+        }
+
+        if ( vga )
+        {
+            unsigned int width = 0, height = 0, depth = 0;
+            bool keep_current = false;
+
+            last = cmdline;
+            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);
+                    /* Allow depth to be 0 or unset. */
+                    if ( !width || !height )
+                        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 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.40.0

Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Jan Beulich]

On 31.03.2023 11:59, Roger Pau Monne wrote:
> Only set the GOP mode if vga is selected in the console option,

This particular aspect of the behavior is inconsistent with legacy
boot behavior: There "vga=" isn't qualified by what "console=" has.

> 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= and vga= options 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>
>[...] 
> --- 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 next 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 )

I can see why you need to check "cmd", but there's no need to check "opt"
I would say.

> @@ -807,7 +830,60 @@ 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 = NULL, *last = cmdline;
> +        /* Default console selection is "com1,vga". */
> +        bool vga = true;
> +
> +        /* For the console option the last occurrence is the enforced one. */
> +        while ( (last = get_option(last, "console=")) != NULL )
> +            opt = last;
> +
> +        if ( opt )
> +        {
> +            const char *s = strstr(opt, "vga");
> +
> +            if ( !s || s > strpbrk(opt, " ") )

Why strpbrk() and not the simpler strchr()? Or did you mean to also look
for tabs, but then didn't include \t here (and in get_option())? (Legacy
boot code also takes \r and \n as separators, btw, but I'm unconvinced
of the need.)

Also aiui this is UB when the function returns NULL, as relational operators
(excluding equality ones) may only be applied when both addresses refer to
the same object (or to the end of an involved array).

> +                vga = false;
> +        }
> +
> +        if ( vga )
> +        {
> +            unsigned int width = 0, height = 0, depth = 0;
> +            bool keep_current = false;
> +
> +            last = cmdline;
> +            while ( (last = get_option(last, "vga=")) != NULL )

It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c)
finds the first instance only. Normal command line handling respects the
last instance only. So while "vga=gfx-... vga=keep" will have the expected
effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be
less inflexible here, but I think this then wants accompanying by an update
to the command line doc, no matter that presently it doesn't really
describe these peculiarities. Otoh it would end up being slightly cheaper
to only look for the first instance here as well. In particular ...

> +            {
> +                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);
> +                    /* Allow depth to be 0 or unset. */
> +                    if ( !width || !height )
> +                        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;

... this zapping of what was successfully parsed before would then not be
needed in any event (else I would question why this is necessary).

Jan

Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Roger Pau Monné]

On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote:
> On 31.03.2023 11:59, Roger Pau Monne wrote:
> > Only set the GOP mode if vga is selected in the console option,
> 
> This particular aspect of the behavior is inconsistent with legacy
> boot behavior: There "vga=" isn't qualified by what "console=" has.

Hm, I find this very odd, why would we fiddle with the VGA (or the GOP
here) if we don't intend to use it for output?

> > 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= and vga= options 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>
> >[...] 
> > --- 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 next 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 )
> 
> I can see why you need to check "cmd", but there's no need to check "opt"
> I would say.

Given this is executed without a page-fault handler in place I thought
it was best to be safe rather than sorry, and avoid any pointer
dereferences.

> > @@ -807,7 +830,60 @@ 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 = NULL, *last = cmdline;
> > +        /* Default console selection is "com1,vga". */
> > +        bool vga = true;
> > +
> > +        /* For the console option the last occurrence is the enforced one. */
> > +        while ( (last = get_option(last, "console=")) != NULL )
> > +            opt = last;
> > +
> > +        if ( opt )
> > +        {
> > +            const char *s = strstr(opt, "vga");
> > +
> > +            if ( !s || s > strpbrk(opt, " ") )
> 
> Why strpbrk() and not the simpler strchr()? Or did you mean to also look
> for tabs, but then didn't include \t here (and in get_option())? (Legacy
> boot code also takes \r and \n as separators, btw, but I'm unconvinced
> of the need.)

I was originally checking for more characters here and didn't switch
when removing those.  I will add \t.

> Also aiui this is UB when the function returns NULL, as relational operators
> (excluding equality ones) may only be applied when both addresses refer to
> the same object (or to the end of an involved array).

Hm, I see, thanks for spotting. So I would need to do:

s > (strpbrk(opt, " ") ?: s)

So that we don't compare against NULL.

Also the original code was wrong AFAICT, as strpbrk() returning NULL
should result in vga=true (as it would imply console= is the last
option on the command line).

> > +                vga = false;
> > +        }
> > +
> > +        if ( vga )
> > +        {
> > +            unsigned int width = 0, height = 0, depth = 0;
> > +            bool keep_current = false;
> > +
> > +            last = cmdline;
> > +            while ( (last = get_option(last, "vga=")) != NULL )
> 
> It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c)
> finds the first instance only. Normal command line handling respects the
> last instance only. So while "vga=gfx-... vga=keep" will have the expected
> effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be
> less inflexible here, but I think this then wants accompanying by an update
> to the command line doc, no matter that presently it doesn't really
> describe these peculiarities.

But if we then describe this behavior in the documentation people
could rely on it.  Right now this is just an implementation detail (or
a bug I would say), and that would justify fixing boot/cmdline.c to
also respect the last instance only.

> Otoh it would end up being slightly cheaper
> to only look for the first instance here as well. In particular ...
> 
> > +            {
> > +                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);
> > +                    /* Allow depth to be 0 or unset. */
> > +                    if ( !width || !height )
> > +                        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;
> 
> ... this zapping of what was successfully parsed before would then not be
> needed in any event (else I would question why this is necessary).

Hm, I don't have a strong opinion, the expected behavior I have of
command line options is that the last one is the one that takes
effect, but it would simplify the change if we only cared about the
first one, albeit that's an odd behavior.

My preference would be to leave the code here respecting the last
instance only, and attempt to fix boot/cmdline.c so it does the same.

Thanks, Roger.

Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Jan Beulich]

On 30.05.2023 18:02, Roger Pau Monné wrote:
> On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote:
>> On 31.03.2023 11:59, Roger Pau Monne wrote:
>>> Only set the GOP mode if vga is selected in the console option,
>>
>> This particular aspect of the behavior is inconsistent with legacy
>> boot behavior: There "vga=" isn't qualified by what "console=" has.
> 
> Hm, I find this very odd, why would we fiddle with the VGA (or the GOP
> here) if we don't intend to use it for output?

Because we also need to arrange for what Dom0 possibly wants to use.
It has no way of setting the mode the low-level (BIOS or EFI) way.

>>> 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= and vga= options 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>
>>> [...] 
>>> --- 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 next 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 )
>>
>> I can see why you need to check "cmd", but there's no need to check "opt"
>> I would say.
> 
> Given this is executed without a page-fault handler in place I thought
> it was best to be safe rather than sorry, and avoid any pointer
> dereferences.

Hmm, I see. We don't do so elsewhere, though, I think.

>>> @@ -807,7 +830,60 @@ 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 = NULL, *last = cmdline;
>>> +        /* Default console selection is "com1,vga". */
>>> +        bool vga = true;
>>> +
>>> +        /* For the console option the last occurrence is the enforced one. */
>>> +        while ( (last = get_option(last, "console=")) != NULL )
>>> +            opt = last;
>>> +
>>> +        if ( opt )
>>> +        {
>>> +            const char *s = strstr(opt, "vga");
>>> +
>>> +            if ( !s || s > strpbrk(opt, " ") )
>>
>> Why strpbrk() and not the simpler strchr()? Or did you mean to also look
>> for tabs, but then didn't include \t here (and in get_option())? (Legacy
>> boot code also takes \r and \n as separators, btw, but I'm unconvinced
>> of the need.)
> 
> I was originally checking for more characters here and didn't switch
> when removing those.  I will add \t.
> 
>> Also aiui this is UB when the function returns NULL, as relational operators
>> (excluding equality ones) may only be applied when both addresses refer to
>> the same object (or to the end of an involved array).
> 
> Hm, I see, thanks for spotting. So I would need to do:
> 
> s > (strpbrk(opt, " ") ?: s)
> 
> So that we don't compare against NULL.
> 
> Also the original code was wrong AFAICT, as strpbrk() returning NULL
> should result in vga=true (as it would imply console= is the last
> option on the command line).

I'm afraid I'm in trouble what "original code" you're referring to here.
Iirc you really only add code to the function. And boot/cmdline.c has no
use of strpbrk() afaics.

>>> +                vga = false;
>>> +        }
>>> +
>>> +        if ( vga )
>>> +        {
>>> +            unsigned int width = 0, height = 0, depth = 0;
>>> +            bool keep_current = false;
>>> +
>>> +            last = cmdline;
>>> +            while ( (last = get_option(last, "vga=")) != NULL )
>>
>> It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c)
>> finds the first instance only. Normal command line handling respects the
>> last instance only. So while "vga=gfx-... vga=keep" will have the expected
>> effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be
>> less inflexible here, but I think this then wants accompanying by an update
>> to the command line doc, no matter that presently it doesn't really
>> describe these peculiarities.
> 
> But if we then describe this behavior in the documentation people
> could rely on it.  Right now this is just an implementation detail (or
> a bug I would say), and that would justify fixing boot/cmdline.c to
> also respect the last instance only.

Yes, fixing the non-EFI code is certainly an option (and then describing
the hopefully consistent result in the doc).

Jan

>> Otoh it would end up being slightly cheaper
>> to only look for the first instance here as well. In particular ...
>>
>>> +            {
>>> +                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);
>>> +                    /* Allow depth to be 0 or unset. */
>>> +                    if ( !width || !height )
>>> +                        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;
>>
>> ... this zapping of what was successfully parsed before would then not be
>> needed in any event (else I would question why this is necessary).
> 
> Hm, I don't have a strong opinion, the expected behavior I have of
> command line options is that the last one is the one that takes
> effect, but it would simplify the change if we only cared about the
> first one, albeit that's an odd behavior.
> 
> My preference would be to leave the code here respecting the last
> instance only, and attempt to fix boot/cmdline.c so it does the same.
> 
> Thanks, Roger.

Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Roger Pau Monné]

On Wed, May 31, 2023 at 11:15:44AM +0200, Jan Beulich wrote:
> On 30.05.2023 18:02, Roger Pau Monné wrote:
> > On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote:
> >> On 31.03.2023 11:59, Roger Pau Monne wrote:
> >>> Only set the GOP mode if vga is selected in the console option,
> >>
> >> This particular aspect of the behavior is inconsistent with legacy
> >> boot behavior: There "vga=" isn't qualified by what "console=" has.
> > 
> > Hm, I find this very odd, why would we fiddle with the VGA (or the GOP
> > here) if we don't intend to use it for output?
> 
> Because we also need to arrange for what Dom0 possibly wants to use.
> It has no way of setting the mode the low-level (BIOS or EFI) way.

I understand this might be needed when Xen is booted as an EFI
application, but otherwise it should be the bootloader that takes care
of setting such mode, as (most?) OSes are normally loaded with boot
services already exited.

I've removed the parsing of the console= option and unconditionally
parse vga= now.  We can always adjust later.

> >>> 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= and vga= options 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>
> >>> [...] 
> >>> --- 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 next 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 )
> >>
> >> I can see why you need to check "cmd", but there's no need to check "opt"
> >> I would say.
> > 
> > Given this is executed without a page-fault handler in place I thought
> > it was best to be safe rather than sorry, and avoid any pointer
> > dereferences.
> 
> Hmm, I see. We don't do so elsewhere, though, I think.

If you insist I can remove it, otherwise I will just leave as-is.

> 
> >>> @@ -807,7 +830,60 @@ 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 = NULL, *last = cmdline;
> >>> +        /* Default console selection is "com1,vga". */
> >>> +        bool vga = true;
> >>> +
> >>> +        /* For the console option the last occurrence is the enforced one. */
> >>> +        while ( (last = get_option(last, "console=")) != NULL )
> >>> +            opt = last;
> >>> +
> >>> +        if ( opt )
> >>> +        {
> >>> +            const char *s = strstr(opt, "vga");
> >>> +
> >>> +            if ( !s || s > strpbrk(opt, " ") )
> >>
> >> Why strpbrk() and not the simpler strchr()? Or did you mean to also look
> >> for tabs, but then didn't include \t here (and in get_option())? (Legacy
> >> boot code also takes \r and \n as separators, btw, but I'm unconvinced
> >> of the need.)
> > 
> > I was originally checking for more characters here and didn't switch
> > when removing those.  I will add \t.
> > 
> >> Also aiui this is UB when the function returns NULL, as relational operators
> >> (excluding equality ones) may only be applied when both addresses refer to
> >> the same object (or to the end of an involved array).
> > 
> > Hm, I see, thanks for spotting. So I would need to do:
> > 
> > s > (strpbrk(opt, " ") ?: s)
> > 
> > So that we don't compare against NULL.
> > 
> > Also the original code was wrong AFAICT, as strpbrk() returning NULL
> > should result in vga=true (as it would imply console= is the last
> > option on the command line).
> 
> I'm afraid I'm in trouble what "original code" you're referring to here.
> Iirc you really only add code to the function. And boot/cmdline.c has no
> use of strpbrk() afaics.

Original code in the patch, anyway this is now gone because I no
longer parse console=.

> >>> +                vga = false;
> >>> +        }
> >>> +
> >>> +        if ( vga )
> >>> +        {
> >>> +            unsigned int width = 0, height = 0, depth = 0;
> >>> +            bool keep_current = false;
> >>> +
> >>> +            last = cmdline;
> >>> +            while ( (last = get_option(last, "vga=")) != NULL )
> >>
> >> It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c)
> >> finds the first instance only. Normal command line handling respects the
> >> last instance only. So while "vga=gfx-... vga=keep" will have the expected
> >> effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be
> >> less inflexible here, but I think this then wants accompanying by an update
> >> to the command line doc, no matter that presently it doesn't really
> >> describe these peculiarities.
> > 
> > But if we then describe this behavior in the documentation people
> > could rely on it.  Right now this is just an implementation detail (or
> > a bug I would say), and that would justify fixing boot/cmdline.c to
> > also respect the last instance only.
> 
> Yes, fixing the non-EFI code is certainly an option (and then describing
> the hopefully consistent result in the doc).

OK, let me take a look, I expect that should be fairly trivial.

Thanks, Roger.

Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Jan Beulich]

On 31.05.2023 11:30, Roger Pau Monné wrote:
> On Wed, May 31, 2023 at 11:15:44AM +0200, Jan Beulich wrote:
>> On 30.05.2023 18:02, Roger Pau Monné wrote:
>>> On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote:
>>>> On 31.03.2023 11:59, Roger Pau Monne wrote:
>>>>> Only set the GOP mode if vga is selected in the console option,
>>>>
>>>> This particular aspect of the behavior is inconsistent with legacy
>>>> boot behavior: There "vga=" isn't qualified by what "console=" has.
>>>
>>> Hm, I find this very odd, why would we fiddle with the VGA (or the GOP
>>> here) if we don't intend to use it for output?
>>
>> Because we also need to arrange for what Dom0 possibly wants to use.
>> It has no way of setting the mode the low-level (BIOS or EFI) way.
> 
> I understand this might be needed when Xen is booted as an EFI
> application, but otherwise it should be the bootloader that takes care
> of setting such mode, as (most?) OSes are normally loaded with boot
> services already exited.

The bootloader doing this is a quirk imo. In the Linux case this implies
knowing the inner structure of the binary to be booted, to communicate
the necessary information, plus peeking into the kernel's command line.

Furthermore I wasn't referring to the EFI-with-bootloader case, but the
legacy BIOS one plus the (mentioned by you) EFI-application one. Even
the MB2 protocol allows the bootloader to hand over with boot services
not exited yet, iirc, so even in that case Xen would be in the position
of using boot service functions (from efi_multiboot2()).

Jan

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

[Roger Pau Monne]

Only initialize StdOut if there's no GOP available.  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>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/efi/efi-boot.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f46c1f021e..42aa135446 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -820,12 +820,6 @@ 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 )
-        efi_arch_console_init(cols, rows);
-
     gop = efi_get_gop(&gop_handle);
 
     if ( gop )
@@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
 
         efi_arch_edid(gop_handle);
     }
+    else
+    {
+        /* If no GOP, init ConOut (StdOut) to the max supported size. */
+        efi_console_set_mode();
+
+        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
+                               &cols, &rows) == EFI_SUCCESS )
+            efi_arch_console_init(cols, rows);
+    }
 
     efi_arch_edd();
     efi_arch_cpu();
-- 
2.40.0

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

[Jan Beulich]

On 31.03.2023 11:59, Roger Pau Monne wrote:
> @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
>  
>          efi_arch_edid(gop_handle);
>      }
> +    else
> +    {
> +        /* If no GOP, init ConOut (StdOut) to the max supported size. */
> +        efi_console_set_mode();
> +
> +        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> +                               &cols, &rows) == EFI_SUCCESS )
> +            efi_arch_console_init(cols, rows);
> +    }

Instead of making this an "else", wouldn't you better check that a
valid gop_mode was found? efi_find_gop_mode() can return ~0 after all.

Furthermore, what if the active mode doesn't support text output? (I
consider the spec unclear in regard to whether this is possible, but
maybe I simply didn't find the right place stating it.)

Finally I think efi_arch_console_init() wants calling nevertheless.

So altogether maybe

    if ( gop_mode == ~0 ||
         StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
                           &cols, &rows) != EFI_SUCCESS )
        /* If no usable GOP mode, init ConOut (StdOut) to the max supported size. */
        efi_console_set_mode();

    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
                           &cols, &rows) == EFI_SUCCESS )
        efi_arch_console_init(cols, rows);

?

Jan

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

[Roger Pau Monné]

On Wed, Apr 05, 2023 at 12:36:55PM +0200, Jan Beulich wrote:
> On 31.03.2023 11:59, Roger Pau Monne wrote:
> > @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
> >  
> >          efi_arch_edid(gop_handle);
> >      }
> > +    else
> > +    {
> > +        /* If no GOP, init ConOut (StdOut) to the max supported size. */
> > +        efi_console_set_mode();
> > +
> > +        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> > +                               &cols, &rows) == EFI_SUCCESS )
> > +            efi_arch_console_init(cols, rows);
> > +    }
> 
> Instead of making this an "else", wouldn't you better check that a
> valid gop_mode was found? efi_find_gop_mode() can return ~0 after all.

When using vga=current gop_mode would also be ~0, in order for
efi_set_gop_mode() to not change the current mode, I was trying to
avoid exposing keep_current or similar extra variable to signal this.

> Furthermore, what if the active mode doesn't support text output? (I
> consider the spec unclear in regard to whether this is possible, but
> maybe I simply didn't find the right place stating it.)
> 
> Finally I think efi_arch_console_init() wants calling nevertheless.
> 
> So altogether maybe
> 
>     if ( gop_mode == ~0 ||
>          StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
>                            &cols, &rows) != EFI_SUCCESS )

I think it would make more sense to call efi_console_set_mode() only
if the current StdOut mode is not valid, as anything different from
vga=current will already force a GOP mode change.

Thanks, Roger.

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

[Jan Beulich]

On 31.05.2023 12:57, Roger Pau Monné wrote:
> On Wed, Apr 05, 2023 at 12:36:55PM +0200, Jan Beulich wrote:
>> On 31.03.2023 11:59, Roger Pau Monne wrote:
>>> @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
>>>  
>>>          efi_arch_edid(gop_handle);
>>>      }
>>> +    else
>>> +    {
>>> +        /* If no GOP, init ConOut (StdOut) to the max supported size. */
>>> +        efi_console_set_mode();
>>> +
>>> +        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
>>> +                               &cols, &rows) == EFI_SUCCESS )
>>> +            efi_arch_console_init(cols, rows);
>>> +    }
>>
>> Instead of making this an "else", wouldn't you better check that a
>> valid gop_mode was found? efi_find_gop_mode() can return ~0 after all.
> 
> When using vga=current gop_mode would also be ~0, in order for
> efi_set_gop_mode() to not change the current mode,

And then we'd skip efi_console_set_mode() here as well, which I think
is what we want with "vga=current"?

> I was trying to
> avoid exposing keep_current or similar extra variable to signal this.
> 
>> Furthermore, what if the active mode doesn't support text output? (I
>> consider the spec unclear in regard to whether this is possible, but
>> maybe I simply didn't find the right place stating it.)
>>
>> Finally I think efi_arch_console_init() wants calling nevertheless.
>>
>> So altogether maybe
>>
>>     if ( gop_mode == ~0 ||
>>          StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
>>                            &cols, &rows) != EFI_SUCCESS )
> 
> I think it would make more sense to call efi_console_set_mode() only
> if the current StdOut mode is not valid, as anything different from
> vga=current will already force a GOP mode change.

Hmm, this may also make sense. I guess I'd like to see the combined
result to be better able to judge.

Jan