diff mbox series

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

Message ID 3fb7f3e9-a6cf-4f8e-1141-100848b1bdd0@suse.com (mailing list archive)
State New, archived
Headers show
Series x86/boot: (mostly) video mode handling adjustments | expand

Commit Message

Jan Beulich March 31, 2022, 9:44 a.m. UTC
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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm not convinced boot_vid_mode really needs setting here; I'm doing so
mainly because setvesabysize also does.
---
v4: Add changelog entry.
v2: Use 0x9b instead of 0x99 for attributes check: I think the value
    used by check_vesa also wants to be converted, to match vesa2's.
    (Perhaps the value wants to become a #define, albeit before doing so
    I'd question the requirement of the mode to be a color one.)

Comments

Roger Pau Monné April 4, 2022, 2:49 p.m. UTC | #1
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?

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

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I'm not convinced boot_vid_mode really needs setting here; I'm doing so
> mainly because setvesabysize also does.
> ---
> v4: Add changelog entry.
> v2: Use 0x9b instead of 0x99 for attributes check: I think the value
>     used by check_vesa also wants to be converted, to match vesa2's.
>     (Perhaps the value wants to become a #define, albeit before doing so
>     I'd question the requirement of the mode to be a color one.)
> 
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog
>  
>  ## [unstable UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
>  
> +### Changed
> + - On x86 "vga=current" can now be used together with GrUB2's gfxpayload setting. Note that
> +   this requires use of "multiboot2" (and "module2") as the GrUB commands loading Xen.
> +
>  ### Removed / support downgraded
>   - dropped support for the (x86-only) "vesa-mtrr" and "vesa-remap" command line options
>  
> --- 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?

> +        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).

> +        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?

Thanks, Roger.
Jan Beulich April 4, 2022, 3:50 p.m. UTC | #2
(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
Roger Pau Monné April 5, 2022, 8:24 a.m. UTC | #3
On Mon, Apr 04, 2022 at 05:50:57PM +0200, Jan Beulich wrote:
> (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.

So this patch helps you by not having to set a mode and just relying
on the mode set by GrUB?

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

I didn't realize check_vesa and set_current where mutually
exclusive.

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

Sorry, didn't notice that v2 comment before.

It's my understanding that the main difference this patch introduces
is that set_current now fetches the currently set mode, so that we
avoid further mode changes if the mode set already matches the
selected one, or if Xen is to use the already set mode?

Thanks, Roger.
Jan Beulich April 5, 2022, 8:36 a.m. UTC | #4
On 05.04.2022 10:24, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 05:50:57PM +0200, Jan Beulich wrote:
>> (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.
> 
> So this patch helps you by not having to set a mode and just relying
> on the mode set by GrUB?

Yes, but it goes beyond that: The modes offered by VESA on the particular
system don't include the higher resolution one under certain circumstances,
so I cannot tell Xen to switch to that mode. By not having to tell Xen a
specific mode (but rather inherit that set / left active by the boot
loader), I can leverage the better mode in most cases, but things will
still work if I turn on (or reset) the system with another machine being
the presently selected one at the KVM switch.

But yes, beyond the particular quirk on this system the benefit is one
less mode switch and hence less screen flickering and slightly faster
boot.

>>>> --- 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.
> 
> I didn't realize check_vesa and set_current where mutually
> exclusive.
> 
>>>> +        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.
> 
> Sorry, didn't notice that v2 comment before.
> 
> It's my understanding that the main difference this patch introduces
> is that set_current now fetches the currently set mode, so that we
> avoid further mode changes if the mode set already matches the
> selected one, or if Xen is to use the already set mode?

Not exactly: You either tell Xen to use the current mode ("vga=current")
or you tell Xen to use a specific mode ("vga=<mode>"). Checking whether
the present mode is the (specific) one Xen was told to switch to would
require yet more work. But skipping a requested mode switch can also
have unintended consequences, so I wouldn't even be certain we would
want to go such a route.

Jan
Roger Pau Monné April 5, 2022, 8:45 a.m. UTC | #5
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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.
Jan Beulich April 6, 2022, 2:23 p.m. UTC | #6
On 05.04.2022 10:45, 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.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

May I ask for an ack or otherwise for the changelog entry, please?

Thanks, Jan
Jan Beulich April 11, 2022, 9:50 a.m. UTC | #7
On 06.04.2022 16:23, Jan Beulich wrote:
> On 05.04.2022 10:45, 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.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> May I ask for an ack or otherwise for the changelog entry, please?

Ping? This is the only thing missing for me to commit the remaining
parts of this series.

Thanks, Jan
Henry Wang April 11, 2022, 10:11 a.m. UTC | #8
Hi Jan,

> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > May I ask for an ack or otherwise for the changelog entry, please?
> 
> Ping? This is the only thing missing for me to commit the remaining
> parts of this series.

Sorry for the late response, the previous e-mail that you directly
"to"ed me fell through the cracks somehow.

Acked-by: Henry Wang <Henry.Wang@arm.com>

Since you also mentioned the changelog entry, I will take a note of this
series and we can have a discussion about adding it when we do the
next Xen release (4.17). Would that sound ok with you?

Kind regards,
Henry

> 
> Thanks, Jan
Jan Beulich April 11, 2022, 10:14 a.m. UTC | #9
On 11.04.2022 12:11, Henry Wang wrote:
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> May I ask for an ack or otherwise for the changelog entry, please?
>>
>> Ping? This is the only thing missing for me to commit the remaining
>> parts of this series.
> 
> Sorry for the late response, the previous e-mail that you directly
> "to"ed me fell through the cracks somehow.
> 
> Acked-by: Henry Wang <Henry.Wang@arm.com>

Thanks.

> Since you also mentioned the changelog entry, I will take a note of this
> series and we can have a discussion about adding it when we do the
> next Xen release (4.17). Would that sound ok with you?

"Adding it" where? Maybe you mean to the release notes, but that's not
entirely clear.

Jan
Henry Wang April 11, 2022, 10:30 a.m. UTC | #10
Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> > Since you also mentioned the changelog entry, I will take a note of this
> > series and we can have a discussion about adding it when we do the
> > next Xen release (4.17). Would that sound ok with you?
> 
> "Adding it" where? Maybe you mean to the release notes, but that's not
> entirely clear.

Yeah, I meant the release notes. Thanks for pointing out that I did not use
accurate wording.

Kind regards,
Henry

> 
> Jan
diff mbox series

Patch

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@  The format is based on [Keep a Changelog
 
 ## [unstable UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
 
+### Changed
+ - On x86 "vga=current" can now be used together with GrUB2's gfxpayload setting. Note that
+   this requires use of "multiboot2" (and "module2") as the GrUB commands loading Xen.
+
 ### Removed / support downgraded
  - dropped support for the (x86-only) "vesa-mtrr" and "vesa-remap" command line options
 
--- 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
+
+        movw    $0x4f03, %ax
+        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
+        jne     .Lsetc_done             # Doh! No linear frame buffer
+
+        movb    $1, bootsym(graphic_mode)
+        movw    %bx, bootsym(boot_vid_mode)
+        movw    %bx, bootsym(video_mode)
+
+.Lsetc_done:
+        stc
+        ret
+
 # Detect VESA modes.
 vesa_modes:
         movw    %di, %bp                # BP=original mode table end