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

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