[2/2] x86: Set up framebuffer given by Multiboot2

Commit Message

Tu Dinh Ngoc Feb. 9, 2022, 1:09 p.m. UTC

Previously, we do not make use of the framebuffer given by Multiboot.
This means graphics will not work in some scenarios such as booting from
Kexec.

Enable the Multiboot framebuffer if it exists and not overridden by EFI
probe.
---
 xen/arch/x86/setup.c | 45 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

Comments

Jan Beulich Feb. 9, 2022, 1:59 p.m. UTC | #1

On 09.02.2022 14:09, Tu Dinh Ngoc wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -551,16 +551,55 @@ struct boot_video_info {
>  extern struct boot_video_info boot_vid_info;
>  #endif
>  
> -static void __init parse_video_info(void)
> +static void __init parse_video_info(multiboot_info_t *mbi)

The parameter can be pointer-to-const afaict.

>  {
>  #ifdef CONFIG_VIDEO
>      struct boot_video_info *bvi = &bootsym(boot_vid_info);
>  
> +    /*
> +     * fb detection will be in this order:
> +     *  - efifb (as efifb probe sets a new GOP mode before parse_video_info is called,
> +     *    we must use this mode instead of the one given by mbifb)
> +     *  - mbifb
> +     *  - vesafb
> +     */

This ordering needs justification, first and foremost because this way
you risk regressions when VESAFB data is also available. There would be
no such risk if you made mbifb lowest priority.

Style: Comments should start with an upper case letter. There are very
few exceptions to this (e.g. when a comment starts with an
identifier referring elsewhere), but here there's no problem with
starting the comment "FB detection ...".

>      /* vga_console_info is filled directly on EFI platform. */
>      if ( efi_enabled(EFI_BOOT) )
>          return;
>  
> -    if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) )
> +    if ( mbi->flags & MBI_FB )

Even with MBI_FB's value corrected in patch 1 - what about the flag
being set from an MB1 bootloader? I'd be hesitant to trust that the
data is dependable upon everywhere.

> +    {
> +        uint64_t lfb_rgb_bitmap = 0;

I don't think you really need this initializer.

> +        vga_console_info.video_type = XEN_VGATYPE_VESA_LFB;
> +        vga_console_info.u.vesa_lfb.width = mbi->fb.width;
> +        vga_console_info.u.vesa_lfb.height = mbi->fb.height;
> +        vga_console_info.u.vesa_lfb.bytes_per_line = mbi->fb.pitch;
> +        vga_console_info.u.vesa_lfb.bits_per_pixel = mbi->fb.bpp;
> +        vga_console_info.u.vesa_lfb.lfb_base = mbi->fb.addr;
> +        vga_console_info.u.vesa_lfb.lfb_size = (mbi->fb.pitch * mbi->fb.height + 0xffff) >> 16;
> +
> +        vga_console_info.u.vesa_lfb.red_pos = mbi->fb.red_pos;
> +        vga_console_info.u.vesa_lfb.red_size = mbi->fb.red_size;
> +        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.red_size) - 1) << mbi->fb.red_pos;

1ull is both shorter and avoids using a cast.

> +        vga_console_info.u.vesa_lfb.green_pos = mbi->fb.green_pos;
> +        vga_console_info.u.vesa_lfb.green_size = mbi->fb.green_size;
> +        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.green_size) - 1) << mbi->fb.green_pos;
> +        vga_console_info.u.vesa_lfb.blue_pos = mbi->fb.blue_pos;
> +        vga_console_info.u.vesa_lfb.blue_size = mbi->fb.blue_size;
> +        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.blue_size) - 1) << mbi->fb.blue_pos;
> +
> +        /* assume non-weird bit format */
> +        vga_console_info.u.vesa_lfb.rsvd_pos = find_first_zero_bit(&lfb_rgb_bitmap, sizeof(lfb_rgb_bitmap) * __CHAR_BIT__);
> +        vga_console_info.u.vesa_lfb.rsvd_size = mbi->fb.bpp - mbi->fb.red_size - mbi->fb.green_size - mbi->fb.blue_size;

The comment really is about this 2nd assignment afaict, so it would better
move down. I'm not convinced "assume" is enough, though. I think the data
should simply not be used if the color placement doesn't match our
expectations.

Also these are overly long lines again.

Finally if lfb_rgb_bitmap was "unsigned long" (and hence still at least
64 bits, as we're on a 64-bit architecture) you could use the simpler
(because not requiring the address to be taken)
find_first_set_bit(~lfb_rgb_bitmap), provided of course lfb_rgb_bitmap
doesn't have all bits set.

> +        if (vga_console_info.u.vesa_lfb.rsvd_pos >= mbi->fb.bpp || vga_console_info.u.vesa_lfb.rsvd_size < 0) {

Style: Missing blanks, line length, and brace placement.

Jan

Patch

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 115f8f6517..04d8be407e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -551,16 +551,55 @@  struct boot_video_info {
 extern struct boot_video_info boot_vid_info;
 #endif
 
-static void __init parse_video_info(void)
+static void __init parse_video_info(multiboot_info_t *mbi)
 {
 #ifdef CONFIG_VIDEO
     struct boot_video_info *bvi = &bootsym(boot_vid_info);
 
+    /*
+     * fb detection will be in this order:
+     *  - efifb (as efifb probe sets a new GOP mode before parse_video_info is called,
+     *    we must use this mode instead of the one given by mbifb)
+     *  - mbifb
+     *  - vesafb
+     */
+
     /* vga_console_info is filled directly on EFI platform. */
     if ( efi_enabled(EFI_BOOT) )
         return;
 
-    if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) )
+    if ( mbi->flags & MBI_FB )
+    {
+        uint64_t lfb_rgb_bitmap = 0;
+
+        vga_console_info.video_type = XEN_VGATYPE_VESA_LFB;
+        vga_console_info.u.vesa_lfb.width = mbi->fb.width;
+        vga_console_info.u.vesa_lfb.height = mbi->fb.height;
+        vga_console_info.u.vesa_lfb.bytes_per_line = mbi->fb.pitch;
+        vga_console_info.u.vesa_lfb.bits_per_pixel = mbi->fb.bpp;
+        vga_console_info.u.vesa_lfb.lfb_base = mbi->fb.addr;
+        vga_console_info.u.vesa_lfb.lfb_size = (mbi->fb.pitch * mbi->fb.height + 0xffff) >> 16;
+
+        vga_console_info.u.vesa_lfb.red_pos = mbi->fb.red_pos;
+        vga_console_info.u.vesa_lfb.red_size = mbi->fb.red_size;
+        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.red_size) - 1) << mbi->fb.red_pos;
+        vga_console_info.u.vesa_lfb.green_pos = mbi->fb.green_pos;
+        vga_console_info.u.vesa_lfb.green_size = mbi->fb.green_size;
+        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.green_size) - 1) << mbi->fb.green_pos;
+        vga_console_info.u.vesa_lfb.blue_pos = mbi->fb.blue_pos;
+        vga_console_info.u.vesa_lfb.blue_size = mbi->fb.blue_size;
+        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.blue_size) - 1) << mbi->fb.blue_pos;
+
+        /* assume non-weird bit format */
+        vga_console_info.u.vesa_lfb.rsvd_pos = find_first_zero_bit(&lfb_rgb_bitmap, sizeof(lfb_rgb_bitmap) * __CHAR_BIT__);
+        vga_console_info.u.vesa_lfb.rsvd_size = mbi->fb.bpp - mbi->fb.red_size - mbi->fb.green_size - mbi->fb.blue_size;
+        if (vga_console_info.u.vesa_lfb.rsvd_pos >= mbi->fb.bpp || vga_console_info.u.vesa_lfb.rsvd_size < 0) {
+            vga_console_info.u.vesa_lfb.rsvd_pos = 0;
+            vga_console_info.u.vesa_lfb.rsvd_size = 0;
+        }
+        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
+    }
+    else if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) )
     {
         vga_console_info.video_type = XEN_VGATYPE_TEXT_MODE_3;
         vga_console_info.u.text_mode_3.font_height = bvi->orig_video_points;
@@ -933,7 +972,7 @@  void __init noreturn __start_xen(unsigned long mbi_p)
      */
     hypervisor_name = hypervisor_probe();
 
-    parse_video_info();
+    parse_video_info(mbi);
 
     rdmsrl(MSR_EFER, this_cpu(efer));
     asm volatile ( "mov %%cr4,%0" : "=r" (get_cpu_info()->cr4) );