[Xen-devel] [PATCH 0/2] EFI GOP fixes

[Xen-devel] [PATCH 0/2] EFI GOP fixes

[Igor Druzhinin]

Igor Druzhinin (2):
  efi/boot: fix set_color function
  efi/boot: make sure chosen mode is set even if firmware tell it is

 xen/common/efi/boot.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] efi/boot: fix set_color function

[Igor Druzhinin]

- 0 is a possible and allowed value for a color mask accroding to
  UEFI Spec 2.6 (11.9) especially for reserved mask
- add missing pointer dereference

Without these changes non-TrueColor modes won't work which will cause
GOP init to fail - observed while trying to boot EFI Xen with Cirrus VGA.

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

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 9a89414..933db88 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1113,10 +1113,14 @@ static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz)
    if ( bpp < 0 )
        return bpp;
    if ( !mask )
-       return -EINVAL;
+   {
+       *pos = 0;
+       *sz = 0;
+       return bpp;
+   }
    for ( *pos = 0; !(mask & 1); ++*pos )
        mask >>= 1;
-   for ( *sz = 0; mask & 1; ++sz)
+   for ( *sz = 0; mask & 1; ++*sz)
        mask >>= 1;
    if ( mask )
        return -EINVAL;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] efi/boot: make sure chosen mode is set even if firmware tell it is

[Igor Druzhinin]

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

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. */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] efi/boot: fix set_color function

[Jan Beulich]

On 03.10.2019 15:49, Igor Druzhinin wrote:
> - 0 is a possible and allowed value for a color mask accroding to
>   UEFI Spec 2.6 (11.9) especially for reserved mask

Hmm, looking at 2.8 (where it's section 12.9, which in turn is why
section titles would be more helpful in such references) I can't
see the case being mentioned explicitly. I can accept that
ReservedMask might be zero, but then I'd prefer to handle that
case in the caller, rather than allowing zero also for the three
colors.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] efi/boot: make sure chosen mode is set even if firmware tell it is

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] efi/boot: fix set_color function

[Igor Druzhinin]

On 04/10/2019 11:34, Jan Beulich wrote:
> On 03.10.2019 15:49, Igor Druzhinin wrote:
>> - 0 is a possible and allowed value for a color mask accroding to
>>   UEFI Spec 2.6 (11.9) especially for reserved mask
> 
> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why
> section titles would be more helpful in such references) I can't
> see the case being mentioned explicitly. I can accept that
> ReservedMask might be zero, but then I'd prefer to handle that
> case in the caller, rather than allowing zero also for the three
> colors.

"If a bit is set in RedMask, GreenMask, or BlueMask then those bits of
the pixel represent the corresponding color." - "If a bit is set..."
implies it might not be set. Nothing prevents mask for the colors be 0
as well.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] efi/boot: fix set_color function

[Jan Beulich]

On 04.10.2019 12:54, Igor Druzhinin wrote:
> On 04/10/2019 11:34, Jan Beulich wrote:
>> On 03.10.2019 15:49, Igor Druzhinin wrote:
>>> - 0 is a possible and allowed value for a color mask accroding to
>>>   UEFI Spec 2.6 (11.9) especially for reserved mask
>>
>> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why
>> section titles would be more helpful in such references) I can't
>> see the case being mentioned explicitly. I can accept that
>> ReservedMask might be zero, but then I'd prefer to handle that
>> case in the caller, rather than allowing zero also for the three
>> colors.
> 
> "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of
> the pixel represent the corresponding color." - "If a bit is set..."
> implies it might not be set.

This talks about the function of individual bits. There's nothing said
about not bit at all being set for a particular color.

> Nothing prevents mask for the colors be 0 as well.

I wouldn't read it like this, no. I'm fine imply such for the reserved
field, but I'd rather consider it a broken mode if one of the colors
has no way of representing at all. In particular

"The color intensities must increase as the color values for a each
 color mask increase with a minimum intensity of all bits in a color
 mask clear to a maximum intensity of all bits in a color mask set."

suggests to me that there can't be zero bits set, or else there'd be
no difference between minimum and maximum intensity. Also, while
mathematically it makes sense for "all bits" to include the case of
zero of them, it doesn't (to me at least) in day-to-day use of the
language.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] efi/boot: fix set_color function

[Igor Druzhinin]

On 04/10/2019 12:14, Jan Beulich wrote:
> On 04.10.2019 12:54, Igor Druzhinin wrote:
>> On 04/10/2019 11:34, Jan Beulich wrote:
>>> On 03.10.2019 15:49, Igor Druzhinin wrote:
>>>> - 0 is a possible and allowed value for a color mask accroding to
>>>>   UEFI Spec 2.6 (11.9) especially for reserved mask
>>>
>>> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why
>>> section titles would be more helpful in such references) I can't
>>> see the case being mentioned explicitly. I can accept that
>>> ReservedMask might be zero, but then I'd prefer to handle that
>>> case in the caller, rather than allowing zero also for the three
>>> colors.
>>
>> "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of
>> the pixel represent the corresponding color." - "If a bit is set..."
>> implies it might not be set.
> 
> This talks about the function of individual bits. There's nothing said
> about not bit at all being set for a particular color.
> 

I know certainly that it's not only me who reads this sentence the same
way - firmware developers as well. But if you insist I will restrict
this change to reserved mask only.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] efi/boot: make sure chosen mode is set even if firmware tell it is

[Igor Druzhinin]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] efi/boot: fix set_color function

[Jan Beulich]

On 04.10.2019 13:25, Igor Druzhinin wrote:
> On 04/10/2019 12:14, Jan Beulich wrote:
>> On 04.10.2019 12:54, Igor Druzhinin wrote:
>>> On 04/10/2019 11:34, Jan Beulich wrote:
>>>> On 03.10.2019 15:49, Igor Druzhinin wrote:
>>>>> - 0 is a possible and allowed value for a color mask accroding to
>>>>>   UEFI Spec 2.6 (11.9) especially for reserved mask
>>>>
>>>> Hmm, looking at 2.8 (where it's section 12.9, which in turn is why
>>>> section titles would be more helpful in such references) I can't
>>>> see the case being mentioned explicitly. I can accept that
>>>> ReservedMask might be zero, but then I'd prefer to handle that
>>>> case in the caller, rather than allowing zero also for the three
>>>> colors.
>>>
>>> "If a bit is set in RedMask, GreenMask, or BlueMask then those bits of
>>> the pixel represent the corresponding color." - "If a bit is set..."
>>> implies it might not be set.
>>
>> This talks about the function of individual bits. There's nothing said
>> about not bit at all being set for a particular color.
>>
> 
> I know certainly that it's not only me who reads this sentence the same
> way - firmware developers as well. But if you insist I will restrict
> this change to reserved mask only.

Please do, unless you can provide a plausible (non-broken) scenario
where one of the three color masks could be zero.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] efi/boot: make sure chosen mode is set even if firmware tell it is

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] efi/boot: make sure chosen mode is set even if firmware tell it is

[Igor Druzhinin]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel