[PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[Roger Pau Monne]

Hello,

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

First patch introduces a new platform hypercall to pass the graphics
console information and mode to a PVH dom0, which doesn't have this
information available as part of the start_info contents.

Further 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.

Marek: after this series using console= without the vga option should
result in Xen not attempting to touch the selected GOP mode and the
screen not getting cleared.

Thanks, Roger.

Roger Pau Monne (5):
  x86/platform: introduce hypercall to get initial video console
    settings
  efi: only set a console mode if the current one is invalid
  efi: try to use the currently set GOP mode
  multiboot2: parse console= option when setting GOP mode
  multiboot2: parse vga= option when setting GOP mode

 xen/arch/x86/boot/head.S          | 15 ++++++++--
 xen/arch/x86/efi/efi-boot.h       | 48 +++++++++++++++++++++++++++++--
 xen/arch/x86/platform_hypercall.c | 11 +++++++
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 xen/common/efi/boot.c             | 25 ++++++++++++++++
 xen/drivers/video/vga.c           |  2 +-
 xen/include/public/platform.h     |  6 ++++
 7 files changed, 103 insertions(+), 5 deletions(-)

-- 
2.37.3

[PATCH 1/5] x86/platform: introduce hypercall to get initial video console settings

[Roger Pau Monne]

This is required so PVH dom0 can get the initial video console state
as handled by Xen.  PV dom0 will get this as part of the start_info,
but it doesn't seem necessary to place such information in the
HVM start info.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/platform_hypercall.c | 11 +++++++++++
 xen/drivers/video/vga.c           |  2 +-
 xen/include/public/platform.h     |  6 ++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index a7341dc3d7..3f0d0389af 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -839,6 +839,17 @@ ret_t do_platform_op(
     }
     break;
 
+    case XENPF_get_dom0_console:
+        if ( !fill_console_start_info(&op->u.dom0_console) )
+        {
+            ret = -ENODEV;
+            break;
+        }
+
+        if ( copy_field_to_guest(u_xenpf_op, op, u.dom0_console) )
+            ret = -EFAULT;
+        break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index 29a88e8241..0a03508bee 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -205,7 +205,7 @@ static void cf_check vga_text_puts(const char *s, size_t nr)
     }
 }
 
-int __init fill_console_start_info(struct dom0_vga_console_info *ci)
+int fill_console_start_info(struct dom0_vga_console_info *ci)
 {
     memcpy(ci, &vga_console_info, sizeof(*ci));
     return 1;
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 5e1494fe9a..14784dfa77 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -605,6 +605,11 @@ struct xenpf_symdata {
 typedef struct xenpf_symdata xenpf_symdata_t;
 DEFINE_XEN_GUEST_HANDLE(xenpf_symdata_t);
 
+/* Fetch the video console information and mode setup by Xen. */
+#define XENPF_get_dom0_console 64
+typedef struct dom0_vga_console_info xenpf_dom0_console_t;
+DEFINE_XEN_GUEST_HANDLE(xenpf_dom0_console_t);
+
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
@@ -635,6 +640,7 @@ struct xen_platform_op {
         xenpf_core_parking_t          core_parking;
         xenpf_resource_op_t           resource_op;
         xenpf_symdata_t               symdata;
+        xenpf_dom0_console_t          dom0_console;
         uint8_t                       pad[128];
     } u;
 };
-- 
2.37.3

[PATCH 2/5] efi: only set a console mode if the current one is invalid

[Roger Pau Monne]

Do not unconditionally set a mode in efi_console_set_mode(), do so
only if the currently set mode is not valid.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/efi/boot.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index db0340c8e2..7e8a8b7857 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void)
     UINTN cols, rows, size;
     unsigned int best, i;
 
+    /* Only set a mode if the current one is not valid. */
+    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) ==
+         EFI_SUCCESS )
+        return;
+
     for ( i = 0, size = 0, best = StdOut->Mode->Mode;
           i < StdOut->Mode->MaxMode; ++i )
     {
-- 
2.37.3

[PATCH 3/5] 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>
---
 xen/common/efi/boot.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 7e8a8b7857..b91a7179a9 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -864,6 +864,26 @@ 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.
+             */
+            cols = mode_info->HorizontalResolution;
+            rows = mode_info->VerticalResolution;
+            depth = 0;
+        }
+    }
+
     for ( i = size = 0; i < gop->Mode->MaxMode; ++i )
     {
         unsigned int bpp = 0;
-- 
2.37.3

[PATCH 4/5] multiboot2: parse console= option 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= option if present.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I'm unsure why the parsing of the multiboot2 tags is done in assembly,
it could very well be done in efi_multiboot2() in C, but I don't want
to switch that code now.
---
 xen/arch/x86/boot/head.S          | 15 ++++++++++++--
 xen/arch/x86/efi/efi-boot.h       | 33 +++++++++++++++++++++++++++++--
 xen/arch/x86/x86_64/asm-offsets.c |  1 +
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0fb7dd3029..6920ad08d1 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -221,9 +221,10 @@ __efi64_mb2_start:
         jmp     x86_32_switch
 
 .Lefi_multiboot2_proto:
-        /* Zero EFI SystemTable and EFI ImageHandle addresses. */
+        /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
         xor     %esi,%esi
         xor     %edi,%edi
+        xor     %edx,%edx
 
         /* Skip Multiboot2 information fixed part. */
         lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
@@ -265,6 +266,15 @@ __efi64_mb2_start:
         cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
         je      .Lrun_bs
 
+        /*
+         * Get command line from Multiboot2 information.
+         * Must be last parsed tag.
+         */
+        cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
+        jne     .Lefi_mb2_next_tag
+        mov     %rcx,%rdx
+        add     $(MB2_tag_string),%rdx
+
 .Lefi_mb2_next_tag:
         /* Go to next Multiboot2 information tag. */
         add     MB2_tag_size(%rcx),%ecx
@@ -324,7 +334,8 @@ __efi64_mb2_start:
 
         /*
          * efi_multiboot2() is called according to System V AMD64 ABI:
-         *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
+         *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
+         *          %rdx - MB2 cmdline
          */
         call    efi_multiboot2
 
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 27f928ed3c..695491a5b7 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -786,7 +786,22 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
 
 static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { }
 
-void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
+/* Return the last occurrence of opt in cmd. */
+static const char __init *get_option(const char *cmd, const char *opt)
+{
+    const char *s = cmd, *o = NULL;
+
+    while ( (s = strstr(s, opt)) != NULL )
+    {
+        s += strlen(opt);
+        o = s;
+    }
+
+    return o;
+}
+
+void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable,
+                           const char *cmdline)
 {
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
     EFI_HANDLE gop_handle;
@@ -807,7 +822,21 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
 
     if ( gop )
     {
-        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
+        const char *opt = get_option(cmdline, "console=");
+        bool vga = false;
+
+        if ( opt )
+        {
+            const char *s = strstr(opt, "vga");
+
+            if ( s && s < strpbrk(opt, " \0"))
+                vga = true;
+        }
+
+        if ( vga )
+        {
+            gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
+        }
 
         efi_arch_edid(gop_handle);
     }
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 287dac101a..fbd6c54188 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -175,6 +175,7 @@ void __dummy__(void)
     OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
     OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer);
     OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
+    OFFSET(MB2_tag_string, multiboot2_tag_string_t, string);
     BLANK();
 
     OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
-- 
2.37.3

[PATCH 5/5] multiboot2: parse vga= option when setting GOP mode

[Roger Pau Monne]

Currently the vga command line gfx- option is ignored when booted
using multboot2 and EFI, as the setting of the GOP mode is done way
before the command line is processed.

Add support for parsing the vga gfx- selection if present in order to
set the selected GOP mode.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/efi/efi-boot.h | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 695491a5b7..e791d65213 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -835,7 +835,22 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
 
         if ( vga )
         {
-            gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
+            unsigned int width = 0, height = 0, depth = 0;
+
+            opt = get_option(cmdline, "vga=gfx-");
+            if ( opt )
+            {
+                width = simple_strtoul(opt, &opt, 10);
+                if ( *opt == 'x' )
+                    height = simple_strtoul(opt + 1, &opt, 10);
+                if ( *opt == 'x' )
+                    depth = simple_strtoul(opt + 1, &opt, 10);
+                /* Allow depth to be 0 or unset. */
+                if ( !width || !height )
+                    width = height = depth = 0;
+            }
+
+            gop_mode = efi_find_gop_mode(gop, width, height, depth);
         }
 
         efi_arch_edid(gop_handle);
-- 
2.37.3

Re: [PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On Wed, Nov 23, 2022 at 04:45:19PM +0100, Roger Pau Monne wrote:
> Marek: after this series using console= without the vga option should
> result in Xen not attempting to touch the selected GOP mode and the
> screen not getting cleared.

Thanks, this seems to work mostly fine.
There is one message printed from setup_efi_pci(): ... ROM ... bytes at ...
I'm not sure what to do about this one (although for Qubes, I can simply
patch it out ;) ).

But to get dom0 display image from BGRT, it seems something else is
needed too. Linux complains "Incorrect checksum in table [BGRT]". The
only relevant google result I get is this: https://support.citrix.com/article/CTX460227/citrix-hypervisor-acpi-warning-incorrect-checksum-in-table-bgrt
It blames firmware. But then, it's suspicious that it's also about Xen.
And also, native Linux on the same hw does not complain about the
checksum. So, I think it's rather Xen to blame...
The table lives in area marked as EfiACPIReclaimMemory in memory map, so
I think it shouldn't be clobbered by Xen, at least in theory. I'll look
into it later. It's getting off-topic for this thread anyway.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[Jan Beulich]

On 24.11.2022 06:15, Marek Marczykowski-Górecki wrote:
> On Wed, Nov 23, 2022 at 04:45:19PM +0100, Roger Pau Monne wrote:
>> Marek: after this series using console= without the vga option should
>> result in Xen not attempting to touch the selected GOP mode and the
>> screen not getting cleared.
> 
> Thanks, this seems to work mostly fine.
> There is one message printed from setup_efi_pci(): ... ROM ... bytes at ...
> I'm not sure what to do about this one (although for Qubes, I can simply
> patch it out ;) ).

What's wrong with that message? It's not directly related to gfx devices
anyway; it merely happens to be the case that gfx devices are the most
common ones to come with a ROM.

Jan

Re: [PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[Roger Pau Monné]

On Thu, Nov 24, 2022 at 06:15:15AM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Nov 23, 2022 at 04:45:19PM +0100, Roger Pau Monne wrote:
> > Marek: after this series using console= without the vga option should
> > result in Xen not attempting to touch the selected GOP mode and the
> > screen not getting cleared.
> 
> Thanks, this seems to work mostly fine.
> There is one message printed from setup_efi_pci(): ... ROM ... bytes at ...
> I'm not sure what to do about this one (although for Qubes, I can simply
> patch it out ;) ).

Hm, I'm unsure.  As a starter they could be gated to debug hypervisor only
builds.  And then I'm unsure whether this information couldn't be
printed later when the console option has been parsed, instead of
printing it from the EFI console interface.

> But to get dom0 display image from BGRT, it seems something else is
> needed too. Linux complains "Incorrect checksum in table [BGRT]". The
> only relevant google result I get is this: https://support.citrix.com/article/CTX460227/citrix-hypervisor-acpi-warning-incorrect-checksum-in-table-bgrt
> It blames firmware. But then, it's suspicious that it's also about Xen.
> And also, native Linux on the same hw does not complain about the
> checksum. So, I think it's rather Xen to blame...
> The table lives in area marked as EfiACPIReclaimMemory in memory map, so
> I think it shouldn't be clobbered by Xen, at least in theory. I'll look
> into it later. It's getting off-topic for this thread anyway.

See commit 89238ef7797023f318f82f4f9dddef59c435b8bd.  I wonder whether
the BGRT image region is marked as EFI_MEMORY_RUNTIME, I will have to
check on my system.

Thanks, Roger.

Re: [PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[Roger Pau Monné]

On Thu, Nov 24, 2022 at 09:59:25AM +0100, Roger Pau Monné wrote:
> On Thu, Nov 24, 2022 at 06:15:15AM +0100, Marek Marczykowski-Górecki wrote:
> > But to get dom0 display image from BGRT, it seems something else is
> > needed too. Linux complains "Incorrect checksum in table [BGRT]". The
> > only relevant google result I get is this: https://support.citrix.com/article/CTX460227/citrix-hypervisor-acpi-warning-incorrect-checksum-in-table-bgrt
> > It blames firmware. But then, it's suspicious that it's also about Xen.
> > And also, native Linux on the same hw does not complain about the
> > checksum. So, I think it's rather Xen to blame...
> > The table lives in area marked as EfiACPIReclaimMemory in memory map, so
> > I think it shouldn't be clobbered by Xen, at least in theory. I'll look
> > into it later. It's getting off-topic for this thread anyway.
> 
> See commit 89238ef7797023f318f82f4f9dddef59c435b8bd.  I wonder whether
> the BGRT image region is marked as EFI_MEMORY_RUNTIME, I will have to
> check on my system.

Just checked on my system, and the BGRT image is placed in a
EfiBootServicesData section with no EFI_MEMORY_RUNTIME attribute.

To fix this we would need to change efi_arch_process_memory_map() so
it takes the BGRT image address into account and marks the region
where it's placed as reserved.  I'm not aware of anyway to get such
address from EFI data, so we would likely need to parse the BGRT in
efi_arch_process_memory_map().

Thanks, Roger.

Re: [PATCH 0/5] gfx: improvements when using multiboot2 and EFI + misc

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

On Thu, Nov 24, 2022 at 10:56:33AM +0100, Roger Pau Monné wrote:
> On Thu, Nov 24, 2022 at 09:59:25AM +0100, Roger Pau Monné wrote:
> > On Thu, Nov 24, 2022 at 06:15:15AM +0100, Marek Marczykowski-Górecki wrote:
> > > But to get dom0 display image from BGRT, it seems something else is
> > > needed too. Linux complains "Incorrect checksum in table [BGRT]". The
> > > only relevant google result I get is this: https://support.citrix.com/article/CTX460227/citrix-hypervisor-acpi-warning-incorrect-checksum-in-table-bgrt
> > > It blames firmware. But then, it's suspicious that it's also about Xen.
> > > And also, native Linux on the same hw does not complain about the
> > > checksum. So, I think it's rather Xen to blame...
> > > The table lives in area marked as EfiACPIReclaimMemory in memory map, so
> > > I think it shouldn't be clobbered by Xen, at least in theory. I'll look
> > > into it later. It's getting off-topic for this thread anyway.
> > 
> > See commit 89238ef7797023f318f82f4f9dddef59c435b8bd.  I wonder whether
> > the BGRT image region is marked as EFI_MEMORY_RUNTIME, I will have to
> > check on my system.
> 
> Just checked on my system, and the BGRT image is placed in a
> EfiBootServicesData section with no EFI_MEMORY_RUNTIME attribute.

Right, while the BGRT table itself is in EfiACPIReclaimMemory, the image
it points to lives in EfiBootServicesData. And no EFI_MEMORY_RUNTIME
attribute there either.

> To fix this we would need to change efi_arch_process_memory_map() so
> it takes the BGRT image address into account and marks the region
> where it's placed as reserved.  I'm not aware of anyway to get such
> address from EFI data, so we would likely need to parse the BGRT in
> efi_arch_process_memory_map().

Since Xen has code to do that already, moving it earlier shouldn't be
too much issue. Can `acpi_boot_table_init()` be called that early? And
then, it sounds very similar to the issue we have with the ESRT table.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] x86/platform: introduce hypercall to get initial video console settings

[Jan Beulich]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> This is required so PVH dom0 can get the initial video console state
> as handled by Xen.  PV dom0 will get this as part of the start_info,
> but it doesn't seem necessary to place such information in the
> HVM start info.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I'm okay with this as is, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but ...

>  xen/arch/x86/platform_hypercall.c | 11 +++++++++++
>  xen/drivers/video/vga.c           |  2 +-
>  xen/include/public/platform.h     |  6 ++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)

... wasn't the goal for Arm to have the same interface?

Jan

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Jan Beulich]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> Do not unconditionally set a mode in efi_console_set_mode(), do so
> only if the currently set mode is not valid.

You don't say why you want to do so. Furthermore ...

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void)
>      UINTN cols, rows, size;
>      unsigned int best, i;
>  
> +    /* Only set a mode if the current one is not valid. */
> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) ==
> +         EFI_SUCCESS )
> +        return;

... it might be okay if you put such a check in efi_multiboot2(), but
the call here from efi_start() is specifically guarded by a check of
whether "-basevideo" was passed to xen.efi. This _may_ not be as
relevant anymore today, but it certainly was 20 years ago (recall
that we've inherited this code from a much older project of ours) -
at that time EFI usually started in 80x25 text mode. And I think that
even today when you end up launching xen.efi from the EFI shell,
you'd be stuck with 80x25 text mode on at least some implementations.

Overall, looking at (for now) just the titles of subsequent patches,
I'm not convinced the change here is needed at all. Or if anything it
may want to go at the end, taking action only when "vga=current" was
specified.

Jan

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

[Jan Beulich]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -864,6 +864,26 @@ 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.
> +             */
> +            cols = mode_info->HorizontalResolution;
> +            rows = mode_info->VerticalResolution;

For these I think you want to replace cols and rows individually, i.e.

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

whereas ...

> +            depth = 0;

... this case looks more complicated, as 0 is also already legitimate
as a value to come in. By zapping a non-zero incoming value you may end
up switching modes _just_ to alter depth, and then you may not fulfill
what was requested. For now I think depth simply wants leaving alone
here (and perhaps using the current mode's value if the incoming value
was zero, thus eliminating the need for a mode change in certain cases).

In a separate change we might then enhance this, e.g. by finding a
supported mode matching geometry but only coming close for "depth".

Jan

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Jan Beulich]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> Only set the GOP mode if vga is selected in the console option,
> otherwise just fetch the information from the current mode in order to
> make it available to dom0.
> 
> Introduce support for passing the command line to the efi_multiboot2()
> helper, and parse the console= option if present.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I'm unsure why the parsing of the multiboot2 tags is done in assembly,
> it could very well be done in efi_multiboot2() in C, but I don't want
> to switch that code now.

I guess that's mainly mirroring the non-EFI boot path, where the amount
of work needed to eventually enter C land is quite a bit larger?
Anything beyond that Daniel may want to point out.

> @@ -265,6 +266,15 @@ __efi64_mb2_start:
>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>          je      .Lrun_bs
>  
> +        /*
> +         * Get command line from Multiboot2 information.
> +         * Must be last parsed tag.

Why? And how do you guarantee this?

> +         */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
> +        jne     .Lefi_mb2_next_tag
> +        mov     %rcx,%rdx
> +        add     $(MB2_tag_string),%rdx

Simply "lea MB2_tag_string(%rcx),%rdx"?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -786,7 +786,22 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>  
>  static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { }
>  
> -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> +/* Return the last occurrence of opt in cmd. */

Is this sufficient in the general case (it may be for "console=", but
perhaps not for "vga=", which may also need finding as per below)?

> +static const char __init *get_option(const char *cmd, const char *opt)

Nit: The first * wants to move earlier.

> +{
> +    const char *s = cmd, *o = NULL;
> +
> +    while ( (s = strstr(s, opt)) != NULL )

I'm afraid this is too easy to break without considering separators as
well. If I'm not mistaken you'd also match e.g. "sync_console=1" for
the sole present caller.

> +    {
> +        s += strlen(opt);
> +        o = s;
> +    }
> +
> +    return o;
> +}
> +
> +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable,
> +                           const char *cmdline)
>  {
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
>      EFI_HANDLE gop_handle;
> @@ -807,7 +822,21 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
>  
>      if ( gop )
>      {
> -        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> +        const char *opt = get_option(cmdline, "console=");
> +        bool vga = false;
> +
> +        if ( opt )
> +        {
> +            const char *s = strstr(opt, "vga");
> +
> +            if ( s && s < strpbrk(opt, " \0"))
> +                vga = true;
> +        }

Don't you also want to find a "vga=gfx-..." option, to avoid ...

> +        if ( vga )
> +        {
> +            gop_mode = efi_find_gop_mode(gop, 0, 0, 0);

... requesting a "random" mode here?

> +        }

Nit: No need for the braces in cases like this one.

Jan

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Jan Beulich]

On 05.12.2022 16:10, Jan Beulich wrote:
> On 23.11.2022 16:45, Roger Pau Monne wrote:
>> @@ -807,7 +822,21 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
>>  
>>      if ( gop )
>>      {
>> -        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
>> +        const char *opt = get_option(cmdline, "console=");
>> +        bool vga = false;
>> +
>> +        if ( opt )
>> +        {
>> +            const char *s = strstr(opt, "vga");
>> +
>> +            if ( s && s < strpbrk(opt, " \0"))
>> +                vga = true;
>> +        }
> 
> Don't you also want to find a "vga=gfx-..." option, to avoid ...

Apologies - I should have looked at the title of the next patch
before writing this.

Jan

Re: [PATCH 5/5] multiboot2: parse vga= option when setting GOP mode

[Jan Beulich]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> Currently the vga command line gfx- option is ignored when booted
> using multboot2 and EFI, as the setting of the GOP mode is done way
> before the command line is processed.
> 
> Add support for parsing the vga gfx- selection if present in order to
> set the selected GOP mode.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit personally I think this should be folded into the previous patch.

Jan

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Daniel Kiper]

Sorry for late reply...

On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote:
> On 23.11.2022 16:45, Roger Pau Monne wrote:
> > Only set the GOP mode if vga is selected in the console option,
> > otherwise just fetch the information from the current mode in order to
> > make it available to dom0.
> >
> > Introduce support for passing the command line to the efi_multiboot2()
> > helper, and parse the console= option if present.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > I'm unsure why the parsing of the multiboot2 tags is done in assembly,
> > it could very well be done in efi_multiboot2() in C, but I don't want
> > to switch that code now.
>
> I guess that's mainly mirroring the non-EFI boot path, where the amount
> of work needed to eventually enter C land is quite a bit larger?
> Anything beyond that Daniel may want to point out.

Yeah, you are right, its mainly mirroring the non-EFI boot path and it
evolved from that code. However, if you want to move it to C go for it...

Daniel

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Roger Pau Monné]

On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote:
> On 23.11.2022 16:45, Roger Pau Monne wrote:
> > @@ -265,6 +266,15 @@ __efi64_mb2_start:
> >          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> >          je      .Lrun_bs
> >  
> > +        /*
> > +         * Get command line from Multiboot2 information.
> > +         * Must be last parsed tag.
> 
> Why? And how do you guarantee this?

I think the comment is misleading, must be the last checked for tag in
the loop that does this in assembly, because it's not using cmove.
I've adjusted to:

        /* Get command line from Multiboot2 information. */
        cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
        jne     .Lno_cmdline
        lea     MB2_tag_string(%rcx),%rdx
        jmp     .Lefi_mb2_next_tag
.Lno_cmdline:

Maybe there's some instruction I'm missing similar to a conditional
lea?

> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -786,7 +786,22 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> >  
> >  static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { }
> >  
> > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> > +/* Return the last occurrence of opt in cmd. */
> 
> Is this sufficient in the general case (it may be for "console=", but
> perhaps not for "vga=", which may also need finding as per below)?

I guess for vga= it's possible to have something like:

vga=current vga=keep

And in that case we do care about the non-last option.

> > +static const char __init *get_option(const char *cmd, const char *opt)
> 
> Nit: The first * wants to move earlier.
> 
> > +{
> > +    const char *s = cmd, *o = NULL;
> > +
> > +    while ( (s = strstr(s, opt)) != NULL )
> 
> I'm afraid this is too easy to break without considering separators as
> well. If I'm not mistaken you'd also match e.g. "sync_console=1" for
> the sole present caller.

Right, wasn't taking into account that sync_console can also be a
boolean (and thus assigned).

> > +        }
> 
> Nit: No need for the braces in cases like this one.

I've added the braces here because it will be expended in the next
patch.  Since you asked for both patches to be merged this will go
away.

Thanks, Roger.

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Jan Beulich]

On 29.03.2023 18:29, Roger Pau Monné wrote:
> On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote:
>> On 23.11.2022 16:45, Roger Pau Monne wrote:
>>> @@ -265,6 +266,15 @@ __efi64_mb2_start:
>>>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>>>          je      .Lrun_bs
>>>  
>>> +        /*
>>> +         * Get command line from Multiboot2 information.
>>> +         * Must be last parsed tag.
>>
>> Why? And how do you guarantee this?
> 
> I think the comment is misleading, must be the last checked for tag in
> the loop that does this in assembly, because it's not using cmove.
> I've adjusted to:
> 
>         /* Get command line from Multiboot2 information. */
>         cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
>         jne     .Lno_cmdline
>         lea     MB2_tag_string(%rcx),%rdx
>         jmp     .Lefi_mb2_next_tag
> .Lno_cmdline:
> 
> Maybe there's some instruction I'm missing similar to a conditional
> lea?

There isn't. If you want to get away without conditional branch, then
you'll need to LEA to a scratch register (unconditional) and then
CMOVcc from that scratch register.

Jan

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Roger Pau Monné]

On Thu, Mar 30, 2023 at 08:24:20AM +0200, Jan Beulich wrote:
> On 29.03.2023 18:29, Roger Pau Monné wrote:
> > On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote:
> >> On 23.11.2022 16:45, Roger Pau Monne wrote:
> >>> @@ -265,6 +266,15 @@ __efi64_mb2_start:
> >>>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> >>>          je      .Lrun_bs
> >>>  
> >>> +        /*
> >>> +         * Get command line from Multiboot2 information.
> >>> +         * Must be last parsed tag.
> >>
> >> Why? And how do you guarantee this?
> > 
> > I think the comment is misleading, must be the last checked for tag in
> > the loop that does this in assembly, because it's not using cmove.
> > I've adjusted to:
> > 
> >         /* Get command line from Multiboot2 information. */
> >         cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
> >         jne     .Lno_cmdline
> >         lea     MB2_tag_string(%rcx),%rdx
> >         jmp     .Lefi_mb2_next_tag
> > .Lno_cmdline:
> > 
> > Maybe there's some instruction I'm missing similar to a conditional
> > lea?
> 
> There isn't. If you want to get away without conditional branch, then
> you'll need to LEA to a scratch register (unconditional) and then
> CMOVcc from that scratch register.

Likely not worth it, unless you dislike the extra conditional branch.

Thanks, Roger.

Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Jan Beulich]

On 30.03.2023 10:11, Roger Pau Monné wrote:
> On Thu, Mar 30, 2023 at 08:24:20AM +0200, Jan Beulich wrote:
>> On 29.03.2023 18:29, Roger Pau Monné wrote:
>>> On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote:
>>>> On 23.11.2022 16:45, Roger Pau Monne wrote:
>>>>> @@ -265,6 +266,15 @@ __efi64_mb2_start:
>>>>>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>>>>>          je      .Lrun_bs
>>>>>  
>>>>> +        /*
>>>>> +         * Get command line from Multiboot2 information.
>>>>> +         * Must be last parsed tag.
>>>>
>>>> Why? And how do you guarantee this?
>>>
>>> I think the comment is misleading, must be the last checked for tag in
>>> the loop that does this in assembly, because it's not using cmove.
>>> I've adjusted to:
>>>
>>>         /* Get command line from Multiboot2 information. */
>>>         cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
>>>         jne     .Lno_cmdline
>>>         lea     MB2_tag_string(%rcx),%rdx
>>>         jmp     .Lefi_mb2_next_tag
>>> .Lno_cmdline:
>>>
>>> Maybe there's some instruction I'm missing similar to a conditional
>>> lea?
>>
>> There isn't. If you want to get away without conditional branch, then
>> you'll need to LEA to a scratch register (unconditional) and then
>> CMOVcc from that scratch register.
> 
> Likely not worth it, unless you dislike the extra conditional branch.

Entirely up to you - I'm fine either way.

Jan

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Roger Pau Monné]

On Mon, Dec 05, 2022 at 03:19:13PM +0100, Jan Beulich wrote:
> On 23.11.2022 16:45, Roger Pau Monne wrote:
> > Do not unconditionally set a mode in efi_console_set_mode(), do so
> > only if the currently set mode is not valid.
> 
> You don't say why you want to do so. Furthermore ...
> 
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void)
> >      UINTN cols, rows, size;
> >      unsigned int best, i;
> >  
> > +    /* Only set a mode if the current one is not valid. */
> > +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) ==
> > +         EFI_SUCCESS )
> > +        return;
> 
> ... it might be okay if you put such a check in efi_multiboot2(), but
> the call here from efi_start() is specifically guarded by a check of
> whether "-basevideo" was passed to xen.efi. This _may_ not be as
> relevant anymore today, but it certainly was 20 years ago (recall
> that we've inherited this code from a much older project of ours) -
> at that time EFI usually started in 80x25 text mode. And I think that
> even today when you end up launching xen.efi from the EFI shell,
> you'd be stuck with 80x25 text mode on at least some implementations.

Won't you use console=vga vga=gfx-...

To switch to a best mode?

> Overall, looking at (for now) just the titles of subsequent patches,
> I'm not convinced the change here is needed at all. Or if anything it
> may want to go at the end, taking action only when "vga=current" was
> specified.

I guess I'm slightly confused by the usage of both GOP and StdOut, I
would assume if we have a gop, and can correctly initialize it there's
no need to fiddle with StdOut also?

Thanks, Roger.

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Jan Beulich]

On 30.03.2023 17:44, Roger Pau Monné wrote:
> On Mon, Dec 05, 2022 at 03:19:13PM +0100, Jan Beulich wrote:
>> On 23.11.2022 16:45, Roger Pau Monne wrote:
>>> Do not unconditionally set a mode in efi_console_set_mode(), do so
>>> only if the currently set mode is not valid.
>>
>> You don't say why you want to do so. Furthermore ...
>>
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void)
>>>      UINTN cols, rows, size;
>>>      unsigned int best, i;
>>>  
>>> +    /* Only set a mode if the current one is not valid. */
>>> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) ==
>>> +         EFI_SUCCESS )
>>> +        return;
>>
>> ... it might be okay if you put such a check in efi_multiboot2(), but
>> the call here from efi_start() is specifically guarded by a check of
>> whether "-basevideo" was passed to xen.efi. This _may_ not be as
>> relevant anymore today, but it certainly was 20 years ago (recall
>> that we've inherited this code from a much older project of ours) -
>> at that time EFI usually started in 80x25 text mode. And I think that
>> even today when you end up launching xen.efi from the EFI shell,
>> you'd be stuck with 80x25 text mode on at least some implementations.
> 
> Won't you use console=vga vga=gfx-...
> 
> To switch to a best mode?

I don't think "vga=gfx-..." is presently consumed in any way xen.efi.
Doing so would require a similar hack of peeking at the (ordinary)
command line options as in legacy booting, except that in xen.efi we
read that command line from a file, which iirc is done only after
fiddling with the video mode.

>> Overall, looking at (for now) just the titles of subsequent patches,
>> I'm not convinced the change here is needed at all. Or if anything it
>> may want to go at the end, taking action only when "vga=current" was
>> specified.
> 
> I guess I'm slightly confused by the usage of both GOP and StdOut, I
> would assume if we have a gop, and can correctly initialize it there's
> no need to fiddle with StdOut also?

Setting the GOP mode is done last before exiting boot services; this
may be a graphics mode which doesn't support a text output protocol.

Jan

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Roger Pau Monné]

On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote:
> On 30.03.2023 17:44, Roger Pau Monné wrote:
> > On Mon, Dec 05, 2022 at 03:19:13PM +0100, Jan Beulich wrote:
> >> On 23.11.2022 16:45, Roger Pau Monne wrote:
> >>> Do not unconditionally set a mode in efi_console_set_mode(), do so
> >>> only if the currently set mode is not valid.
> >>
> >> You don't say why you want to do so. Furthermore ...
> >>
> >>> --- a/xen/common/efi/boot.c
> >>> +++ b/xen/common/efi/boot.c
> >>> @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void)
> >>>      UINTN cols, rows, size;
> >>>      unsigned int best, i;
> >>>  
> >>> +    /* Only set a mode if the current one is not valid. */
> >>> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) ==
> >>> +         EFI_SUCCESS )
> >>> +        return;
> >>
> >> ... it might be okay if you put such a check in efi_multiboot2(), but
> >> the call here from efi_start() is specifically guarded by a check of
> >> whether "-basevideo" was passed to xen.efi. This _may_ not be as
> >> relevant anymore today, but it certainly was 20 years ago (recall
> >> that we've inherited this code from a much older project of ours) -
> >> at that time EFI usually started in 80x25 text mode. And I think that
> >> even today when you end up launching xen.efi from the EFI shell,
> >> you'd be stuck with 80x25 text mode on at least some implementations.
> > 
> > Won't you use console=vga vga=gfx-...
> > 
> > To switch to a best mode?
> 
> I don't think "vga=gfx-..." is presently consumed in any way xen.efi.
> Doing so would require a similar hack of peeking at the (ordinary)
> command line options as in legacy booting, except that in xen.efi we
> read that command line from a file, which iirc is done only after
> fiddling with the video mode.

I will only take care of multiboot2, since I don't have a way to test
xen.efi ATM.

> >> Overall, looking at (for now) just the titles of subsequent patches,
> >> I'm not convinced the change here is needed at all. Or if anything it
> >> may want to go at the end, taking action only when "vga=current" was
> >> specified.
> > 
> > I guess I'm slightly confused by the usage of both GOP and StdOut, I
> > would assume if we have a gop, and can correctly initialize it there's
> > no need to fiddle with StdOut also?
> 
> Setting the GOP mode is done last before exiting boot services; this
> may be a graphics mode which doesn't support a text output protocol.

Right, that's what I was missing.  I assumed that all modes available
in GOP would be compatible with the ConOut mode.

Would you be OK with leaving StdOut as-is when booted from multiboot2,
or there's a chance of things not being properly setup?

IMO it's not very friendly to change the StdOut mode if not explicitly
requested, as in the multiboot2 case that gets setup by the
bootloader.

Thanks, Roger.

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Jan Beulich]

On 30.03.2023 18:17, Roger Pau Monné wrote:
> On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote:
>> On 30.03.2023 17:44, Roger Pau Monné wrote:
>>> I guess I'm slightly confused by the usage of both GOP and StdOut, I
>>> would assume if we have a gop, and can correctly initialize it there's
>>> no need to fiddle with StdOut also?
>>
>> Setting the GOP mode is done last before exiting boot services; this
>> may be a graphics mode which doesn't support a text output protocol.
> 
> Right, that's what I was missing.  I assumed that all modes available
> in GOP would be compatible with the ConOut mode.
> 
> Would you be OK with leaving StdOut as-is when booted from multiboot2,
> or there's a chance of things not being properly setup?

On modern UEFI it may be unlikely, but I think it's not impossible (see
below).

> IMO it's not very friendly to change the StdOut mode if not explicitly
> requested, as in the multiboot2 case that gets setup by the
> bootloader.

May get set up, that is. If it was set up, then yes, we probably should
leave it alone unless told to use another mode. I.e. no vga= or
vga=current should minimally result in no further mode change. Aiui we
can't easily honor vga=gfx-... in that case, so leaving the mode alone
there may also be better than trying to guess a mode. The only time
where I would think it would be nice to switch by default even in the
xen.gz case is if the boot loader handed us the screen in some text
mode.

Jan

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Roger Pau Monné]

On Fri, Mar 31, 2023 at 08:51:46AM +0200, Jan Beulich wrote:
> On 30.03.2023 18:17, Roger Pau Monné wrote:
> > On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote:
> >> On 30.03.2023 17:44, Roger Pau Monné wrote:
> >>> I guess I'm slightly confused by the usage of both GOP and StdOut, I
> >>> would assume if we have a gop, and can correctly initialize it there's
> >>> no need to fiddle with StdOut also?
> >>
> >> Setting the GOP mode is done last before exiting boot services; this
> >> may be a graphics mode which doesn't support a text output protocol.
> > 
> > Right, that's what I was missing.  I assumed that all modes available
> > in GOP would be compatible with the ConOut mode.
> > 
> > Would you be OK with leaving StdOut as-is when booted from multiboot2,
> > or there's a chance of things not being properly setup?
> 
> On modern UEFI it may be unlikely, but I think it's not impossible (see
> below).
> 
> > IMO it's not very friendly to change the StdOut mode if not explicitly
> > requested, as in the multiboot2 case that gets setup by the
> > bootloader.
> 
> May get set up, that is. If it was set up, then yes, we probably should
> leave it alone unless told to use another mode. I.e. no vga= or
> vga=current should minimally result in no further mode change. Aiui we
> can't easily honor vga=gfx-... in that case, so leaving the mode alone
> there may also be better than trying to guess a mode. The only time
> where I would think it would be nice to switch by default even in the
> xen.gz case is if the boot loader handed us the screen in some text
> mode.

How would you detect such case?

ConOut is always text-mode like because it's a
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL interface.

Would it be a matter of checking whether the current GOP mode is
valid, and if so leave it as-is unless told otherwise by a command
line parameter?

I would also like to avoid the unconditional resizing of the ConOut
interface that's done in efi_console_set_mode(), as that has the size
effect of changing the GOP mode, so I would only call
efi_console_set_mode() is there's no gop.

Not sure it's meaningful to change the ConOut number of cols/rows if
there's no GOP, maybe it's possible to have some kind of screen that's
usable for EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL but not as a GOP?

Thanks, Roger.

Re: [PATCH 2/5] efi: only set a console mode if the current one is invalid

[Jan Beulich]

On 31.03.2023 09:37, Roger Pau Monné wrote:
> On Fri, Mar 31, 2023 at 08:51:46AM +0200, Jan Beulich wrote:
>> On 30.03.2023 18:17, Roger Pau Monné wrote:
>>> On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote:
>>>> On 30.03.2023 17:44, Roger Pau Monné wrote:
>>>>> I guess I'm slightly confused by the usage of both GOP and StdOut, I
>>>>> would assume if we have a gop, and can correctly initialize it there's
>>>>> no need to fiddle with StdOut also?
>>>>
>>>> Setting the GOP mode is done last before exiting boot services; this
>>>> may be a graphics mode which doesn't support a text output protocol.
>>>
>>> Right, that's what I was missing.  I assumed that all modes available
>>> in GOP would be compatible with the ConOut mode.
>>>
>>> Would you be OK with leaving StdOut as-is when booted from multiboot2,
>>> or there's a chance of things not being properly setup?
>>
>> On modern UEFI it may be unlikely, but I think it's not impossible (see
>> below).
>>
>>> IMO it's not very friendly to change the StdOut mode if not explicitly
>>> requested, as in the multiboot2 case that gets setup by the
>>> bootloader.
>>
>> May get set up, that is. If it was set up, then yes, we probably should
>> leave it alone unless told to use another mode. I.e. no vga= or
>> vga=current should minimally result in no further mode change. Aiui we
>> can't easily honor vga=gfx-... in that case, so leaving the mode alone
>> there may also be better than trying to guess a mode. The only time
>> where I would think it would be nice to switch by default even in the
>> xen.gz case is if the boot loader handed us the screen in some text
>> mode.
> 
> How would you detect such case?
> 
> ConOut is always text-mode like because it's a
> EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL interface.
> 
> Would it be a matter of checking whether the current GOP mode is
> valid, and if so leave it as-is unless told otherwise by a command
> line parameter?

I think so, yes.

> I would also like to avoid the unconditional resizing of the ConOut
> interface that's done in efi_console_set_mode(), as that has the size
> effect of changing the GOP mode, so I would only call
> efi_console_set_mode() is there's no gop.

Or maybe when the set mode isn't text-output capable.

> Not sure it's meaningful to change the ConOut number of cols/rows if
> there's no GOP, maybe it's possible to have some kind of screen that's
> usable for EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL but not as a GOP?

Of course there is. As said, earlier on screens started in 80x25 mode.
Even going to 80x50 or 80x60 is already an improvement. Plus there are
systems which support wider-than-80-cols text modes.

Jan