(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
