In some graphics modes firmware is allowed to return 0 in pixel reserved
bitmask which doesn't go against UEFI Spec 2.8 (12.9 Graphics Output Protocol).

Without this change 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/arch/x86/efi/efi-boot.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index a0737f9..4af6314 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -528,9 +528,15 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
         bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
                         &vga_console_info.u.vesa_lfb.blue_pos,
                         &vga_console_info.u.vesa_lfb.blue_size);
-        bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
-                        &vga_console_info.u.vesa_lfb.rsvd_pos,
-                        &vga_console_info.u.vesa_lfb.rsvd_size);
+        if ( !mode_info->PixelInformation.ReservedMask )
+        {
+            vga_console_info.u.vesa_lfb.rsvd_pos = 0;
+            vga_console_info.u.vesa_lfb.rsvd_size = 0;
+        }
+        else
+            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
+                            &vga_console_info.u.vesa_lfb.rsvd_pos,
+                            &vga_console_info.u.vesa_lfb.rsvd_size);
         if ( bpp > 0 )
             break;
         /* fall through */
-- 
2.7.4


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

On 09.10.2019 22:40, Igor Druzhinin wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -528,9 +528,15 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>          bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
>                          &vga_console_info.u.vesa_lfb.blue_pos,
>                          &vga_console_info.u.vesa_lfb.blue_size);
> -        bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> -                        &vga_console_info.u.vesa_lfb.rsvd_pos,
> -                        &vga_console_info.u.vesa_lfb.rsvd_size);
> +        if ( !mode_info->PixelInformation.ReservedMask )
> +        {
> +            vga_console_info.u.vesa_lfb.rsvd_pos = 0;
> +            vga_console_info.u.vesa_lfb.rsvd_size = 0;
> +        }
> +        else
> +            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> +                            &vga_console_info.u.vesa_lfb.rsvd_pos,
> +                            &vga_console_info.u.vesa_lfb.rsvd_size);

Why not simply

        if ( mode_info->PixelInformation.ReservedMask )
            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
                            &vga_console_info.u.vesa_lfb.rsvd_pos,
                            &vga_console_info.u.vesa_lfb.rsvd_size);

? There's nothing I can see which might have changed
vga_console_info.u.vesa_lfb.rsvd_{pos,size} from its zero-initialized
value. With this adjustment (which could be done while committing) or
with a reason supplied for the more complex code
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

On 10/10/2019 08:13, Jan Beulich wrote:
> On 09.10.2019 22:40, Igor Druzhinin wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -528,9 +528,15 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>>          bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
>>                          &vga_console_info.u.vesa_lfb.blue_pos,
>>                          &vga_console_info.u.vesa_lfb.blue_size);
>> -        bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
>> -                        &vga_console_info.u.vesa_lfb.rsvd_pos,
>> -                        &vga_console_info.u.vesa_lfb.rsvd_size);
>> +        if ( !mode_info->PixelInformation.ReservedMask )
>> +        {
>> +            vga_console_info.u.vesa_lfb.rsvd_pos = 0;
>> +            vga_console_info.u.vesa_lfb.rsvd_size = 0;
>> +        }
>> +        else
>> +            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
>> +                            &vga_console_info.u.vesa_lfb.rsvd_pos,
>> +                            &vga_console_info.u.vesa_lfb.rsvd_size);
> 
> Why not simply
> 
>         if ( mode_info->PixelInformation.ReservedMask )
>             bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
>                             &vga_console_info.u.vesa_lfb.rsvd_pos,
>                             &vga_console_info.u.vesa_lfb.rsvd_size);
> 
> ? There's nothing I can see which might have changed
> vga_console_info.u.vesa_lfb.rsvd_{pos,size} from its zero-initialized
> value. With this adjustment (which could be done while committing) or
> with a reason supplied for the more complex code
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 

Didn't notice it was actually statically zero-initialized. Perfectly
fine with the suggested change.

Igor

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