Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes

[Jan Beulich <jbeulich@xxxxxxxx>]

(reducing Cc list some)

On 04.04.2022 16:49, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
>> GrUB2 can be told to leave the screen in the graphics mode it has been
>> using (or any other one), via "set gfxpayload=keep" (or suitable
>> variants thereof). In this case we can avoid doing another mode switch
>> ourselves. This in particular avoids possibly setting the screen to a
>> less desirable mode: On one of my test systems the set of modes
>> reported available by the VESA BIOS depends on whether the interposed
>> KVM switch has that machine set as the active one. If it's not active,
>> only modes up to 1024x768 get reported, while when active 1280x1024
>> modes are also included. For things to always work with an explicitly
>> specified mode (via the "vga=" option), that mode therefore needs be a
>> 1024x768 one.
>>
>> For some reason this only works for me with "multiboot2" (and
>> "module2"); "multiboot" (and "module") still forces the screen into text
>> mode, despite my reading of the sources suggesting otherwise.
>>
>> For starters I'm limiting this to graphics modes; I do think this ought
>> to also work for text modes, but
>> - I can't tell whether GrUB2 can set any text mode other than 80x25
>>   (I've only found plain "text" to be valid as a "gfxpayload" setting),
>> - I'm uncertain whether supporting that is worth it, since I'm uncertain
>>   how many people would be running their systems/screens in text mode,
>> - I'd like to limit the amount of code added to the realmode trampoline.
>>
>> For starters I'm also limiting mode information retrieval to raw BIOS
>> accesses. This will allow things to work (in principle) also with other
>> boot environments where a graphics mode can be left in place. The
>> downside is that this then still is dependent upon switching back to
>> real mode, so retrieving the needed information from multiboot info is
>> likely going to be desirable down the road.
> 
> I'm unsure, what's the benefit from retrieving this information from
> the VESA blob rather than from multiboot(2) structures?

As said - it allows things to work even when that data isn't provided.
Note also how I say "for starters" - patch 2 adds logic to retrieve
the information from MB.

> Is it because we require a VESA mode to be set before we parse the
> multiboot information?

No, I don't think so.

>> --- a/xen/arch/x86/boot/video.S
>> +++ b/xen/arch/x86/boot/video.S
>> @@ -575,7 +575,6 @@ set14:  movw    $0x1111, %ax
>>          movb    $0x01, %ah              # Define cursor scan lines 11-12
>>          movw    $0x0b0c, %cx
>>          int     $0x10
>> -set_current:
>>          stc
>>          ret
>>  
>> @@ -693,6 +692,39 @@ vga_modes:
>>          .word   VIDEO_80x60, 0x50,0x3c,0        # 80x60
>>  vga_modes_end:
>>  
>> +# If the current mode is a VESA graphics one, obtain its parameters.
>> +set_current:
>> +        leaw    vesa_glob_info, %di
>> +        movw    $0x4f00, %ax
>> +        int     $0x10
>> +        cmpw    $0x004f, %ax
>> +        jne     .Lsetc_done
> 
> You don't seem to make use of the information fetched here? I guess
> this is somehow required to access the other functions?

See the similar logic at check_vesa. The information is used later, by
mode_params (half way into mopar_gr). Quite likely this could be done
just in a single place, but that would require some restructuring of
the code, which I'd like to avoid doing here.

>> +        movw    $0x4f03, %ax
> 
> It would help readability to have defines for those values, ie:
> VESA_GET_CURRENT_MODE or some such (not that you need to do it here,
> just a comment).

Right - this applies to all of our BIOS interfacing code, I guess.

>> +        int     $0x10
>> +        cmpw    $0x004f, %ax
>> +        jne     .Lsetc_done
>> +
>> +        leaw    vesa_mode_info, %di     # Get mode information structure
>> +        movw    %bx, %cx
>> +        movw    $0x4f01, %ax
>> +        int     $0x10
>> +        cmpw    $0x004f, %ax
>> +        jne     .Lsetc_done
>> +
>> +        movb    (%di), %al              # Check mode attributes
>> +        andb    $0x9b, %al
>> +        cmpb    $0x9b, %al
> 
> So you also check that the reserved D1 bit is set to 1 as mandated by
> the spec. This is slightly different than what's done in check_vesa,
> would you mind adding a define for this an unifying with check_vesa?

Well, see the v2 changelog comment. I'm somewhat hesitant to do that
here; I'd prefer to consolidate this in a separate patch.

Jan