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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.