[PATCH 0/2] x86: Use Multiboot framebuffer

[PATCH 0/2] x86: Use Multiboot framebuffer

[Tu Dinh Ngoc]

Xen does not currently use the Multiboot framebuffer. This means there
is no graphics when booting Xen with Kexec.

This patchset parses and uses the Multiboot framebuffer information
during boot.

Tu Dinh Ngoc (2):
  x86: Parse Multiboot2 framebuffer information
  x86: Set up framebuffer given by Multiboot2

 xen/arch/x86/boot/reloc.c    | 22 ++++++++++++++++++
 xen/arch/x86/setup.c         | 45 +++++++++++++++++++++++++++++++++---
 xen/include/xen/multiboot.h  | 17 ++++++++++++++
 xen/include/xen/multiboot2.h | 33 ++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/2] x86: Parse Multiboot2 framebuffer information

[Tu Dinh Ngoc]

Multiboot2 exposes framebuffer data in its boot information tags. Xen
requests this information from the bootloader, but does not make use of
it.

Parse this information for later use.
---
 xen/arch/x86/boot/reloc.c    | 22 ++++++++++++++++++++++
 xen/include/xen/multiboot.h  | 17 +++++++++++++++++
 xen/include/xen/multiboot2.h | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 4f4039bb7c..01a53d3ae5 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -156,6 +156,8 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
     multiboot_info_t *mbi_out;
     u32 ptr;
     unsigned int i, mod_idx = 0;
+    u64 fbaddr;
+    u8 fbtype;
 
     ptr = alloc_mem(sizeof(*mbi_out));
     mbi_out = _p(ptr);
@@ -254,6 +256,26 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
             ++mod_idx;
             break;
 
+        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
+            fbaddr = get_mb2_data(tag, framebuffer, framebuffer_addr);
+            fbtype = get_mb2_data(tag, framebuffer, framebuffer_type);
+            if (fbaddr == 0 || fbtype != MULTIBOOT2_FRAMEBUFFER_TYPE_RGB)
+                break;
+            mbi_out->flags |= MBI_FB;
+            mbi_out->fb.addr = fbaddr;
+            mbi_out->fb.pitch = get_mb2_data(tag, framebuffer, framebuffer_pitch);
+            mbi_out->fb.width = get_mb2_data(tag, framebuffer, framebuffer_width);
+            mbi_out->fb.height = get_mb2_data(tag, framebuffer, framebuffer_height);
+            mbi_out->fb.bpp = get_mb2_data(tag, framebuffer, framebuffer_bpp);
+            mbi_out->fb.type = fbtype;
+            mbi_out->fb.red_pos = get_mb2_data(tag, framebuffer, framebuffer_red_field_position);
+            mbi_out->fb.red_size = get_mb2_data(tag, framebuffer, framebuffer_red_mask_size);
+            mbi_out->fb.green_pos = get_mb2_data(tag, framebuffer, framebuffer_green_field_position);
+            mbi_out->fb.green_size = get_mb2_data(tag, framebuffer, framebuffer_green_mask_size);
+            mbi_out->fb.blue_pos = get_mb2_data(tag, framebuffer, framebuffer_blue_field_position);
+            mbi_out->fb.blue_size = get_mb2_data(tag, framebuffer, framebuffer_blue_mask_size);
+            break;
+
         case MULTIBOOT2_TAG_TYPE_END:
             return mbi_out;
 
diff --git a/xen/include/xen/multiboot.h b/xen/include/xen/multiboot.h
index d1b43e1183..2d829b5fa7 100644
--- a/xen/include/xen/multiboot.h
+++ b/xen/include/xen/multiboot.h
@@ -42,6 +42,7 @@
 #define MBI_BIOSCONFIG (_AC(1,u) << 8)
 #define MBI_LOADERNAME (_AC(1,u) << 9)
 #define MBI_APM        (_AC(1,u) << 10)
+#define MBI_FB         (_AC(1,u) << 11)
 
 #ifndef __ASSEMBLY__
 
@@ -101,6 +102,22 @@ typedef struct {
 
     /* Valid if flags sets MBI_APM */
     u32 apm_table;
+
+    /* Valid if flags sets MBI_FB */
+    struct {
+        u64 addr;
+        u32 pitch;
+        u32 width;
+        u32 height;
+        u8 bpp;
+        u8 type;
+        u8 red_pos;
+        u8 red_size;
+        u8 green_pos;
+        u8 green_size;
+        u8 blue_pos;
+        u8 blue_size;
+    } fb;
 } multiboot_info_t;
 
 /* The module structure.  */
diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h
index 5acd225044..a86a080038 100644
--- a/xen/include/xen/multiboot2.h
+++ b/xen/include/xen/multiboot2.h
@@ -177,6 +177,39 @@ typedef struct {
     u32 mod_end;
     char cmdline[];
 } multiboot2_tag_module_t;
+
+typedef struct {
+    u8 red;
+    u8 green;
+    u8 blue;
+} multiboot2_framebuffer_color_t;
+
+typedef struct {
+    u32 type;
+    u32 size;
+    u64 framebuffer_addr;
+    u32 framebuffer_pitch;
+    u32 framebuffer_width;
+    u32 framebuffer_height;
+    u8 framebuffer_bpp;
+    u8 framebuffer_type;
+    u16 reserved;
+
+    union {
+        struct {
+            u16 framebuffer_palette_num_colors;
+            multiboot2_framebuffer_color_t framebuffer_palette[0];
+        };
+        struct {
+            u8 framebuffer_red_field_position;
+            u8 framebuffer_red_mask_size;
+            u8 framebuffer_green_field_position;
+            u8 framebuffer_green_mask_size;
+            u8 framebuffer_blue_field_position;
+            u8 framebuffer_blue_mask_size;
+        };
+    };
+} multiboot2_tag_framebuffer_t;
 #endif /* __ASSEMBLY__ */
 
 #endif /* __MULTIBOOT2_H__ */
-- 
2.25.1

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

[Tu Dinh Ngoc]

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

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) );
-- 
2.25.1

Re: [PATCH 1/2] x86: Parse Multiboot2 framebuffer information

[Jan Beulich]

On 09.02.2022 14:09, Tu Dinh Ngoc wrote:
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -156,6 +156,8 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
>      multiboot_info_t *mbi_out;
>      u32 ptr;
>      unsigned int i, mod_idx = 0;
> +    u64 fbaddr;
> +    u8 fbtype;

Style: Despite adjacent pre-existing code doing so, new code should
not be using u<N> anymore, but use uint<N>_t instead.

> @@ -254,6 +256,26 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
>              ++mod_idx;
>              break;
>  
> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> +            fbaddr = get_mb2_data(tag, framebuffer, framebuffer_addr);
> +            fbtype = get_mb2_data(tag, framebuffer, framebuffer_type);
> +            if (fbaddr == 0 || fbtype != MULTIBOOT2_FRAMEBUFFER_TYPE_RGB)

Style: Blanks needed immediately inside the parentheses.

> +                break;
> +            mbi_out->flags |= MBI_FB;
> +            mbi_out->fb.addr = fbaddr;
> +            mbi_out->fb.pitch = get_mb2_data(tag, framebuffer, framebuffer_pitch);
> +            mbi_out->fb.width = get_mb2_data(tag, framebuffer, framebuffer_width);
> +            mbi_out->fb.height = get_mb2_data(tag, framebuffer, framebuffer_height);
> +            mbi_out->fb.bpp = get_mb2_data(tag, framebuffer, framebuffer_bpp);
> +            mbi_out->fb.type = fbtype;
> +            mbi_out->fb.red_pos = get_mb2_data(tag, framebuffer, framebuffer_red_field_position);
> +            mbi_out->fb.red_size = get_mb2_data(tag, framebuffer, framebuffer_red_mask_size);
> +            mbi_out->fb.green_pos = get_mb2_data(tag, framebuffer, framebuffer_green_field_position);
> +            mbi_out->fb.green_size = get_mb2_data(tag, framebuffer, framebuffer_green_mask_size);
> +            mbi_out->fb.blue_pos = get_mb2_data(tag, framebuffer, framebuffer_blue_field_position);
> +            mbi_out->fb.blue_size = get_mb2_data(tag, framebuffer, framebuffer_blue_mask_size);
> +            break;

Some of these lines are overly long. Much like you don't have
frambuffer_ prefixes on multiboot_info_t field names, you could
omit them on multiboot2_tag_framebuffer_t, which would reduce line
length some already. You're likely still not going to get around
wrapping some of the lines.

> --- a/xen/include/xen/multiboot.h
> +++ b/xen/include/xen/multiboot.h
> @@ -42,6 +42,7 @@
>  #define MBI_BIOSCONFIG (_AC(1,u) << 8)
>  #define MBI_LOADERNAME (_AC(1,u) << 9)
>  #define MBI_APM        (_AC(1,u) << 10)
> +#define MBI_FB         (_AC(1,u) << 11)

From what I can see in grub's multiboot.h bit 11 is VBE_INFO, while
bit 12 is FRAMEBUFFER_INFO.

> @@ -101,6 +102,22 @@ typedef struct {
>  
>      /* Valid if flags sets MBI_APM */
>      u32 apm_table;
> +
> +    /* Valid if flags sets MBI_FB */
> +    struct {
> +        u64 addr;
> +        u32 pitch;
> +        u32 width;
> +        u32 height;
> +        u8 bpp;
> +        u8 type;
> +        u8 red_pos;
> +        u8 red_size;
> +        u8 green_pos;
> +        u8 green_size;
> +        u8 blue_pos;
> +        u8 blue_size;
> +    } fb;
>  } multiboot_info_t;

What you add is not MB1-compatible (VBE fields come first). Furthermore
the addition means mbi_reloc() will suddenly copy more data, which I
don't think can be assumed to be fully compatible.

Jan

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

[Jan Beulich]

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

Re: [PATCH 0/2] x86: Use Multiboot framebuffer

[Jan Beulich]

On 09.02.2022 14:09, Tu Dinh Ngoc wrote:
> Xen does not currently use the Multiboot framebuffer. This means there
> is no graphics when booting Xen with Kexec.
> 
> This patchset parses and uses the Multiboot framebuffer information
> during boot.
> 
> Tu Dinh Ngoc (2):
>   x86: Parse Multiboot2 framebuffer information
>   x86: Set up framebuffer given by Multiboot2
> 
>  xen/arch/x86/boot/reloc.c    | 22 ++++++++++++++++++
>  xen/arch/x86/setup.c         | 45 +++++++++++++++++++++++++++++++++---
>  xen/include/xen/multiboot.h  | 17 ++++++++++++++
>  xen/include/xen/multiboot2.h | 33 ++++++++++++++++++++++++++
>  4 files changed, 114 insertions(+), 3 deletions(-)

Btw, please see also
https://lists.xen.org/archives/html/xen-devel/2021-12/msg00379.html

Jan