[2/2] efi/boot: make sure chosen mode is set even if firmware tell it is
diff mbox series

Message ID 1570110555-24287-3-git-send-email-igor.druzhinin@citrix.com
State Superseded
Headers show
Series
  • EFI GOP fixes
Related show

Commit Message

Igor Druzhinin Oct. 3, 2019, 1:49 p.m. UTC
If a bootloader is using native driver instead of EFI GOP it might
reset graphics mode to be different from what firmware thinks it
currently is. Set chosen mode unconditionally to fix this possible
misalignment.

Observed with EFI GRUB2 compiled with all possible video drivers where
native drivers take priority over firmware.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 xen/common/efi/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jan Beulich Oct. 4, 2019, 10:40 a.m. UTC | #1
On 03.10.2019 15:49, Igor Druzhinin wrote:
> If a bootloader is using native driver instead of EFI GOP it might
> reset graphics mode to be different from what firmware thinks it
> currently is. Set chosen mode unconditionally to fix this possible
> misalignment.
> 
> Observed with EFI GRUB2 compiled with all possible video drivers where
> native drivers take priority over firmware.

I don't think this case can happen with just plain EFI. Therefore ...

> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  xen/common/efi/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 933db88..4067721 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1052,7 +1052,7 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>      UINTN info_size;
>  
>      /* Set graphics mode. */
> -    if ( gop_mode < gop->Mode->MaxMode && gop_mode != gop->Mode->Mode )
> +    if ( gop_mode < gop->Mode->MaxMode )
>          gop->SetMode(gop, gop_mode);

... rather than deleting the right side of the && I'd like to
suggest to extend to to take effect only when coming straight
from EFI (i.e. EFI_LOADER set in efi_flags). The comment then
should be extended to explain why this is. (Reason being that
I'd prefer to avoid mode switches unless they're needed for
a certain reason.)

Jan
Igor Druzhinin Oct. 4, 2019, 11:33 a.m. UTC | #2
On 04/10/2019 11:40, Jan Beulich wrote:
> On 03.10.2019 15:49, Igor Druzhinin wrote:
>> If a bootloader is using native driver instead of EFI GOP it might
>> reset graphics mode to be different from what firmware thinks it
>> currently is. Set chosen mode unconditionally to fix this possible
>> misalignment.
>>
>> Observed with EFI GRUB2 compiled with all possible video drivers where
>> native drivers take priority over firmware.
> 
> I don't think this case can happen with just plain EFI. Therefore ...
> 

Could you clarify what you mean by "plain EFI" here? Do you mean being
booted as EFI binary unlike through multiboot protocol? I think in both
cases it's possible to come there through a bootloader.

Igor
Jan Beulich Oct. 4, 2019, 1:04 p.m. UTC | #3
On 04.10.2019 13:33, Igor Druzhinin wrote:
> On 04/10/2019 11:40, Jan Beulich wrote:
>> On 03.10.2019 15:49, Igor Druzhinin wrote:
>>> If a bootloader is using native driver instead of EFI GOP it might
>>> reset graphics mode to be different from what firmware thinks it
>>> currently is. Set chosen mode unconditionally to fix this possible
>>> misalignment.
>>>
>>> Observed with EFI GRUB2 compiled with all possible video drivers where
>>> native drivers take priority over firmware.
>>
>> I don't think this case can happen with just plain EFI. Therefore ...
>>
> 
> Could you clarify what you mean by "plain EFI" here? Do you mean being
> booted as EFI binary unlike through multiboot protocol?

Yes - like when running xen.efi from the EFI shell command line.

> I think in both
> cases it's possible to come there through a bootloader.

Anything invoking an EFI application with a screwed up EFI
environment is bogus. I can see how grub would violate such
assumptions though, and how one might call this valid when
invoking the next binary with a protocol other than what plain
EFI provides (read: MB2 here).

Jan
Igor Druzhinin Oct. 4, 2019, 1:08 p.m. UTC | #4
On 04/10/2019 14:04, Jan Beulich wrote:
> On 04.10.2019 13:33, Igor Druzhinin wrote:
>> On 04/10/2019 11:40, Jan Beulich wrote:
>>> On 03.10.2019 15:49, Igor Druzhinin wrote:
>>>> If a bootloader is using native driver instead of EFI GOP it might
>>>> reset graphics mode to be different from what firmware thinks it
>>>> currently is. Set chosen mode unconditionally to fix this possible
>>>> misalignment.
>>>>
>>>> Observed with EFI GRUB2 compiled with all possible video drivers where
>>>> native drivers take priority over firmware.
>>>
>>> I don't think this case can happen with just plain EFI. Therefore ...
>>>
>>
>> Could you clarify what you mean by "plain EFI" here? Do you mean being
>> booted as EFI binary unlike through multiboot protocol?
> 
> Yes - like when running xen.efi from the EFI shell command line.
> 
>> I think in both
>> cases it's possible to come there through a bootloader.
> 
> Anything invoking an EFI application with a screwed up EFI
> environment is bogus. I can see how grub would violate such
> assumptions though, and how one might call this valid when
> invoking the next binary with a protocol other than what plain
> EFI provides (read: MB2 here).
> 

I'll check if it's the case and will CC Daniel if it's broken that way.

Igor

Patch
diff mbox series

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 933db88..4067721 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1052,7 +1052,7 @@  static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
     UINTN info_size;
 
     /* Set graphics mode. */
-    if ( gop_mode < gop->Mode->MaxMode && gop_mode != gop->Mode->Mode )
+    if ( gop_mode < gop->Mode->MaxMode )
         gop->SetMode(gop, gop_mode);
 
     /* Get graphics and frame buffer info. */