[PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments

[PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments

[Jan Beulich]

v4 largely has some re-basing changes compared to v3, plus one new patch.
I have to admit that I find it particularly sad that the enhancement done
by patch 1 has not made 4.16. This series had been submitted well in time.

1: make "vga=current" work with graphics modes
2: obtain video info from boot loader
3: EFI: retrieve EDID
4: simplify mode_table
5: fold branches in video handling code
6: fold/replace moves in video handling code
7: LEA -> MOV in video handling code
8: fold two MOVs into an ADD

Jan

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

[Jan Beulich]

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

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

(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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[Henry Wang]

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

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

[Jan Beulich]

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

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

[Henry Wang]

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

[PATCH v4 2/8] x86/boot: obtain video info from boot loader

[Jan Beulich]

With MB2 the boot loader may provide this information, allowing us to
obtain it without needing to enter real mode (assuming we don't need to
set a new mode from "vga=", but can instead inherit the one the
bootloader may have established).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Re-base.
v3: Re-base.
v2: New.

--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -53,6 +53,7 @@ typedef unsigned int u32;
 typedef unsigned long long u64;
 typedef unsigned int size_t;
 typedef u8 uint8_t;
+typedef u16 uint16_t;
 typedef u32 uint32_t;
 typedef u64 uint64_t;
 
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -562,12 +562,18 @@ trampoline_setup:
         mov     %esi, sym_esi(xen_phys_start)
         mov     %esi, sym_esi(trampoline_xen_phys_start)
 
-        mov     sym_esi(trampoline_phys), %ecx
-
         /* Get bottom-most low-memory stack address. */
+        mov     sym_esi(trampoline_phys), %ecx
         add     $TRAMPOLINE_SPACE,%ecx
 
+#ifdef CONFIG_VIDEO
+        lea     sym_esi(boot_vid_info), %edx
+#else
+        xor     %edx, %edx
+#endif
+
         /* Save Multiboot / PVH info struct (after relocation) for later use. */
+        push    %edx                /* Boot video info to be filled from MB2. */
         push    %ecx                /* Bottom-most low-memory stack address. */
         push    %ebx                /* Multiboot / PVH information address. */
         push    %eax                /* Magic number. */
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -14,9 +14,10 @@
 
 /*
  * This entry point is entered from xen/arch/x86/boot/head.S with:
- *   - 0x4(%esp) = MAGIC,
- *   - 0x8(%esp) = INFORMATION_ADDRESS,
- *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
+ *   - 0x04(%esp) = MAGIC,
+ *   - 0x08(%esp) = INFORMATION_ADDRESS,
+ *   - 0x0c(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
+ *   - 0x10(%esp) = BOOT_VIDEO_INFO_ADDRESS.
  */
 asm (
     "    .text                         \n"
@@ -32,6 +33,39 @@ asm (
 #include "../../../include/xen/kconfig.h"
 #include <public/arch-x86/hvm/start_info.h>
 
+#ifdef CONFIG_VIDEO
+# include "video.h"
+
+/* VESA control information */
+struct __packed vesa_ctrl_info {
+    uint8_t signature[4];
+    uint16_t version;
+    uint32_t oem_name;
+    uint32_t capabilities;
+    uint32_t mode_list;
+    uint16_t mem_size;
+    /* We don't use any further fields. */
+};
+
+/* VESA 2.0 mode information */
+struct vesa_mode_info {
+    uint16_t attrib;
+    uint8_t window[14]; /* We don't use the individual fields. */
+    uint16_t bytes_per_line;
+    uint16_t width;
+    uint16_t height;
+    uint8_t cell_width;
+    uint8_t cell_height;
+    uint8_t nr_planes;
+    uint8_t depth;
+    uint8_t memory[5]; /* We don't use the individual fields. */
+    struct boot_video_colors colors;
+    uint8_t direct_color;
+    uint32_t base;
+    /* We don't use any further fields. */
+};
+#endif /* CONFIG_VIDEO */
+
 #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t *)(tag))->member)
 #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
 
@@ -146,7 +180,7 @@ static multiboot_info_t *mbi_reloc(u32 m
     return mbi_out;
 }
 
-static multiboot_info_t *mbi2_reloc(u32 mbi_in)
+static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out)
 {
     const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
     const multiboot2_memory_map_t *mmap_src;
@@ -154,6 +188,9 @@ static multiboot_info_t *mbi2_reloc(u32
     module_t *mbi_out_mods = NULL;
     memory_map_t *mmap_dst;
     multiboot_info_t *mbi_out;
+#ifdef CONFIG_VIDEO
+    struct boot_video_info *video = NULL;
+#endif
     u32 ptr;
     unsigned int i, mod_idx = 0;
 
@@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
             ++mod_idx;
             break;
 
+#ifdef CONFIG_VIDEO
+        case MULTIBOOT2_TAG_TYPE_VBE:
+            if ( video_out )
+            {
+                const struct vesa_ctrl_info *ci;
+                const struct vesa_mode_info *mi;
+
+                video = _p(video_out);
+                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
+                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
+
+                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
+                {
+                    video->capabilities = ci->capabilities;
+                    video->lfb_linelength = mi->bytes_per_line;
+                    video->lfb_width = mi->width;
+                    video->lfb_height = mi->height;
+                    video->lfb_depth = mi->depth;
+                    video->lfb_base = mi->base;
+                    video->lfb_size = ci->mem_size;
+                    video->colors = mi->colors;
+                    video->vesa_attrib = mi->attrib;
+                }
+
+                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
+                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
+            }
+            break;
+
+        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
+            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
+                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
+            {
+                video_out = 0;
+                video = NULL;
+            }
+            break;
+#endif /* CONFIG_VIDEO */
+
         case MULTIBOOT2_TAG_TYPE_END:
-            return mbi_out;
+            goto end; /* Cannot "break;" here. */
 
         default:
             break;
         }
 
+ end:
+
+#ifdef CONFIG_VIDEO
+    if ( video )
+        video->orig_video_isVGA = 0x23;
+#endif
+
     return mbi_out;
 }
 
-void * __stdcall reloc(u32 magic, u32 in, u32 trampoline)
+void *__stdcall reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
+                      uint32_t video_info)
 {
     alloc = trampoline;
 
@@ -274,7 +358,7 @@ void * __stdcall reloc(u32 magic, u32 in
         return mbi_reloc(in);
 
     case MULTIBOOT2_BOOTLOADER_MAGIC:
-        return mbi2_reloc(in);
+        return mbi2_reloc(in, video_info);
 
     case XEN_HVM_START_MAGIC_VALUE:
         if ( IS_ENABLED(CONFIG_PVH_GUEST) )
--- a/xen/arch/x86/boot/video.h
+++ b/xen/arch/x86/boot/video.h
@@ -28,4 +28,45 @@
 /* The "recalculate timings" flag */
 #define VIDEO_RECALC        0x8000
 
+#ifndef __ASSEMBLY__
+struct boot_video_info {
+    uint8_t  orig_x;             /* 0x00 */
+    uint8_t  orig_y;             /* 0x01 */
+    uint8_t  orig_video_mode;    /* 0x02 */
+    uint8_t  orig_video_cols;    /* 0x03 */
+    uint8_t  orig_video_lines;   /* 0x04 */
+    uint8_t  orig_video_isVGA;   /* 0x05 */
+    uint16_t orig_video_points;  /* 0x06 */
+
+    /* VESA graphic mode -- linear frame buffer */
+    uint32_t capabilities;       /* 0x08 */
+    uint16_t lfb_linelength;     /* 0x0c */
+    uint16_t lfb_width;          /* 0x0e */
+    uint16_t lfb_height;         /* 0x10 */
+    uint16_t lfb_depth;          /* 0x12 */
+    uint32_t lfb_base;           /* 0x14 */
+    uint32_t lfb_size;           /* 0x18 */
+    union {
+        struct {
+            uint8_t  red_size;   /* 0x1c */
+            uint8_t  red_pos;    /* 0x1d */
+            uint8_t  green_size; /* 0x1e */
+            uint8_t  green_pos;  /* 0x1f */
+            uint8_t  blue_size;  /* 0x20 */
+            uint8_t  blue_pos;   /* 0x21 */
+            uint8_t  rsvd_size;  /* 0x22 */
+            uint8_t  rsvd_pos;   /* 0x23 */
+        };
+        struct boot_video_colors {
+            uint8_t  rgbr[8];
+        } colors;
+    };
+    struct {
+        uint16_t seg;            /* 0x24 */
+        uint16_t off;            /* 0x26 */
+    } vesapm;
+    uint16_t vesa_attrib;        /* 0x28 */
+};
+#endif /* __ASSEMBLY__ */
+
 #endif /* __BOOT_VIDEO_H__ */
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -532,35 +532,7 @@ static void __init setup_max_pdx(unsigne
 static struct e820map __initdata boot_e820;
 
 #ifdef CONFIG_VIDEO
-struct boot_video_info {
-    u8  orig_x;             /* 0x00 */
-    u8  orig_y;             /* 0x01 */
-    u8  orig_video_mode;    /* 0x02 */
-    u8  orig_video_cols;    /* 0x03 */
-    u8  orig_video_lines;   /* 0x04 */
-    u8  orig_video_isVGA;   /* 0x05 */
-    u16 orig_video_points;  /* 0x06 */
-
-    /* VESA graphic mode -- linear frame buffer */
-    u32 capabilities;       /* 0x08 */
-    u16 lfb_linelength;     /* 0x0c */
-    u16 lfb_width;          /* 0x0e */
-    u16 lfb_height;         /* 0x10 */
-    u16 lfb_depth;          /* 0x12 */
-    u32 lfb_base;           /* 0x14 */
-    u32 lfb_size;           /* 0x18 */
-    u8  red_size;           /* 0x1c */
-    u8  red_pos;            /* 0x1d */
-    u8  green_size;         /* 0x1e */
-    u8  green_pos;          /* 0x1f */
-    u8  blue_size;          /* 0x20 */
-    u8  blue_pos;           /* 0x21 */
-    u8  rsvd_size;          /* 0x22 */
-    u8  rsvd_pos;           /* 0x23 */
-    u16 vesapm_seg;         /* 0x24 */
-    u16 vesapm_off;         /* 0x26 */
-    u16 vesa_attrib;        /* 0x28 */
-};
+# include "boot/video.h"
 extern struct boot_video_info boot_vid_info;
 #endif
 
--- a/xen/include/xen/multiboot2.h
+++ b/xen/include/xen/multiboot2.h
@@ -158,6 +158,59 @@ typedef struct {
     multiboot2_memory_map_t entries[];
 } multiboot2_tag_mmap_t;
 
+typedef struct
+{
+    uint32_t type;
+    uint32_t size;
+    uint16_t vbe_mode;
+    uint16_t vbe_interface_seg;
+    uint16_t vbe_interface_off;
+    uint16_t vbe_interface_len;
+    uint8_t vbe_control_info[512];
+    uint8_t vbe_mode_info[256];
+} multiboot2_tag_vbe_t;
+
+typedef struct
+{
+    uint8_t red;
+    uint8_t green;
+    uint8_t blue;
+} multiboot2_color_t;
+
+typedef struct
+{
+    uint32_t type;
+    uint32_t size;
+    uint64_t framebuffer_addr;
+    uint32_t framebuffer_pitch;
+    uint32_t framebuffer_width;
+    uint32_t framebuffer_height;
+    uint8_t framebuffer_bpp;
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_INDEXED  0
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_RGB      1
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_EGA_TEXT 2
+    uint8_t framebuffer_type;
+    uint16_t reserved;
+
+    union
+    {
+        struct
+        {
+            uint16_t framebuffer_palette_num_colors;
+            multiboot2_color_t framebuffer_palette[];
+        };
+        struct
+        {
+            uint8_t framebuffer_red_field_position;
+            uint8_t framebuffer_red_mask_size;
+            uint8_t framebuffer_green_field_position;
+            uint8_t framebuffer_green_mask_size;
+            uint8_t framebuffer_blue_field_position;
+            uint8_t framebuffer_blue_mask_size;
+        };
+    };
+} multiboot2_tag_framebuffer_t;
+
 typedef struct {
     u32 type;
     u32 size;

Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> With MB2 the boot loader may provide this information, allowing us to
> obtain it without needing to enter real mode (assuming we don't need to
> set a new mode from "vga=", but can instead inherit the one the
> bootloader may have established).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: Re-base.
> v3: Re-base.
> v2: New.
> 
> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -53,6 +53,7 @@ typedef unsigned int u32;
>  typedef unsigned long long u64;
>  typedef unsigned int size_t;
>  typedef u8 uint8_t;
> +typedef u16 uint16_t;
>  typedef u32 uint32_t;
>  typedef u64 uint64_t;
>  
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -562,12 +562,18 @@ trampoline_setup:
>          mov     %esi, sym_esi(xen_phys_start)
>          mov     %esi, sym_esi(trampoline_xen_phys_start)
>  
> -        mov     sym_esi(trampoline_phys), %ecx
> -
>          /* Get bottom-most low-memory stack address. */
> +        mov     sym_esi(trampoline_phys), %ecx
>          add     $TRAMPOLINE_SPACE,%ecx

Just for my understanding, since you are already touching the
instruction, why not switch it to a lea like you do below?

Is that because you would also like to take the opportunity to fold
the add into the lea and that would be too much of a change?

>  
> +#ifdef CONFIG_VIDEO
> +        lea     sym_esi(boot_vid_info), %edx
> +#else
> +        xor     %edx, %edx
> +#endif
> +
>          /* Save Multiboot / PVH info struct (after relocation) for later use. */
> +        push    %edx                /* Boot video info to be filled from MB2. */
>          push    %ecx                /* Bottom-most low-memory stack address. */
>          push    %ebx                /* Multiboot / PVH information address. */
>          push    %eax                /* Magic number. */
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -14,9 +14,10 @@
>  
>  /*
>   * This entry point is entered from xen/arch/x86/boot/head.S with:
> - *   - 0x4(%esp) = MAGIC,
> - *   - 0x8(%esp) = INFORMATION_ADDRESS,
> - *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> + *   - 0x04(%esp) = MAGIC,
> + *   - 0x08(%esp) = INFORMATION_ADDRESS,
> + *   - 0x0c(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> + *   - 0x10(%esp) = BOOT_VIDEO_INFO_ADDRESS.
>   */
>  asm (
>      "    .text                         \n"
> @@ -32,6 +33,39 @@ asm (
>  #include "../../../include/xen/kconfig.h"
>  #include <public/arch-x86/hvm/start_info.h>
>  
> +#ifdef CONFIG_VIDEO
> +# include "video.h"
> +
> +/* VESA control information */
> +struct __packed vesa_ctrl_info {
> +    uint8_t signature[4];
> +    uint16_t version;
> +    uint32_t oem_name;
> +    uint32_t capabilities;
> +    uint32_t mode_list;
> +    uint16_t mem_size;
> +    /* We don't use any further fields. */
> +};
> +
> +/* VESA 2.0 mode information */
> +struct vesa_mode_info {

Should we add __packed here just in case further added fields are no
longer naturally aligned? (AFAICT all field right now are aligned to
it's size so there's no need for it).

> +    uint16_t attrib;
> +    uint8_t window[14]; /* We don't use the individual fields. */
> +    uint16_t bytes_per_line;
> +    uint16_t width;
> +    uint16_t height;
> +    uint8_t cell_width;
> +    uint8_t cell_height;
> +    uint8_t nr_planes;
> +    uint8_t depth;
> +    uint8_t memory[5]; /* We don't use the individual fields. */
> +    struct boot_video_colors colors;
> +    uint8_t direct_color;
> +    uint32_t base;
> +    /* We don't use any further fields. */
> +};

Would it make sense to put those struct definitions in boot/video.h
like you do for boot_video_info?

I also wonder whether you could then hide the #ifdef CONFIG_VIDEO
check inside of the header itself.

> +#endif /* CONFIG_VIDEO */
> +
>  #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t *)(tag))->member)
>  #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
>  
> @@ -146,7 +180,7 @@ static multiboot_info_t *mbi_reloc(u32 m
>      return mbi_out;
>  }
>  
> -static multiboot_info_t *mbi2_reloc(u32 mbi_in)
> +static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out)
>  {
>      const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
>      const multiboot2_memory_map_t *mmap_src;
> @@ -154,6 +188,9 @@ static multiboot_info_t *mbi2_reloc(u32
>      module_t *mbi_out_mods = NULL;
>      memory_map_t *mmap_dst;
>      multiboot_info_t *mbi_out;
> +#ifdef CONFIG_VIDEO
> +    struct boot_video_info *video = NULL;
> +#endif
>      u32 ptr;
>      unsigned int i, mod_idx = 0;
>  
> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
>              ++mod_idx;
>              break;
>  
> +#ifdef CONFIG_VIDEO
> +        case MULTIBOOT2_TAG_TYPE_VBE:
> +            if ( video_out )
> +            {
> +                const struct vesa_ctrl_info *ci;
> +                const struct vesa_mode_info *mi;
> +
> +                video = _p(video_out);
> +                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> +                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> +
> +                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
> +                {
> +                    video->capabilities = ci->capabilities;
> +                    video->lfb_linelength = mi->bytes_per_line;
> +                    video->lfb_width = mi->width;
> +                    video->lfb_height = mi->height;
> +                    video->lfb_depth = mi->depth;
> +                    video->lfb_base = mi->base;
> +                    video->lfb_size = ci->mem_size;
> +                    video->colors = mi->colors;
> +                    video->vesa_attrib = mi->attrib;
> +                }
> +
> +                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
> +                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
> +            }
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> +            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> +                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> +            {
> +                video_out = 0;
> +                video = NULL;
> +            }

I'm confused, don't you need to store the information in the
framebuffer tag for use after relocation?

> +            break;
> +#endif /* CONFIG_VIDEO */
> +
>          case MULTIBOOT2_TAG_TYPE_END:
> -            return mbi_out;
> +            goto end; /* Cannot "break;" here. */
>  
>          default:
>              break;
>          }
>  
> + end:
> +
> +#ifdef CONFIG_VIDEO
> +    if ( video )
> +        video->orig_video_isVGA = 0x23;

I see we use this elsewhere, what's the meaning of this (magic) 0x23?

Thanks, Roger.

Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader

[Jan Beulich]

On 05.04.2022 11:35, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -562,12 +562,18 @@ trampoline_setup:
>>          mov     %esi, sym_esi(xen_phys_start)
>>          mov     %esi, sym_esi(trampoline_xen_phys_start)
>>  
>> -        mov     sym_esi(trampoline_phys), %ecx
>> -
>>          /* Get bottom-most low-memory stack address. */
>> +        mov     sym_esi(trampoline_phys), %ecx
>>          add     $TRAMPOLINE_SPACE,%ecx
> 
> Just for my understanding, since you are already touching the
> instruction, why not switch it to a lea like you do below?
> 
> Is that because you would also like to take the opportunity to fold
> the add into the lea and that would be too much of a change?

No. This MOV cannot be converted, as its source operand isn't an
immediate (or register); such a conversion would also be undesirable,
for increasing insn size. See the later patch doing conversions in
the other direction, to reduce code size. Somewhat similarly ...

>> +#ifdef CONFIG_VIDEO
>> +        lea     sym_esi(boot_vid_info), %edx

... this LEA also cannot be expressed by a single MOV.

>> @@ -32,6 +33,39 @@ asm (
>>  #include "../../../include/xen/kconfig.h"
>>  #include <public/arch-x86/hvm/start_info.h>
>>  
>> +#ifdef CONFIG_VIDEO
>> +# include "video.h"
>> +
>> +/* VESA control information */
>> +struct __packed vesa_ctrl_info {
>> +    uint8_t signature[4];
>> +    uint16_t version;
>> +    uint32_t oem_name;
>> +    uint32_t capabilities;
>> +    uint32_t mode_list;
>> +    uint16_t mem_size;
>> +    /* We don't use any further fields. */
>> +};
>> +
>> +/* VESA 2.0 mode information */
>> +struct vesa_mode_info {
> 
> Should we add __packed here just in case further added fields are no
> longer naturally aligned? (AFAICT all field right now are aligned to
> it's size so there's no need for it).

I think we should avoid __packed whenever possible.

>> +    uint16_t attrib;
>> +    uint8_t window[14]; /* We don't use the individual fields. */
>> +    uint16_t bytes_per_line;
>> +    uint16_t width;
>> +    uint16_t height;
>> +    uint8_t cell_width;
>> +    uint8_t cell_height;
>> +    uint8_t nr_planes;
>> +    uint8_t depth;
>> +    uint8_t memory[5]; /* We don't use the individual fields. */
>> +    struct boot_video_colors colors;
>> +    uint8_t direct_color;
>> +    uint32_t base;
>> +    /* We don't use any further fields. */
>> +};
> 
> Would it make sense to put those struct definitions in boot/video.h
> like you do for boot_video_info?

Personally I prefer to expose things in headers only when multiple
other files want to consume what is being declared/defined.

>> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
>>              ++mod_idx;
>>              break;
>>  
>> +#ifdef CONFIG_VIDEO
>> +        case MULTIBOOT2_TAG_TYPE_VBE:
>> +            if ( video_out )
>> +            {
>> +                const struct vesa_ctrl_info *ci;
>> +                const struct vesa_mode_info *mi;
>> +
>> +                video = _p(video_out);
>> +                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
>> +                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
>> +
>> +                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
>> +                {
>> +                    video->capabilities = ci->capabilities;
>> +                    video->lfb_linelength = mi->bytes_per_line;
>> +                    video->lfb_width = mi->width;
>> +                    video->lfb_height = mi->height;
>> +                    video->lfb_depth = mi->depth;
>> +                    video->lfb_base = mi->base;
>> +                    video->lfb_size = ci->mem_size;
>> +                    video->colors = mi->colors;
>> +                    video->vesa_attrib = mi->attrib;
>> +                }
>> +
>> +                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
>> +                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
>> +            }
>> +            break;
>> +
>> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
>> +            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
>> +                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
>> +            {
>> +                video_out = 0;
>> +                video = NULL;
>> +            }
> 
> I'm confused, don't you need to store the information in the
> framebuffer tag for use after relocation?

If there was a consumer - yes. Right now this tag is used only to
invalidate the information taken from the other tag (or to suppress
taking values from there if that other tag came later) in case the
framebuffer type doesn't match what we support.

>> +            break;
>> +#endif /* CONFIG_VIDEO */
>> +
>>          case MULTIBOOT2_TAG_TYPE_END:
>> -            return mbi_out;
>> +            goto end; /* Cannot "break;" here. */
>>  
>>          default:
>>              break;
>>          }
>>  
>> + end:
>> +
>> +#ifdef CONFIG_VIDEO
>> +    if ( video )
>> +        video->orig_video_isVGA = 0x23;
> 
> I see we use this elsewhere, what's the meaning of this (magic) 0x23?

This is a value Linux uses (also as a plain number without any #define
iirc; at least it was that way when we first inherited that value).
Short of knowing where they took it from or what meaning they associate
with the value it would be hard for us to give this a (meaningful) name
and hence use a #define. And hence it's equally hard to properly answer
your question.

Jan

Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader

[Roger Pau Monné]

On Tue, Apr 05, 2022 at 12:57:51PM +0200, Jan Beulich wrote:
> On 05.04.2022 11:35, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/boot/head.S
> >> +++ b/xen/arch/x86/boot/head.S
> >> @@ -562,12 +562,18 @@ trampoline_setup:
> >>          mov     %esi, sym_esi(xen_phys_start)
> >>          mov     %esi, sym_esi(trampoline_xen_phys_start)
> >>  
> >> -        mov     sym_esi(trampoline_phys), %ecx
> >> -
> >>          /* Get bottom-most low-memory stack address. */
> >> +        mov     sym_esi(trampoline_phys), %ecx
> >>          add     $TRAMPOLINE_SPACE,%ecx
> > 
> > Just for my understanding, since you are already touching the
> > instruction, why not switch it to a lea like you do below?
> > 
> > Is that because you would also like to take the opportunity to fold
> > the add into the lea and that would be too much of a change?
> 
> No. This MOV cannot be converted, as its source operand isn't an
> immediate (or register); such a conversion would also be undesirable,
> for increasing insn size. See the later patch doing conversions in
> the other direction, to reduce code size. Somewhat similarly ...
> 
> >> +#ifdef CONFIG_VIDEO
> >> +        lea     sym_esi(boot_vid_info), %edx
> 
> ... this LEA also cannot be expressed by a single MOV.
> 
> >> @@ -32,6 +33,39 @@ asm (
> >>  #include "../../../include/xen/kconfig.h"
> >>  #include <public/arch-x86/hvm/start_info.h>
> >>  
> >> +#ifdef CONFIG_VIDEO
> >> +# include "video.h"
> >> +
> >> +/* VESA control information */
> >> +struct __packed vesa_ctrl_info {
> >> +    uint8_t signature[4];
> >> +    uint16_t version;
> >> +    uint32_t oem_name;
> >> +    uint32_t capabilities;
> >> +    uint32_t mode_list;
> >> +    uint16_t mem_size;
> >> +    /* We don't use any further fields. */
> >> +};
> >> +
> >> +/* VESA 2.0 mode information */
> >> +struct vesa_mode_info {
> > 
> > Should we add __packed here just in case further added fields are no
> > longer naturally aligned? (AFAICT all field right now are aligned to
> > it's size so there's no need for it).
> 
> I think we should avoid __packed whenever possible.
> 
> >> +    uint16_t attrib;
> >> +    uint8_t window[14]; /* We don't use the individual fields. */
> >> +    uint16_t bytes_per_line;
> >> +    uint16_t width;
> >> +    uint16_t height;
> >> +    uint8_t cell_width;
> >> +    uint8_t cell_height;
> >> +    uint8_t nr_planes;
> >> +    uint8_t depth;
> >> +    uint8_t memory[5]; /* We don't use the individual fields. */
> >> +    struct boot_video_colors colors;
> >> +    uint8_t direct_color;
> >> +    uint32_t base;
> >> +    /* We don't use any further fields. */
> >> +};
> > 
> > Would it make sense to put those struct definitions in boot/video.h
> > like you do for boot_video_info?
> 
> Personally I prefer to expose things in headers only when multiple
> other files want to consume what is being declared/defined.
> 
> >> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
> >>              ++mod_idx;
> >>              break;
> >>  
> >> +#ifdef CONFIG_VIDEO
> >> +        case MULTIBOOT2_TAG_TYPE_VBE:
> >> +            if ( video_out )
> >> +            {
> >> +                const struct vesa_ctrl_info *ci;
> >> +                const struct vesa_mode_info *mi;
> >> +
> >> +                video = _p(video_out);
> >> +                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> >> +                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> >> +
> >> +                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
> >> +                {
> >> +                    video->capabilities = ci->capabilities;
> >> +                    video->lfb_linelength = mi->bytes_per_line;
> >> +                    video->lfb_width = mi->width;
> >> +                    video->lfb_height = mi->height;
> >> +                    video->lfb_depth = mi->depth;
> >> +                    video->lfb_base = mi->base;
> >> +                    video->lfb_size = ci->mem_size;
> >> +                    video->colors = mi->colors;
> >> +                    video->vesa_attrib = mi->attrib;
> >> +                }
> >> +
> >> +                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
> >> +                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
> >> +            }
> >> +            break;
> >> +
> >> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> >> +            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> >> +                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> >> +            {
> >> +                video_out = 0;
> >> +                video = NULL;
> >> +            }
> > 
> > I'm confused, don't you need to store the information in the
> > framebuffer tag for use after relocation?
> 
> If there was a consumer - yes. Right now this tag is used only to
> invalidate the information taken from the other tag (or to suppress
> taking values from there if that other tag came later) in case the
> framebuffer type doesn't match what we support.
> 
> >> +            break;
> >> +#endif /* CONFIG_VIDEO */
> >> +
> >>          case MULTIBOOT2_TAG_TYPE_END:
> >> -            return mbi_out;
> >> +            goto end; /* Cannot "break;" here. */
> >>  
> >>          default:
> >>              break;
> >>          }
> >>  
> >> + end:
> >> +
> >> +#ifdef CONFIG_VIDEO
> >> +    if ( video )
> >> +        video->orig_video_isVGA = 0x23;
> > 
> > I see we use this elsewhere, what's the meaning of this (magic) 0x23?
> 
> This is a value Linux uses (also as a plain number without any #define
> iirc; at least it was that way when we first inherited that value).
> Short of knowing where they took it from or what meaning they associate
> with the value it would be hard for us to give this a (meaningful) name
> and hence use a #define. And hence it's equally hard to properly answer
> your question.

OK, so it's a magic value. I'm happy with the rest, so:

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

Thanks, Roger.

[PATCH v4 3/8] x86/EFI: retrieve EDID

[Jan Beulich]

When booting directly from EFI, obtaining this information from EFI is
the only possible way. And even when booting with a boot loader
interposed, it's more clean not to use legacy BIOS calls for this
purpose. (The downside being that there are no "capabilities" that we
can retrieve the EFI way.)

To achieve this we need to propagate the handle used to obtain the
EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
one or both of the two low bits also doesn't seem appropriate.

GrUB also checks an "agp-internal-edid" variable. As I haven't been able
to find any related documentation, and as GrUB being happy about the
variable being any size (rather than at least / precisely 128 bytes),
I didn't follow that route.
---
v3: Re-base.
v2: New.

--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
 {
 }
 
+static void __init efi_arch_edid(EFI_HANDLE gop_handle)
+{
+}
+
 static void __init efi_arch_memory_setup(void)
 {
 }
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -922,7 +922,14 @@ store_edid:
         pushw   %dx
         pushw   %di
 
-        cmpb    $1, bootsym(opt_edid)   # EDID disabled on cmdline (edid=no)?
+        movb    bootsym(opt_edid), %al
+        cmpw    $0x1313, bootsym(boot_edid_caps) # Data already retrieved?
+        je      .Lcheck_edid
+        cmpb    $2, %al                 # EDID forced on cmdline (edid=force)?
+        jne     .Lno_edid
+
+.Lcheck_edid:
+        cmpb    $1, %al                 # EDID disabled on cmdline (edid=no)?
         je      .Lno_edid
 
         leaw    vesa_glob_info, %di
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
 #endif
 }
 
+#ifdef CONFIG_VIDEO
+static bool __init copy_edid(const void *buf, unsigned int size)
+{
+    /*
+     * Be conservative - for both undersized and oversized blobs it is unclear
+     * what to actually do with them. The more that unlike the VESA BIOS
+     * interface we also have no associated "capabilities" value (which might
+     * carry a hint as to possible interpretation).
+     */
+    if ( size != ARRAY_SIZE(boot_edid_info) )
+        return false;
+
+    memcpy(boot_edid_info, buf, size);
+    boot_edid_caps = 0;
+
+    return true;
+}
+#endif
+
+static void __init efi_arch_edid(EFI_HANDLE gop_handle)
+{
+#ifdef CONFIG_VIDEO
+    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
+    static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
+    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
+    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
+    EFI_STATUS status;
+
+    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
+                                  (void **)&active_edid, efi_ih, NULL,
+                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+    if ( status == EFI_SUCCESS &&
+         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
+        return;
+
+    status = efi_bs->OpenProtocol(gop_handle, &discovered_guid,
+                                  (void **)&discovered_edid, efi_ih, NULL,
+                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+    if ( status == EFI_SUCCESS )
+        copy_edid(discovered_edid->Edid, discovered_edid->SizeOfEdid);
+#endif
+}
+
 static void __init efi_arch_memory_setup(void)
 {
     unsigned int i;
@@ -729,6 +772,7 @@ static void __init efi_arch_flush_dcache
 void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
+    EFI_HANDLE gop_handle;
     UINTN cols, gop_mode = ~0, rows;
 
     __set_bit(EFI_BOOT, &efi_flags);
@@ -742,11 +786,15 @@ void __init efi_multiboot2(EFI_HANDLE Im
                            &cols, &rows) == EFI_SUCCESS )
         efi_arch_console_init(cols, rows);
 
-    gop = efi_get_gop();
+    gop = efi_get_gop(&gop_handle);
 
     if ( gop )
+    {
         gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
 
+        efi_arch_edid(gop_handle);
+    }
+
     efi_arch_edd();
     efi_arch_cpu();
 
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -118,7 +118,7 @@ static bool read_section(const EFI_LOADE
 
 static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
 static void efi_console_set_mode(void);
-static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
+static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(EFI_HANDLE *gop_handle);
 static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
                                UINTN cols, UINTN rows, UINTN depth);
 static void efi_tables(void);
@@ -758,7 +758,7 @@ static void __init efi_console_set_mode(
         StdOut->SetMode(StdOut, best);
 }
 
-static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
+static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(EFI_HANDLE *gop_handle)
 {
     EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
@@ -783,7 +783,10 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL __in
             continue;
         status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
         if ( !EFI_ERROR(status) )
+        {
+            *gop_handle = handles[i];
             break;
+        }
     }
     if ( handles )
         efi_bs->FreePool(handles);
@@ -1222,6 +1225,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
     if ( use_cfg_file )
     {
         EFI_FILE_HANDLE dir_handle;
+        EFI_HANDLE gop_handle;
         UINTN depth, cols, rows, size;
 
         size = cols = rows = depth = 0;
@@ -1230,7 +1234,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
                                &cols, &rows) == EFI_SUCCESS )
             efi_arch_console_init(cols, rows);
 
-        gop = efi_get_gop();
+        gop = efi_get_gop(&gop_handle);
 
         /* Get the file system interface. */
         dir_handle = get_parent_handle(loaded_image, &file_name);
@@ -1360,7 +1364,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
         dir_handle->Close(dir_handle);
 
         if ( gop && !base_video )
+        {
             gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
+
+            efi_arch_edid(gop_handle);
+        }
     }
 
     /* Get the number of boot modules specified on the DT or an error (<0) */
@@ -1387,7 +1395,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
 
     efi_arch_edd();
 
-    /* XXX Collect EDID info. */
     efi_arch_cpu();
 
     efi_tables();
--- a/xen/include/efi/efiprot.h
+++ b/xen/include/efi/efiprot.h
@@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
   EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT         Blt;
   EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE        *Mode;
 };
+
+/*
+ * EFI EDID Discovered Protocol
+ * UEFI Specification Version 2.5 Section 11.9
+ */
+#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
+    { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 0x66, 0xAA} }
+
+typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
+    UINT32   SizeOfEdid;
+    UINT8   *Edid;
+} EFI_EDID_DISCOVERED_PROTOCOL;
+
+/*
+ * EFI EDID Active Protocol
+ * UEFI Specification Version 2.5 Section 11.9
+ */
+#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
+    { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 0x79, 0x86} }
+
+typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
+    UINT32   SizeOfEdid;
+    UINT8   *Edid;
+} EFI_EDID_ACTIVE_PROTOCOL;
+
+/*
+ * EFI EDID Override Protocol
+ * UEFI Specification Version 2.5 Section 11.9
+ */
+#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
+    { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 0x0B, 0xD5} }
+
+INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
+
+typedef
+EFI_STATUS
+(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
+  IN      struct _EFI_EDID_OVERRIDE_PROTOCOL   *This,
+  IN      EFI_HANDLE                           *ChildHandle,
+  OUT     UINT32                               *Attributes,
+  IN OUT  UINTN                                *EdidSize,
+  IN OUT  UINT8                               **Edid);
+
+typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
+    EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID  GetEdid;
+} EFI_EDID_OVERRIDE_PROTOCOL;
+
 #endif
 

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Luca Fancellu]

> On 31 Mar 2022, at 10:45, Jan Beulich <jbeulich@suse.com> wrote:
> 
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
> 
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Hi Jan,

For the arm and common part, the changes looks good to me.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

I tested also the whole serie in the sense that it boots properly on arm,
unfortunately I could not test the functionality.

Cheers,
Luca

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
> 
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
> one or both of the two low bits also doesn't seem appropriate.
> 
> GrUB also checks an "agp-internal-edid" variable. As I haven't been able
> to find any related documentation, and as GrUB being happy about the
> variable being any size (rather than at least / precisely 128 bytes),
> I didn't follow that route.
> ---
> v3: Re-base.
> v2: New.
> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
>  {
>  }
>  
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +}
> +
>  static void __init efi_arch_memory_setup(void)
>  {
>  }
> --- a/xen/arch/x86/boot/video.S
> +++ b/xen/arch/x86/boot/video.S
> @@ -922,7 +922,14 @@ store_edid:
>          pushw   %dx
>          pushw   %di
>  
> -        cmpb    $1, bootsym(opt_edid)   # EDID disabled on cmdline (edid=no)?
> +        movb    bootsym(opt_edid), %al
> +        cmpw    $0x1313, bootsym(boot_edid_caps) # Data already retrieved?
> +        je      .Lcheck_edid
> +        cmpb    $2, %al                 # EDID forced on cmdline (edid=force)?
> +        jne     .Lno_edid
> +
> +.Lcheck_edid:
> +        cmpb    $1, %al                 # EDID disabled on cmdline (edid=no)?
>          je      .Lno_edid
>  
>          leaw    vesa_glob_info, %di
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
>  #endif
>  }
>  
> +#ifdef CONFIG_VIDEO
> +static bool __init copy_edid(const void *buf, unsigned int size)
> +{
> +    /*
> +     * Be conservative - for both undersized and oversized blobs it is unclear
> +     * what to actually do with them. The more that unlike the VESA BIOS
> +     * interface we also have no associated "capabilities" value (which might
> +     * carry a hint as to possible interpretation).
> +     */
> +    if ( size != ARRAY_SIZE(boot_edid_info) )
> +        return false;
> +
> +    memcpy(boot_edid_info, buf, size);
> +    boot_edid_caps = 0;
> +
> +    return true;
> +}
> +#endif
> +
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +#ifdef CONFIG_VIDEO
> +    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> +    static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;

Is there a need to make those static?

I think this function is either called from efi_start or
efi_multiboot, but there aren't multiple calls to it? (also both
parameters are IN only, so not to be changed by the EFI method?

I have the feeling setting them to static is done because they can't
be set to const?

> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> +    EFI_STATUS status;
> +
> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> +                                  (void **)&active_edid, efi_ih, NULL,
> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +    if ( status == EFI_SUCCESS &&
> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> +        return;

Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?

From my reading of the UEFI spec this will either return
EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
ignoring EFI_EDID_OVERRIDE_PROTOCOL?

> +    status = efi_bs->OpenProtocol(gop_handle, &discovered_guid,
> +                                  (void **)&discovered_edid, efi_ih, NULL,
> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +    if ( status == EFI_SUCCESS )
> +        copy_edid(discovered_edid->Edid, discovered_edid->SizeOfEdid);
> +#endif
> +}
> +
>  static void __init efi_arch_memory_setup(void)
>  {
>      unsigned int i;
> @@ -729,6 +772,7 @@ static void __init efi_arch_flush_dcache
>  void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  {
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> +    EFI_HANDLE gop_handle;
>      UINTN cols, gop_mode = ~0, rows;
>  
>      __set_bit(EFI_BOOT, &efi_flags);
> @@ -742,11 +786,15 @@ void __init efi_multiboot2(EFI_HANDLE Im
>                             &cols, &rows) == EFI_SUCCESS )
>          efi_arch_console_init(cols, rows);
>  
> -    gop = efi_get_gop();
> +    gop = efi_get_gop(&gop_handle);
>  
>      if ( gop )
> +    {
>          gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
>  
> +        efi_arch_edid(gop_handle);
> +    }
> +
>      efi_arch_edd();
>      efi_arch_cpu();
>  
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -118,7 +118,7 @@ static bool read_section(const EFI_LOADE
>  
>  static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
>  static void efi_console_set_mode(void);
> -static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(EFI_HANDLE *gop_handle);
>  static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>                                 UINTN cols, UINTN rows, UINTN depth);
>  static void efi_tables(void);
> @@ -758,7 +758,7 @@ static void __init efi_console_set_mode(
>          StdOut->SetMode(StdOut, best);
>  }
>  
> -static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(EFI_HANDLE *gop_handle)
>  {
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
> @@ -783,7 +783,10 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL __in
>              continue;
>          status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
>          if ( !EFI_ERROR(status) )
> +        {
> +            *gop_handle = handles[i];
>              break;
> +        }
>      }
>      if ( handles )
>          efi_bs->FreePool(handles);
> @@ -1222,6 +1225,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>      if ( use_cfg_file )
>      {
>          EFI_FILE_HANDLE dir_handle;
> +        EFI_HANDLE gop_handle;
>          UINTN depth, cols, rows, size;
>  
>          size = cols = rows = depth = 0;
> @@ -1230,7 +1234,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>                                 &cols, &rows) == EFI_SUCCESS )
>              efi_arch_console_init(cols, rows);
>  
> -        gop = efi_get_gop();
> +        gop = efi_get_gop(&gop_handle);
>  
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);
> @@ -1360,7 +1364,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>          dir_handle->Close(dir_handle);
>  
>          if ( gop && !base_video )
> +        {
>              gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
> +
> +            efi_arch_edid(gop_handle);
> +        }
>      }
>  
>      /* Get the number of boot modules specified on the DT or an error (<0) */
> @@ -1387,7 +1395,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>  
>      efi_arch_edd();
>  
> -    /* XXX Collect EDID info. */
>      efi_arch_cpu();
>  
>      efi_tables();
> --- a/xen/include/efi/efiprot.h
> +++ b/xen/include/efi/efiprot.h
> @@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
>    EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT         Blt;
>    EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE        *Mode;
>  };
> +
> +/*
> + * EFI EDID Discovered Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
> +    { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 0x66, 0xAA} }
> +
> +typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
> +    UINT32   SizeOfEdid;
> +    UINT8   *Edid;
> +} EFI_EDID_DISCOVERED_PROTOCOL;
> +
> +/*
> + * EFI EDID Active Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
> +    { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 0x79, 0x86} }
> +
> +typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
> +    UINT32   SizeOfEdid;
> +    UINT8   *Edid;
> +} EFI_EDID_ACTIVE_PROTOCOL;
> +
> +/*
> + * EFI EDID Override Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
> +    { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 0x0B, 0xD5} }
> +
> +INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
> +  IN      struct _EFI_EDID_OVERRIDE_PROTOCOL   *This,
> +  IN      EFI_HANDLE                           *ChildHandle,
> +  OUT     UINT32                               *Attributes,
> +  IN OUT  UINTN                                *EdidSize,
> +  IN OUT  UINT8                               **Edid);
> +
> +typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
> +    EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID  GetEdid;
> +} EFI_EDID_OVERRIDE_PROTOCOL;
> +
>  #endif

FWIW, EFI_EDID_OVERRIDE_PROTOCOL_GUID is not used by the patch, so I
guess it's introduced for completeness (or because it's used by
further patches).

Thanks, Roger.

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Jan Beulich]

On 05.04.2022 12:27, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
>>  #endif
>>  }
>>  
>> +#ifdef CONFIG_VIDEO
>> +static bool __init copy_edid(const void *buf, unsigned int size)
>> +{
>> +    /*
>> +     * Be conservative - for both undersized and oversized blobs it is unclear
>> +     * what to actually do with them. The more that unlike the VESA BIOS
>> +     * interface we also have no associated "capabilities" value (which might
>> +     * carry a hint as to possible interpretation).
>> +     */
>> +    if ( size != ARRAY_SIZE(boot_edid_info) )
>> +        return false;
>> +
>> +    memcpy(boot_edid_info, buf, size);
>> +    boot_edid_caps = 0;
>> +
>> +    return true;
>> +}
>> +#endif
>> +
>> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
>> +{
>> +#ifdef CONFIG_VIDEO
>> +    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
>> +    static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
> 
> Is there a need to make those static?
> 
> I think this function is either called from efi_start or
> efi_multiboot, but there aren't multiple calls to it? (also both
> parameters are IN only, so not to be changed by the EFI method?
> 
> I have the feeling setting them to static is done because they can't
> be set to const?

Even if they could be const, they ought to also be static. They don't
strictly need to be, but without "static" code will be generated to
populate the on-stack variables; quite possibly the compiler would
even allocate an unnamed static variable and memcpy() from there onto
the stack.

>> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>> +    EFI_STATUS status;
>> +
>> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>> +                                  (void **)&active_edid, efi_ih, NULL,
>> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>> +    if ( status == EFI_SUCCESS &&
>> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>> +        return;
> 
> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> 
> From my reading of the UEFI spec this will either return
> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> ignoring EFI_EDID_OVERRIDE_PROTOCOL?

That's the theory. As per one of the post-commit-message remarks I had
looked at what GrUB does, and I decided to follow its behavior in this
regard, assuming they do what they do to work around quirks. As said
in the remark, I didn't want to go as far as also cloning their use of
the undocumented (afaik) "agp-internal-edid" variable.

>> --- a/xen/include/efi/efiprot.h
>> +++ b/xen/include/efi/efiprot.h
>> @@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
>>    EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT         Blt;
>>    EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE        *Mode;
>>  };
>> +
>> +/*
>> + * EFI EDID Discovered Protocol
>> + * UEFI Specification Version 2.5 Section 11.9
>> + */
>> +#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
>> +    { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 0x66, 0xAA} }
>> +
>> +typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
>> +    UINT32   SizeOfEdid;
>> +    UINT8   *Edid;
>> +} EFI_EDID_DISCOVERED_PROTOCOL;
>> +
>> +/*
>> + * EFI EDID Active Protocol
>> + * UEFI Specification Version 2.5 Section 11.9
>> + */
>> +#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
>> +    { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 0x79, 0x86} }
>> +
>> +typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
>> +    UINT32   SizeOfEdid;
>> +    UINT8   *Edid;
>> +} EFI_EDID_ACTIVE_PROTOCOL;
>> +
>> +/*
>> + * EFI EDID Override Protocol
>> + * UEFI Specification Version 2.5 Section 11.9
>> + */
>> +#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
>> +    { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 0x0B, 0xD5} }
>> +
>> +INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
>> +
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
>> +  IN      struct _EFI_EDID_OVERRIDE_PROTOCOL   *This,
>> +  IN      EFI_HANDLE                           *ChildHandle,
>> +  OUT     UINT32                               *Attributes,
>> +  IN OUT  UINTN                                *EdidSize,
>> +  IN OUT  UINT8                               **Edid);
>> +
>> +typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
>> +    EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID  GetEdid;
>> +} EFI_EDID_OVERRIDE_PROTOCOL;
>> +
>>  #endif
> 
> FWIW, EFI_EDID_OVERRIDE_PROTOCOL_GUID is not used by the patch, so I
> guess it's introduced for completeness (or because it's used by
> further patches).

Indeed (the former; there's no use in later patches).

Jan

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Roger Pau Monné]

On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
> On 05.04.2022 12:27, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/efi/efi-boot.h
> >> +++ b/xen/arch/x86/efi/efi-boot.h
> >> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
> >>  #endif
> >>  }
> >>  
> >> +#ifdef CONFIG_VIDEO
> >> +static bool __init copy_edid(const void *buf, unsigned int size)
> >> +{
> >> +    /*
> >> +     * Be conservative - for both undersized and oversized blobs it is unclear
> >> +     * what to actually do with them. The more that unlike the VESA BIOS
> >> +     * interface we also have no associated "capabilities" value (which might
> >> +     * carry a hint as to possible interpretation).
> >> +     */
> >> +    if ( size != ARRAY_SIZE(boot_edid_info) )
> >> +        return false;
> >> +
> >> +    memcpy(boot_edid_info, buf, size);
> >> +    boot_edid_caps = 0;
> >> +
> >> +    return true;
> >> +}
> >> +#endif
> >> +
> >> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> >> +{
> >> +#ifdef CONFIG_VIDEO
> >> +    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> >> +    static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
> > 
> > Is there a need to make those static?
> > 
> > I think this function is either called from efi_start or
> > efi_multiboot, but there aren't multiple calls to it? (also both
> > parameters are IN only, so not to be changed by the EFI method?
> > 
> > I have the feeling setting them to static is done because they can't
> > be set to const?
> 
> Even if they could be const, they ought to also be static. They don't
> strictly need to be, but without "static" code will be generated to
> populate the on-stack variables; quite possibly the compiler would
> even allocate an unnamed static variable and memcpy() from there onto
> the stack.

I thought that making those const (and then annotate with __initconst)
would already have the same effect as having it static, as there will
be no memcpy in that case either.

> >> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> >> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> >> +    EFI_STATUS status;
> >> +
> >> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> >> +                                  (void **)&active_edid, efi_ih, NULL,
> >> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >> +    if ( status == EFI_SUCCESS &&
> >> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> >> +        return;
> > 
> > Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> > 
> > From my reading of the UEFI spec this will either return
> > EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> > If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> > falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> > EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> > ignoring EFI_EDID_OVERRIDE_PROTOCOL?
> 
> That's the theory. As per one of the post-commit-message remarks I had
> looked at what GrUB does, and I decided to follow its behavior in this
> regard, assuming they do what they do to work around quirks. As said
> in the remark, I didn't want to go as far as also cloning their use of
> the undocumented (afaik) "agp-internal-edid" variable.

Could you add this as a comment here? So it's not lost on commit as
being just a post-commit log remark. With that:

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

Thanks, Roger.

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Jan Beulich]

On 05.04.2022 17:47, Roger Pau Monné wrote:
> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>> On 05.04.2022 12:27, Roger Pau Monné wrote:
>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
>>>>  #endif
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_VIDEO
>>>> +static bool __init copy_edid(const void *buf, unsigned int size)
>>>> +{
>>>> +    /*
>>>> +     * Be conservative - for both undersized and oversized blobs it is unclear
>>>> +     * what to actually do with them. The more that unlike the VESA BIOS
>>>> +     * interface we also have no associated "capabilities" value (which might
>>>> +     * carry a hint as to possible interpretation).
>>>> +     */
>>>> +    if ( size != ARRAY_SIZE(boot_edid_info) )
>>>> +        return false;
>>>> +
>>>> +    memcpy(boot_edid_info, buf, size);
>>>> +    boot_edid_caps = 0;
>>>> +
>>>> +    return true;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
>>>> +{
>>>> +#ifdef CONFIG_VIDEO
>>>> +    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
>>>> +    static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
>>>
>>> Is there a need to make those static?
>>>
>>> I think this function is either called from efi_start or
>>> efi_multiboot, but there aren't multiple calls to it? (also both
>>> parameters are IN only, so not to be changed by the EFI method?
>>>
>>> I have the feeling setting them to static is done because they can't
>>> be set to const?
>>
>> Even if they could be const, they ought to also be static. They don't
>> strictly need to be, but without "static" code will be generated to
>> populate the on-stack variables; quite possibly the compiler would
>> even allocate an unnamed static variable and memcpy() from there onto
>> the stack.
> 
> I thought that making those const (and then annotate with __initconst)
> would already have the same effect as having it static, as there will
> be no memcpy in that case either.

You cannot annotate non-static variables with __initconst.

>>>> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>>>> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>>>> +    EFI_STATUS status;
>>>> +
>>>> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>>>> +                                  (void **)&active_edid, efi_ih, NULL,
>>>> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>> +    if ( status == EFI_SUCCESS &&
>>>> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>>>> +        return;
>>>
>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>>>
>>> From my reading of the UEFI spec this will either return
>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>>
>> That's the theory. As per one of the post-commit-message remarks I had
>> looked at what GrUB does, and I decided to follow its behavior in this
>> regard, assuming they do what they do to work around quirks. As said
>> in the remark, I didn't want to go as far as also cloning their use of
>> the undocumented (afaik) "agp-internal-edid" variable.

Actually it's a little different, as I realized while re-checking in
order to reply to your request below. While GrUB looks to use this
only "just in case", our use is actually to also cope with failure
in copy_edid(): In case the overridden EDID doesn't match the size
constraint (which is more strict than GrUB's), we would retry with
the "discovered" one in the hope that its size is okay.

> Could you add this as a comment here? So it's not lost on commit as
> being just a post-commit log remark.

As a result I'm unsure of the value of a comment here going beyond
what the comment in copy_edid() already says. For now I've added

    /*
     * In case an override is in place which doesn't fit copy_edid(), also try
     * obtaining the discovered EDID in the hope that it's better than nothing.
     */

In turn ...

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

... I'll wait with applying this (thanks) until we've reached
agreement.

Jan

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
> On 05.04.2022 17:47, Roger Pau Monné wrote:
> > On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
> >> On 05.04.2022 12:27, Roger Pau Monné wrote:
> >>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/efi/efi-boot.h
> >>>> +++ b/xen/arch/x86/efi/efi-boot.h
> >>>> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
> >>>>  #endif
> >>>>  }
> >>>>  
> >>>> +#ifdef CONFIG_VIDEO
> >>>> +static bool __init copy_edid(const void *buf, unsigned int size)
> >>>> +{
> >>>> +    /*
> >>>> +     * Be conservative - for both undersized and oversized blobs it is unclear
> >>>> +     * what to actually do with them. The more that unlike the VESA BIOS
> >>>> +     * interface we also have no associated "capabilities" value (which might
> >>>> +     * carry a hint as to possible interpretation).
> >>>> +     */
> >>>> +    if ( size != ARRAY_SIZE(boot_edid_info) )
> >>>> +        return false;
> >>>> +
> >>>> +    memcpy(boot_edid_info, buf, size);
> >>>> +    boot_edid_caps = 0;
> >>>> +
> >>>> +    return true;
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> >>>> +{
> >>>> +#ifdef CONFIG_VIDEO
> >>>> +    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> >>>> +    static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
> >>>
> >>> Is there a need to make those static?
> >>>
> >>> I think this function is either called from efi_start or
> >>> efi_multiboot, but there aren't multiple calls to it? (also both
> >>> parameters are IN only, so not to be changed by the EFI method?
> >>>
> >>> I have the feeling setting them to static is done because they can't
> >>> be set to const?
> >>
> >> Even if they could be const, they ought to also be static. They don't
> >> strictly need to be, but without "static" code will be generated to
> >> populate the on-stack variables; quite possibly the compiler would
> >> even allocate an unnamed static variable and memcpy() from there onto
> >> the stack.
> > 
> > I thought that making those const (and then annotate with __initconst)
> > would already have the same effect as having it static, as there will
> > be no memcpy in that case either.
> 
> You cannot annotate non-static variables with __initconst.

Oh, I guess I've never realized.

> >>>> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> >>>> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> >>>> +    EFI_STATUS status;
> >>>> +
> >>>> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> >>>> +                                  (void **)&active_edid, efi_ih, NULL,
> >>>> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>>> +    if ( status == EFI_SUCCESS &&
> >>>> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> >>>> +        return;
> >>>
> >>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> >>>
> >>> From my reading of the UEFI spec this will either return
> >>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> >>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> >>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> >>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> >>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
> >>
> >> That's the theory. As per one of the post-commit-message remarks I had
> >> looked at what GrUB does, and I decided to follow its behavior in this
> >> regard, assuming they do what they do to work around quirks. As said
> >> in the remark, I didn't want to go as far as also cloning their use of
> >> the undocumented (afaik) "agp-internal-edid" variable.
> 
> Actually it's a little different, as I realized while re-checking in
> order to reply to your request below. While GrUB looks to use this
> only "just in case", our use is actually to also cope with failure
> in copy_edid(): In case the overridden EDID doesn't match the size
> constraint (which is more strict than GrUB's), we would retry with
> the "discovered" one in the hope that its size is okay.

Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:

"Returns EDID values and attributes that the Video BIOS must use"

And since EFI_EDID_ACTIVE_PROTOCOL will return
EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
the call itself failing, but Xen failing to parse the result (because
of the usage of must in the sentence).

I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
succeeds but it's Xen the one failing to parse the result.

> > Could you add this as a comment here? So it's not lost on commit as
> > being just a post-commit log remark.
> 
> As a result I'm unsure of the value of a comment here going beyond
> what the comment in copy_edid() already says. For now I've added
> 
>     /*
>      * In case an override is in place which doesn't fit copy_edid(), also try
>      * obtaining the discovered EDID in the hope that it's better than nothing.
>      */

I think the comment is fine, but as mentioned above I wonder whether
by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
system more than by simply failing to get video output, hence I
think a more conservative approach might be to just use
EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.

As with firmware, this should mostly mimic what others do in order to
be on the safe side, so if GrUB does so we should likely follow suit.

Thanks, Roger.

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Jan Beulich]

On 06.04.2022 11:34, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
>> On 05.04.2022 17:47, Roger Pau Monné wrote:
>>> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>>>> On 05.04.2022 12:27, Roger Pau Monné wrote:
>>>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>>>>>> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>>>>>> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>>>>>> +    EFI_STATUS status;
>>>>>> +
>>>>>> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>>>>>> +                                  (void **)&active_edid, efi_ih, NULL,
>>>>>> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>>> +    if ( status == EFI_SUCCESS &&
>>>>>> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>>>>>> +        return;
>>>>>
>>>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>>>>>
>>>>> From my reading of the UEFI spec this will either return
>>>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
>>>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
>>>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
>>>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
>>>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>>>>
>>>> That's the theory. As per one of the post-commit-message remarks I had
>>>> looked at what GrUB does, and I decided to follow its behavior in this
>>>> regard, assuming they do what they do to work around quirks. As said
>>>> in the remark, I didn't want to go as far as also cloning their use of
>>>> the undocumented (afaik) "agp-internal-edid" variable.
>>
>> Actually it's a little different, as I realized while re-checking in
>> order to reply to your request below. While GrUB looks to use this
>> only "just in case", our use is actually to also cope with failure
>> in copy_edid(): In case the overridden EDID doesn't match the size
>> constraint (which is more strict than GrUB's), we would retry with
>> the "discovered" one in the hope that its size is okay.
> 
> Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
> 
> "Returns EDID values and attributes that the Video BIOS must use"

I'm tempted to say "We're not the Video BIOS." ;-)

> And since EFI_EDID_ACTIVE_PROTOCOL will return
> EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
> fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
> the call itself failing, but Xen failing to parse the result (because
> of the usage of must in the sentence).
> 
> I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
> EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
> succeeds but it's Xen the one failing to parse the result.
> 
>>> Could you add this as a comment here? So it's not lost on commit as
>>> being just a post-commit log remark.
>>
>> As a result I'm unsure of the value of a comment here going beyond
>> what the comment in copy_edid() already says. For now I've added
>>
>>     /*
>>      * In case an override is in place which doesn't fit copy_edid(), also try
>>      * obtaining the discovered EDID in the hope that it's better than nothing.
>>      */
> 
> I think the comment is fine, but as mentioned above I wonder whether
> by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
> resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
> system more than by simply failing to get video output, hence I
> think a more conservative approach might be to just use
> EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
> 
> As with firmware, this should mostly mimic what others do in order to
> be on the safe side, so if GrUB does so we should likely follow suit.

But they're careless in other ways; I don't want to mimic that. I would
assume (or at least hope) that a discovered EDID still fits the system,
perhaps not as optimally as a subsequently installed override. But then
I also lack sufficient knowledge on all aspects which EDID may be
relevant for, so it's all guesswork anyway afaic.

Jan

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 02:40:50PM +0200, Jan Beulich wrote:
> On 06.04.2022 11:34, Roger Pau Monné wrote:
> > On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
> >> On 05.04.2022 17:47, Roger Pau Monné wrote:
> >>> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
> >>>> On 05.04.2022 12:27, Roger Pau Monné wrote:
> >>>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> >>>>>> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> >>>>>> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> >>>>>> +    EFI_STATUS status;
> >>>>>> +
> >>>>>> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> >>>>>> +                                  (void **)&active_edid, efi_ih, NULL,
> >>>>>> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>>>>> +    if ( status == EFI_SUCCESS &&
> >>>>>> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> >>>>>> +        return;
> >>>>>
> >>>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> >>>>>
> >>>>> From my reading of the UEFI spec this will either return
> >>>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> >>>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> >>>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> >>>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> >>>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
> >>>>
> >>>> That's the theory. As per one of the post-commit-message remarks I had
> >>>> looked at what GrUB does, and I decided to follow its behavior in this
> >>>> regard, assuming they do what they do to work around quirks. As said
> >>>> in the remark, I didn't want to go as far as also cloning their use of
> >>>> the undocumented (afaik) "agp-internal-edid" variable.
> >>
> >> Actually it's a little different, as I realized while re-checking in
> >> order to reply to your request below. While GrUB looks to use this
> >> only "just in case", our use is actually to also cope with failure
> >> in copy_edid(): In case the overridden EDID doesn't match the size
> >> constraint (which is more strict than GrUB's), we would retry with
> >> the "discovered" one in the hope that its size is okay.
> > 
> > Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
> > 
> > "Returns EDID values and attributes that the Video BIOS must use"
> 
> I'm tempted to say "We're not the Video BIOS." ;-)

I think UEFI expects this to be exclusively used by legacy Video BIOS
implementations but not OSes?

> > And since EFI_EDID_ACTIVE_PROTOCOL will return
> > EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
> > fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
> > the call itself failing, but Xen failing to parse the result (because
> > of the usage of must in the sentence).
> > 
> > I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
> > EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
> > succeeds but it's Xen the one failing to parse the result.
> > 
> >>> Could you add this as a comment here? So it's not lost on commit as
> >>> being just a post-commit log remark.
> >>
> >> As a result I'm unsure of the value of a comment here going beyond
> >> what the comment in copy_edid() already says. For now I've added
> >>
> >>     /*
> >>      * In case an override is in place which doesn't fit copy_edid(), also try
> >>      * obtaining the discovered EDID in the hope that it's better than nothing.
> >>      */
> > 
> > I think the comment is fine, but as mentioned above I wonder whether
> > by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
> > resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
> > system more than by simply failing to get video output, hence I
> > think a more conservative approach might be to just use
> > EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
> > 
> > As with firmware, this should mostly mimic what others do in order to
> > be on the safe side, so if GrUB does so we should likely follow suit.
> 
> But they're careless in other ways; I don't want to mimic that. I would
> assume (or at least hope) that a discovered EDID still fits the system,
> perhaps not as optimally as a subsequently installed override. But then
> I also lack sufficient knowledge on all aspects which EDID may be
> relevant for, so it's all guesswork anyway afaic.

Yes, I'm afraid I don't have any more insight. I'm slightly concerned
about this, but I guess not so much as in to block the change:

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

I would word the comment as:

/*
 * In case an override is in place which doesn't fit copy_edid(), also
 * try obtaining the discovered EDID in the hope that it's better than
 * nothing.
 *
 * Note that attempting to use the information in
 * EFI_EDID_DISCOVERED_PROTOCOL when there's an override provided by
 * EFI_EDID_ACTIVE_PROTOCOL could lead to issues.
 */

Thanks, Roger.

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Jan Beulich]

On 06.04.2022 16:14, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 02:40:50PM +0200, Jan Beulich wrote:
>> On 06.04.2022 11:34, Roger Pau Monné wrote:
>>> On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
>>>> On 05.04.2022 17:47, Roger Pau Monné wrote:
>>>>> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>>>>>> On 05.04.2022 12:27, Roger Pau Monné wrote:
>>>>>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>>>>>>>> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>>>>>>>> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>>>>>>>> +    EFI_STATUS status;
>>>>>>>> +
>>>>>>>> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>>>>>>>> +                                  (void **)&active_edid, efi_ih, NULL,
>>>>>>>> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>>>>> +    if ( status == EFI_SUCCESS &&
>>>>>>>> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>>>>>>>> +        return;
>>>>>>>
>>>>>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>>>>>>>
>>>>>>> From my reading of the UEFI spec this will either return
>>>>>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
>>>>>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
>>>>>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
>>>>>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
>>>>>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>>>>>>
>>>>>> That's the theory. As per one of the post-commit-message remarks I had
>>>>>> looked at what GrUB does, and I decided to follow its behavior in this
>>>>>> regard, assuming they do what they do to work around quirks. As said
>>>>>> in the remark, I didn't want to go as far as also cloning their use of
>>>>>> the undocumented (afaik) "agp-internal-edid" variable.
>>>>
>>>> Actually it's a little different, as I realized while re-checking in
>>>> order to reply to your request below. While GrUB looks to use this
>>>> only "just in case", our use is actually to also cope with failure
>>>> in copy_edid(): In case the overridden EDID doesn't match the size
>>>> constraint (which is more strict than GrUB's), we would retry with
>>>> the "discovered" one in the hope that its size is okay.
>>>
>>> Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
>>>
>>> "Returns EDID values and attributes that the Video BIOS must use"
>>
>> I'm tempted to say "We're not the Video BIOS." ;-)
> 
> I think UEFI expects this to be exclusively used by legacy Video BIOS
> implementations but not OSes?
> 
>>> And since EFI_EDID_ACTIVE_PROTOCOL will return
>>> EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
>>> fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
>>> the call itself failing, but Xen failing to parse the result (because
>>> of the usage of must in the sentence).
>>>
>>> I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
>>> EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
>>> succeeds but it's Xen the one failing to parse the result.
>>>
>>>>> Could you add this as a comment here? So it's not lost on commit as
>>>>> being just a post-commit log remark.
>>>>
>>>> As a result I'm unsure of the value of a comment here going beyond
>>>> what the comment in copy_edid() already says. For now I've added
>>>>
>>>>     /*
>>>>      * In case an override is in place which doesn't fit copy_edid(), also try
>>>>      * obtaining the discovered EDID in the hope that it's better than nothing.
>>>>      */
>>>
>>> I think the comment is fine, but as mentioned above I wonder whether
>>> by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
>>> resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
>>> system more than by simply failing to get video output, hence I
>>> think a more conservative approach might be to just use
>>> EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
>>>
>>> As with firmware, this should mostly mimic what others do in order to
>>> be on the safe side, so if GrUB does so we should likely follow suit.
>>
>> But they're careless in other ways; I don't want to mimic that. I would
>> assume (or at least hope) that a discovered EDID still fits the system,
>> perhaps not as optimally as a subsequently installed override. But then
>> I also lack sufficient knowledge on all aspects which EDID may be
>> relevant for, so it's all guesswork anyway afaic.
> 
> Yes, I'm afraid I don't have any more insight. I'm slightly concerned
> about this, but I guess not so much as in to block the change:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I would word the comment as:
> 
> /*
>  * In case an override is in place which doesn't fit copy_edid(), also
>  * try obtaining the discovered EDID in the hope that it's better than
>  * nothing.
>  *
>  * Note that attempting to use the information in
>  * EFI_EDID_DISCOVERED_PROTOCOL when there's an override provided by
>  * EFI_EDID_ACTIVE_PROTOCOL could lead to issues.
>  */

I've extended it like this.

Jan

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Jan Beulich]

On 31.03.2022 11:45, Jan Beulich wrote:
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
> 
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
> one or both of the two low bits also doesn't seem appropriate.
> 
> GrUB also checks an "agp-internal-edid" variable. As I haven't been able
> to find any related documentation, and as GrUB being happy about the
> variable being any size (rather than at least / precisely 128 bytes),
> I didn't follow that route.
> ---
> v3: Re-base.
> v2: New.
> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
>  {
>  }
>  
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +}
> +
>  static void __init efi_arch_memory_setup(void)
>  {
>  }

May I ask for an Arm side ack here, please?

Thanks, Jan

Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Bertrand Marquis]

Hi Jan,

> On 31 Mar 2022, at 10:45, Jan Beulich <jbeulich@suse.com> wrote:
> 
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
> 
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm

Cheers
Bertrand

[PATCH v4 4/8] x86/boot: simplify mode_table

[Jan Beulich]

There's no point in writing 80x25 text mode information via multiple
insns all storing immediate values. The data can simply be included
first thing in the vga_modes table, allowing the already present
REP MOVSB to take care of everything in one go.

While touching this also correct a related but stale comment.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -655,8 +655,9 @@ outidx: outb    %al, %dx
 # Build the table of video modes (stored after the setup.S code at the
 # `modelist' label. Each video mode record looks like:
 #        .word        MODE-ID             (our special mode ID (see above))
-#        .byte        rows                (number of rows)
-#        .byte        columns             (number of columns)
+#        .word        rows                (number of rows)
+#        .word        columns             (number of columns)
+#        .word        0                   (color depth; gfx modes only)
 # Returns address of the end of the table in DI, the end is marked
 # with a ASK_VGA ID.
 mode_table:
@@ -665,12 +666,6 @@ mode_table:
         jnz     mtab1
 
         leaw    modelist, %di           # Store standard modes:
-        movw    $VIDEO_80x25,(%di)      # The 80x25 mode (ALL)
-        movw    $0x50,2(%di)
-        movw    $0x19,4(%di)
-        movw    $0x00,6(%di)
-        addw    $8,%di
-
         leaw    bootsym(vga_modes), %si # All modes for std VGA
         movw    $vga_modes_end-vga_modes, %cx
         rep     movsb
@@ -684,6 +679,7 @@ ret0:   ret
 
 # Modes usable on all standard VGAs
 vga_modes:
+        .word   VIDEO_80x25, 0x50,0x19,0        # 80x25
         .word   VIDEO_80x50, 0x50,0x32,0        # 80x50
         .word   VIDEO_80x43, 0x50,0x2b,0        # 80x43
         .word   VIDEO_80x28, 0x50,0x1c,0        # 80x28

Re: [PATCH v4 4/8] x86/boot: simplify mode_table

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:48:51AM +0200, Jan Beulich wrote:
> There's no point in writing 80x25 text mode information via multiple
> insns all storing immediate values. The data can simply be included
> first thing in the vga_modes table, allowing the already present
> REP MOVSB to take care of everything in one go.
> 
> While touching this also correct a related but stale comment.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

[PATCH v4 5/8] x86/boot: fold branches in video handling code

[Jan Beulich]

Using Jcc to branch around a JMP is necessary only in pre-386 code,
where Jcc is limited to disp8. Use the opposite Jcc directly in two
places. Since it's adjacent, also convert an ORB to TESTB.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -332,8 +332,7 @@ lment:  movb    $0, (%di)
         cmpw    $0x656d, (%si)          # 'me'
         jnz     lmhx
         cmpw    $0x756e, 2(%si)         # 'nu'
-        jnz     lmhx
-        jmp     listm
+        jz      listm
 
 lmhx:   xorw    %bx, %bx                # Else => mode ID in hex
 lmhex:  lodsb
@@ -401,10 +400,8 @@ mode_set:
         cmpb    $VIDEO_FIRST_VESA>>8, %ah
         jnc     check_vesa
 
-        orb     %ah, %ah
-        jnz     setbad
-
-        jmp     setmenu
+        testb   %ah, %ah
+        jz      setmenu
 
 setbad: clc
         ret

Re: [PATCH v4 5/8] x86/boot: fold branches in video handling code

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:49:24AM +0200, Jan Beulich wrote:
> Using Jcc to branch around a JMP is necessary only in pre-386 code,
> where Jcc is limited to disp8. Use the opposite Jcc directly in two
> places. Since it's adjacent, also convert an ORB to TESTB.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

[PATCH v4 6/8] x86/boot: fold/replace moves in video handling code

[Jan Beulich]

Replace (mainly) MOV forms with shorter insns (or sequences thereof).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course there's more room for improvement. For example there look to
be a number of LEAs which really could be MOVs.

--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -121,8 +121,7 @@ mopar_gr:
         movw    %ax, _param(PARAM_LFB_WIDTH)
         movw    20(%di), %ax
         movw    %ax, _param(PARAM_LFB_HEIGHT)
-        movb    25(%di), %al
-        movb    $0, %ah
+        movzbw  25(%di), %ax
         movw    %ax, _param(PARAM_LFB_DEPTH)
         movl    40(%di), %eax
         movl    %eax, _param(PARAM_LFB_BASE)
@@ -135,8 +134,7 @@ mopar_gr:
 
 # get video mem size
         leaw    vesa_glob_info, %di
-        xorl    %eax, %eax
-        movw    18(%di), %ax
+        movzwl  18(%di), %eax
         movl    %eax, _param(PARAM_LFB_SIZE)
 
 # store mode capabilities
@@ -144,14 +142,11 @@ mopar_gr:
         movl    %eax, _param(PARAM_CAPABILITIES)
 
 # switching the DAC to 8-bit is for <= 8 bpp only
-        movw    _param(PARAM_LFB_DEPTH), %ax
-        cmpw    $8, %ax
+        cmpw    $8, _param(PARAM_LFB_DEPTH)
         jg      dac_done
 
 # get DAC switching capability
-        xorl    %eax, %eax
-        movb    10(%di), %al
-        testb   $1, %al
+        testb   $1, 10(%di)
         jz      dac_set
 
 # attempt to switch DAC to 8-bit
@@ -164,17 +159,17 @@ mopar_gr:
 
 dac_set:
 # set color size to DAC size
-        movb    bootsym(dac_size), %al
+        movzbw  bootsym(dac_size), %ax
         movb    %al, _param(PARAM_LFB_COLORS+0)
         movb    %al, _param(PARAM_LFB_COLORS+2)
         movb    %al, _param(PARAM_LFB_COLORS+4)
         movb    %al, _param(PARAM_LFB_COLORS+6)
 
 # set color offsets to 0
-        movb    $0, _param(PARAM_LFB_COLORS+1)
-        movb    $0, _param(PARAM_LFB_COLORS+3)
-        movb    $0, _param(PARAM_LFB_COLORS+5)
-        movb    $0, _param(PARAM_LFB_COLORS+7)
+        movb    %ah, _param(PARAM_LFB_COLORS+1)
+        movb    %ah, _param(PARAM_LFB_COLORS+3)
+        movb    %ah, _param(PARAM_LFB_COLORS+5)
+        movb    %ah, _param(PARAM_LFB_COLORS+7)
 
 dac_done:
 # get protected mode interface information
@@ -504,7 +499,8 @@ setvesabysize:
         call    mode_table
         leaw    modelist,%si
 1:      add     $8,%si
-        cmpw    $ASK_VGA,-8(%si)        # End?
+        movw    -8(%si),%bx
+        cmpw    $ASK_VGA,%bx            # End?
         je      setbad
         movw    -6(%si),%ax
         cmpw    %ax,bootsym(vesa_size)+0
@@ -515,9 +511,7 @@ setvesabysize:
         movw    -2(%si),%ax
         cmpw    %ax,bootsym(vesa_size)+4
         jne     1b
-        movw    -8(%si),%ax
-        movw    %ax,%bx
-        movw    %ax,bootsym(boot_vid_mode)
+        movw    %bx,bootsym(boot_vid_mode)
         jmp     check_vesa
 
 # Table of routines for setting of the special modes.
@@ -773,8 +767,7 @@ vesa2:  pushw   %cx
         movw    %bx, 2(%di)
         movw    0x14(%di), %bx          # Height
         movw    %bx, 4(%di)
-        xorw    %bx, %bx
-        movb    0x19(%di), %bl          # Depth
+        movzbw  0x19(%di), %bx          # Depth
         movw    %bx, 6(%di)
 
         addw    $8, %di                 # The mode is valid. Store it.
@@ -901,8 +894,7 @@ gettime:
         movb    %dh, %al                # %dh contains the seconds
         andb    $0x0f, %al
         movb    %dh, %ah
-        movb    $0x04, %cl
-        shrb    %cl, %ah
+        shrb    $4, %ah
         aad
         popw    %cx
         ret
@@ -959,8 +951,8 @@ store_edid:
 .Lforce_edid:
         movw    $0x4f15, %ax            # do VBE/DDC
         movw    $0x01, %bx
-        movw    $0x00, %cx
-        movw    $0x00, %dx
+        xorw    %cx, %cx
+        xorw    %dx, %dx
         movw    $bootsym(boot_edid_info), %di
         int     $0x10
 

Re: [PATCH v4 6/8] x86/boot: fold/replace moves in video handling code

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:50:00AM +0200, Jan Beulich wrote:
> Replace (mainly) MOV forms with shorter insns (or sequences thereof).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

[PATCH v4 7/8] x86/boot: LEA -> MOV in video handling code

[Jan Beulich]

Replace most LEA instances with (one byte shorter) MOV.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: New.

--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -60,7 +60,7 @@ video:  xorw    %ax, %ax
         call    mode_set                        # Set the mode
         jc      vid1
 
-        leaw    bootsym(badmdt), %si            # Invalid mode ID
+        movw    $bootsym(badmdt), %si           # Invalid mode ID
         call    prtstr
 vid2:   call    mode_menu
 vid1:   call    store_edid
@@ -113,7 +113,7 @@ mopar2: movb    %al, _param(PARAM_VIDEO_
 
 # Fetching of VESA frame buffer parameters
 mopar_gr:
-        leaw    vesa_mode_info, %di
+        movw    $vesa_mode_info, %di
         movb    $0x23, _param(PARAM_HAVE_VGA)
         movw    16(%di), %ax
         movw    %ax, _param(PARAM_LFB_LINELENGTH)
@@ -133,7 +133,7 @@ mopar_gr:
         movw    %ax, _param(PARAM_VESA_ATTRIB)
 
 # get video mem size
-        leaw    vesa_glob_info, %di
+        movw    $vesa_glob_info, %di
         movzwl  18(%di), %eax
         movl    %eax, _param(PARAM_LFB_SIZE)
 
@@ -189,7 +189,7 @@ no_pm:  pushw   %ds
 
 # The video mode menu
 mode_menu:
-        leaw    bootsym(keymsg), %si    # "Return/Space/Timeout" message
+        movw    $bootsym(keymsg), %si   # "Return/Space/Timeout" message
         call    prtstr
         call    flush
 nokey:  call    getkt
@@ -206,22 +206,22 @@ nokey:  call    getkt
 defmd1: ret                             # No mode chosen? Default 80x25
 
 listm:  call    mode_table              # List mode table
-listm0: leaw    bootsym(name_bann), %si # Print adapter name
+listm0: movw    $bootsym(name_bann), %si # Print adapter name
         call    prtstr
         movw    bootsym(card_name), %si
         orw     %si, %si
         jnz     an2
 
-        leaw    bootsym(vga_name), %si
+        movw    $bootsym(vga_name), %si
         jmp     an1
 
 an2:    call    prtstr
-        leaw    bootsym(svga_name), %si
+        movw    $bootsym(svga_name), %si
 an1:    call    prtstr
-        leaw    bootsym(listhdr), %si   # Table header
+        movw    $bootsym(listhdr), %si  # Table header
         call    prtstr
         movb    $0x30, %dl              # DL holds mode number
-        leaw    modelist, %si
+        movw    $modelist, %si
 lm1:    cmpw    $ASK_VGA, (%si)         # End?
         jz      lm2
 
@@ -240,7 +240,7 @@ lm1:    cmpw    $ASK_VGA, (%si)
         testb   $0xff,(%si)
         jnz     1f
         push    %si
-        leaw    bootsym(textmode), %si
+        movw    $bootsym(textmode), %si
         call    prtstr
         pop     %si
         lodsw
@@ -257,7 +257,7 @@ lm1:    cmpw    $ASK_VGA, (%si)
 
         cmpb    $'z'+1, %dl
         jnz     skip_bail
-        leaw    bootsym(menu_bail_msg), %si
+        movw    $bootsym(menu_bail_msg), %si
         call    prtstr
         jmp     lm2
 
@@ -266,13 +266,13 @@ skip_bail:
         jnz     skip_pause
         push    %si
         push    %dx
-        leaw    bootsym(menu_more_msg), %si  # '<press space>'
+        movw    $bootsym(menu_more_msg), %si # '<press space>'
         call    prtstr
         call    flush
 1:      call    getkey
         cmpb    $0x20, %al              # SPACE ?
         jne     1b                      # yes - manual mode selection
-        leaw    bootsym(crlft), %si
+        movw    $bootsym(crlft), %si
         call    prtstr
         pop     %dx
         pop     %si
@@ -283,9 +283,9 @@ skip_pause:
         movb    $'a', %dl
         jmp     lm1
 
-lm2:    leaw    bootsym(prompt), %si    # Mode prompt
+lm2:    movw    $bootsym(prompt), %si   # Mode prompt
         call    prtstr
-        leaw    bootsym(edit_buf), %di  # Editor buffer
+        movw    $bootsym(edit_buf), %di # Editor buffer
 lm3:    call    getkey
         cmpb    $0x0d, %al              # Enter?
         jz      lment
@@ -315,9 +315,9 @@ lmbs:   cmpw    $bootsym(edit_buf), %di
         jmp     lm3
         
 lment:  movb    $0, (%di)
-        leaw    bootsym(crlft), %si
+        movw    $bootsym(crlft), %si
         call    prtstr
-        leaw    bootsym(edit_buf), %si
+        movw    $bootsym(edit_buf), %si
         cmpb    $0, (%si)               # Empty string = default mode
         jz      lmdef
 
@@ -373,7 +373,7 @@ mnusel: lodsb
 lmuse:  call    mode_set
         jc      lmdef
 
-lmbad:  leaw    bootsym(unknt), %si
+lmbad:  movw    $bootsym(unknt), %si
         call    prtstr
         jmp     mode_menu
 lmdef:  ret
@@ -424,13 +424,13 @@ setmenu:
         jmp     mode_set
 
 check_vesa:
-        leaw    vesa_glob_info, %di
+        movw    $vesa_glob_info, %di
         movw    $0x4f00, %ax
         int     $0x10
         cmpw    $0x004f, %ax
         jnz     setbad
 
-        leaw    vesa_mode_info, %di     # Get mode information structure
+        movw    $vesa_mode_info, %di    # Get mode information structure
         leaw    -VIDEO_FIRST_VESA(%bx), %cx
         movw    $0x4f01, %ax
         int     $0x10
@@ -497,7 +497,7 @@ inidx:  outb    %al, %dx
 
 setvesabysize:
         call    mode_table
-        leaw    modelist,%si
+        movw    $modelist,%si
 1:      add     $8,%si
         movw    -8(%si),%bx
         cmpw    $ASK_VGA,%bx            # End?
@@ -656,8 +656,8 @@ mode_table:
         orw     %di, %di
         jnz     mtab1
 
-        leaw    modelist, %di           # Store standard modes:
-        leaw    bootsym(vga_modes), %si # All modes for std VGA
+        movw    $modelist, %di          # Store standard modes:
+        movw    $bootsym(vga_modes), %si # All modes for std VGA
         movw    $vga_modes_end-vga_modes, %cx
         rep     movsb
 
@@ -665,7 +665,7 @@ mode_table:
 
         movw    $ASK_VGA, (%di)         # End marker
         movw    %di, bootsym(mt_end)
-mtab1:  leaw    modelist, %si           # SI=mode list, DI=list end
+mtab1:  movw    $modelist, %si          # SI=mode list, DI=list end
 ret0:   ret
 
 # Modes usable on all standard VGAs
@@ -681,7 +681,7 @@ vga_modes_end:
 
 # If the current mode is a VESA graphics one, obtain its parameters.
 set_current:
-        leaw    vesa_glob_info, %di
+        movw    $vesa_glob_info, %di
         movw    $0x4f00, %ax
         int     $0x10
         cmpw    $0x004f, %ax
@@ -692,7 +692,7 @@ set_current:
         cmpw    $0x004f, %ax
         jne     .Lsetc_done
 
-        leaw    vesa_mode_info, %di     # Get mode information structure
+        movw    $vesa_mode_info, %di    # Get mode information structure
         movw    %bx, %cx
         movw    $0x4f01, %ax
         int     $0x10
@@ -715,7 +715,7 @@ set_current:
 # Detect VESA modes.
 vesa_modes:
         movw    %di, %bp                # BP=original mode table end
-        leaw    vesa_glob_info, %di
+        movw    $vesa_glob_info, %di
         movw    $0x4f00, %ax            # VESA Get card info call
         int     $0x10
         movw    %di, %si
@@ -772,7 +772,7 @@ vesa2:  pushw   %cx
 
         addw    $8, %di                 # The mode is valid. Store it.
 vesan:  loop    vesa1                   # Next mode. Limit exceeded => error
-vesae:  leaw    bootsym(vesaer), %si
+vesae:  movw    $bootsym(vesaer), %si
         call    prtstr
         movw    %bp, %di                # Discard already found modes.
 vesar:  popw    %gs
@@ -917,7 +917,7 @@ store_edid:
         cmpb    $1, %al                 # EDID disabled on cmdline (edid=no)?
         je      .Lno_edid
 
-        leaw    vesa_glob_info, %di
+        movw    $vesa_glob_info, %di
         movw    $0x4f00, %ax
         int     $0x10
         cmpw    $0x004f, %ax

Re: [PATCH v4 7/8] x86/boot: LEA -> MOV in video handling code

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:50:20AM +0200, Jan Beulich wrote:
> Replace most LEA instances with (one byte shorter) MOV.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

[PATCH v4 8/8] x86/boot: fold two MOVs into an ADD

[Jan Beulich]

There's no point going through %ax; the addition can be done directly in
%di.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -24,9 +24,7 @@ get_memory_map:
         cmpw    $E820_BIOS_MAX, bootsym(bios_e820nr) # up to this many entries
         jae     .Ldone
 
-        movw    %di,%ax
-        addw    $20,%ax
-        movw    %ax,%di
+        addw    $20,%di
         testl   %ebx,%ebx                       # check to see if
         jnz     1b                              # %ebx is set to EOF
 

Re: [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD

[Roger Pau Monné]

On Thu, Mar 31, 2022 at 11:51:02AM +0200, Jan Beulich wrote:
> There's no point going through %ax; the addition can be done directly in
> %di.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.