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

[Jan Beulich <jbeulich@xxxxxxxx>]

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