diff mbox series

[v4,2/8] x86/boot: obtain video info from boot loader

Message ID 6fc25224-7985-73a6-8877-bc209f64bf8c@suse.com (mailing list archive)
State New, archived
Headers show
Series x86/boot: (mostly) video mode handling adjustments | expand

Commit Message

Jan Beulich March 31, 2022, 9:45 a.m. UTC
With MB2 the boot loader may provide this information, allowing us to
obtain it without needing to enter real mode (assuming we don't need to
set a new mode from "vga=", but can instead inherit the one the
bootloader may have established).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Re-base.
v3: Re-base.
v2: New.

Comments

Roger Pau Monné April 5, 2022, 9:35 a.m. UTC | #1
On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> With MB2 the boot loader may provide this information, allowing us to
> obtain it without needing to enter real mode (assuming we don't need to
> set a new mode from "vga=", but can instead inherit the one the
> bootloader may have established).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: Re-base.
> v3: Re-base.
> v2: New.
> 
> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -53,6 +53,7 @@ typedef unsigned int u32;
>  typedef unsigned long long u64;
>  typedef unsigned int size_t;
>  typedef u8 uint8_t;
> +typedef u16 uint16_t;
>  typedef u32 uint32_t;
>  typedef u64 uint64_t;
>  
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -562,12 +562,18 @@ trampoline_setup:
>          mov     %esi, sym_esi(xen_phys_start)
>          mov     %esi, sym_esi(trampoline_xen_phys_start)
>  
> -        mov     sym_esi(trampoline_phys), %ecx
> -
>          /* Get bottom-most low-memory stack address. */
> +        mov     sym_esi(trampoline_phys), %ecx
>          add     $TRAMPOLINE_SPACE,%ecx

Just for my understanding, since you are already touching the
instruction, why not switch it to a lea like you do below?

Is that because you would also like to take the opportunity to fold
the add into the lea and that would be too much of a change?

>  
> +#ifdef CONFIG_VIDEO
> +        lea     sym_esi(boot_vid_info), %edx
> +#else
> +        xor     %edx, %edx
> +#endif
> +
>          /* Save Multiboot / PVH info struct (after relocation) for later use. */
> +        push    %edx                /* Boot video info to be filled from MB2. */
>          push    %ecx                /* Bottom-most low-memory stack address. */
>          push    %ebx                /* Multiboot / PVH information address. */
>          push    %eax                /* Magic number. */
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -14,9 +14,10 @@
>  
>  /*
>   * This entry point is entered from xen/arch/x86/boot/head.S with:
> - *   - 0x4(%esp) = MAGIC,
> - *   - 0x8(%esp) = INFORMATION_ADDRESS,
> - *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> + *   - 0x04(%esp) = MAGIC,
> + *   - 0x08(%esp) = INFORMATION_ADDRESS,
> + *   - 0x0c(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> + *   - 0x10(%esp) = BOOT_VIDEO_INFO_ADDRESS.
>   */
>  asm (
>      "    .text                         \n"
> @@ -32,6 +33,39 @@ asm (
>  #include "../../../include/xen/kconfig.h"
>  #include <public/arch-x86/hvm/start_info.h>
>  
> +#ifdef CONFIG_VIDEO
> +# include "video.h"
> +
> +/* VESA control information */
> +struct __packed vesa_ctrl_info {
> +    uint8_t signature[4];
> +    uint16_t version;
> +    uint32_t oem_name;
> +    uint32_t capabilities;
> +    uint32_t mode_list;
> +    uint16_t mem_size;
> +    /* We don't use any further fields. */
> +};
> +
> +/* VESA 2.0 mode information */
> +struct vesa_mode_info {

Should we add __packed here just in case further added fields are no
longer naturally aligned? (AFAICT all field right now are aligned to
it's size so there's no need for it).

> +    uint16_t attrib;
> +    uint8_t window[14]; /* We don't use the individual fields. */
> +    uint16_t bytes_per_line;
> +    uint16_t width;
> +    uint16_t height;
> +    uint8_t cell_width;
> +    uint8_t cell_height;
> +    uint8_t nr_planes;
> +    uint8_t depth;
> +    uint8_t memory[5]; /* We don't use the individual fields. */
> +    struct boot_video_colors colors;
> +    uint8_t direct_color;
> +    uint32_t base;
> +    /* We don't use any further fields. */
> +};

Would it make sense to put those struct definitions in boot/video.h
like you do for boot_video_info?

I also wonder whether you could then hide the #ifdef CONFIG_VIDEO
check inside of the header itself.

> +#endif /* CONFIG_VIDEO */
> +
>  #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t *)(tag))->member)
>  #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
>  
> @@ -146,7 +180,7 @@ static multiboot_info_t *mbi_reloc(u32 m
>      return mbi_out;
>  }
>  
> -static multiboot_info_t *mbi2_reloc(u32 mbi_in)
> +static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out)
>  {
>      const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
>      const multiboot2_memory_map_t *mmap_src;
> @@ -154,6 +188,9 @@ static multiboot_info_t *mbi2_reloc(u32
>      module_t *mbi_out_mods = NULL;
>      memory_map_t *mmap_dst;
>      multiboot_info_t *mbi_out;
> +#ifdef CONFIG_VIDEO
> +    struct boot_video_info *video = NULL;
> +#endif
>      u32 ptr;
>      unsigned int i, mod_idx = 0;
>  
> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
>              ++mod_idx;
>              break;
>  
> +#ifdef CONFIG_VIDEO
> +        case MULTIBOOT2_TAG_TYPE_VBE:
> +            if ( video_out )
> +            {
> +                const struct vesa_ctrl_info *ci;
> +                const struct vesa_mode_info *mi;
> +
> +                video = _p(video_out);
> +                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> +                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> +
> +                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
> +                {
> +                    video->capabilities = ci->capabilities;
> +                    video->lfb_linelength = mi->bytes_per_line;
> +                    video->lfb_width = mi->width;
> +                    video->lfb_height = mi->height;
> +                    video->lfb_depth = mi->depth;
> +                    video->lfb_base = mi->base;
> +                    video->lfb_size = ci->mem_size;
> +                    video->colors = mi->colors;
> +                    video->vesa_attrib = mi->attrib;
> +                }
> +
> +                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
> +                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
> +            }
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> +            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> +                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> +            {
> +                video_out = 0;
> +                video = NULL;
> +            }

I'm confused, don't you need to store the information in the
framebuffer tag for use after relocation?

> +            break;
> +#endif /* CONFIG_VIDEO */
> +
>          case MULTIBOOT2_TAG_TYPE_END:
> -            return mbi_out;
> +            goto end; /* Cannot "break;" here. */
>  
>          default:
>              break;
>          }
>  
> + end:
> +
> +#ifdef CONFIG_VIDEO
> +    if ( video )
> +        video->orig_video_isVGA = 0x23;

I see we use this elsewhere, what's the meaning of this (magic) 0x23?

Thanks, Roger.
Jan Beulich April 5, 2022, 10:57 a.m. UTC | #2
On 05.04.2022 11:35, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -562,12 +562,18 @@ trampoline_setup:
>>          mov     %esi, sym_esi(xen_phys_start)
>>          mov     %esi, sym_esi(trampoline_xen_phys_start)
>>  
>> -        mov     sym_esi(trampoline_phys), %ecx
>> -
>>          /* Get bottom-most low-memory stack address. */
>> +        mov     sym_esi(trampoline_phys), %ecx
>>          add     $TRAMPOLINE_SPACE,%ecx
> 
> Just for my understanding, since you are already touching the
> instruction, why not switch it to a lea like you do below?
> 
> Is that because you would also like to take the opportunity to fold
> the add into the lea and that would be too much of a change?

No. This MOV cannot be converted, as its source operand isn't an
immediate (or register); such a conversion would also be undesirable,
for increasing insn size. See the later patch doing conversions in
the other direction, to reduce code size. Somewhat similarly ...

>> +#ifdef CONFIG_VIDEO
>> +        lea     sym_esi(boot_vid_info), %edx

... this LEA also cannot be expressed by a single MOV.

>> @@ -32,6 +33,39 @@ asm (
>>  #include "../../../include/xen/kconfig.h"
>>  #include <public/arch-x86/hvm/start_info.h>
>>  
>> +#ifdef CONFIG_VIDEO
>> +# include "video.h"
>> +
>> +/* VESA control information */
>> +struct __packed vesa_ctrl_info {
>> +    uint8_t signature[4];
>> +    uint16_t version;
>> +    uint32_t oem_name;
>> +    uint32_t capabilities;
>> +    uint32_t mode_list;
>> +    uint16_t mem_size;
>> +    /* We don't use any further fields. */
>> +};
>> +
>> +/* VESA 2.0 mode information */
>> +struct vesa_mode_info {
> 
> Should we add __packed here just in case further added fields are no
> longer naturally aligned? (AFAICT all field right now are aligned to
> it's size so there's no need for it).

I think we should avoid __packed whenever possible.

>> +    uint16_t attrib;
>> +    uint8_t window[14]; /* We don't use the individual fields. */
>> +    uint16_t bytes_per_line;
>> +    uint16_t width;
>> +    uint16_t height;
>> +    uint8_t cell_width;
>> +    uint8_t cell_height;
>> +    uint8_t nr_planes;
>> +    uint8_t depth;
>> +    uint8_t memory[5]; /* We don't use the individual fields. */
>> +    struct boot_video_colors colors;
>> +    uint8_t direct_color;
>> +    uint32_t base;
>> +    /* We don't use any further fields. */
>> +};
> 
> Would it make sense to put those struct definitions in boot/video.h
> like you do for boot_video_info?

Personally I prefer to expose things in headers only when multiple
other files want to consume what is being declared/defined.

>> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
>>              ++mod_idx;
>>              break;
>>  
>> +#ifdef CONFIG_VIDEO
>> +        case MULTIBOOT2_TAG_TYPE_VBE:
>> +            if ( video_out )
>> +            {
>> +                const struct vesa_ctrl_info *ci;
>> +                const struct vesa_mode_info *mi;
>> +
>> +                video = _p(video_out);
>> +                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
>> +                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
>> +
>> +                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
>> +                {
>> +                    video->capabilities = ci->capabilities;
>> +                    video->lfb_linelength = mi->bytes_per_line;
>> +                    video->lfb_width = mi->width;
>> +                    video->lfb_height = mi->height;
>> +                    video->lfb_depth = mi->depth;
>> +                    video->lfb_base = mi->base;
>> +                    video->lfb_size = ci->mem_size;
>> +                    video->colors = mi->colors;
>> +                    video->vesa_attrib = mi->attrib;
>> +                }
>> +
>> +                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
>> +                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
>> +            }
>> +            break;
>> +
>> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
>> +            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
>> +                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
>> +            {
>> +                video_out = 0;
>> +                video = NULL;
>> +            }
> 
> I'm confused, don't you need to store the information in the
> framebuffer tag for use after relocation?

If there was a consumer - yes. Right now this tag is used only to
invalidate the information taken from the other tag (or to suppress
taking values from there if that other tag came later) in case the
framebuffer type doesn't match what we support.

>> +            break;
>> +#endif /* CONFIG_VIDEO */
>> +
>>          case MULTIBOOT2_TAG_TYPE_END:
>> -            return mbi_out;
>> +            goto end; /* Cannot "break;" here. */
>>  
>>          default:
>>              break;
>>          }
>>  
>> + end:
>> +
>> +#ifdef CONFIG_VIDEO
>> +    if ( video )
>> +        video->orig_video_isVGA = 0x23;
> 
> I see we use this elsewhere, what's the meaning of this (magic) 0x23?

This is a value Linux uses (also as a plain number without any #define
iirc; at least it was that way when we first inherited that value).
Short of knowing where they took it from or what meaning they associate
with the value it would be hard for us to give this a (meaningful) name
and hence use a #define. And hence it's equally hard to properly answer
your question.

Jan
Roger Pau Monné April 5, 2022, 11:34 a.m. UTC | #3
On Tue, Apr 05, 2022 at 12:57:51PM +0200, Jan Beulich wrote:
> On 05.04.2022 11:35, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/boot/head.S
> >> +++ b/xen/arch/x86/boot/head.S
> >> @@ -562,12 +562,18 @@ trampoline_setup:
> >>          mov     %esi, sym_esi(xen_phys_start)
> >>          mov     %esi, sym_esi(trampoline_xen_phys_start)
> >>  
> >> -        mov     sym_esi(trampoline_phys), %ecx
> >> -
> >>          /* Get bottom-most low-memory stack address. */
> >> +        mov     sym_esi(trampoline_phys), %ecx
> >>          add     $TRAMPOLINE_SPACE,%ecx
> > 
> > Just for my understanding, since you are already touching the
> > instruction, why not switch it to a lea like you do below?
> > 
> > Is that because you would also like to take the opportunity to fold
> > the add into the lea and that would be too much of a change?
> 
> No. This MOV cannot be converted, as its source operand isn't an
> immediate (or register); such a conversion would also be undesirable,
> for increasing insn size. See the later patch doing conversions in
> the other direction, to reduce code size. Somewhat similarly ...
> 
> >> +#ifdef CONFIG_VIDEO
> >> +        lea     sym_esi(boot_vid_info), %edx
> 
> ... this LEA also cannot be expressed by a single MOV.
> 
> >> @@ -32,6 +33,39 @@ asm (
> >>  #include "../../../include/xen/kconfig.h"
> >>  #include <public/arch-x86/hvm/start_info.h>
> >>  
> >> +#ifdef CONFIG_VIDEO
> >> +# include "video.h"
> >> +
> >> +/* VESA control information */
> >> +struct __packed vesa_ctrl_info {
> >> +    uint8_t signature[4];
> >> +    uint16_t version;
> >> +    uint32_t oem_name;
> >> +    uint32_t capabilities;
> >> +    uint32_t mode_list;
> >> +    uint16_t mem_size;
> >> +    /* We don't use any further fields. */
> >> +};
> >> +
> >> +/* VESA 2.0 mode information */
> >> +struct vesa_mode_info {
> > 
> > Should we add __packed here just in case further added fields are no
> > longer naturally aligned? (AFAICT all field right now are aligned to
> > it's size so there's no need for it).
> 
> I think we should avoid __packed whenever possible.
> 
> >> +    uint16_t attrib;
> >> +    uint8_t window[14]; /* We don't use the individual fields. */
> >> +    uint16_t bytes_per_line;
> >> +    uint16_t width;
> >> +    uint16_t height;
> >> +    uint8_t cell_width;
> >> +    uint8_t cell_height;
> >> +    uint8_t nr_planes;
> >> +    uint8_t depth;
> >> +    uint8_t memory[5]; /* We don't use the individual fields. */
> >> +    struct boot_video_colors colors;
> >> +    uint8_t direct_color;
> >> +    uint32_t base;
> >> +    /* We don't use any further fields. */
> >> +};
> > 
> > Would it make sense to put those struct definitions in boot/video.h
> > like you do for boot_video_info?
> 
> Personally I prefer to expose things in headers only when multiple
> other files want to consume what is being declared/defined.
> 
> >> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
> >>              ++mod_idx;
> >>              break;
> >>  
> >> +#ifdef CONFIG_VIDEO
> >> +        case MULTIBOOT2_TAG_TYPE_VBE:
> >> +            if ( video_out )
> >> +            {
> >> +                const struct vesa_ctrl_info *ci;
> >> +                const struct vesa_mode_info *mi;
> >> +
> >> +                video = _p(video_out);
> >> +                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> >> +                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> >> +
> >> +                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
> >> +                {
> >> +                    video->capabilities = ci->capabilities;
> >> +                    video->lfb_linelength = mi->bytes_per_line;
> >> +                    video->lfb_width = mi->width;
> >> +                    video->lfb_height = mi->height;
> >> +                    video->lfb_depth = mi->depth;
> >> +                    video->lfb_base = mi->base;
> >> +                    video->lfb_size = ci->mem_size;
> >> +                    video->colors = mi->colors;
> >> +                    video->vesa_attrib = mi->attrib;
> >> +                }
> >> +
> >> +                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
> >> +                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
> >> +            }
> >> +            break;
> >> +
> >> +        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> >> +            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> >> +                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> >> +            {
> >> +                video_out = 0;
> >> +                video = NULL;
> >> +            }
> > 
> > I'm confused, don't you need to store the information in the
> > framebuffer tag for use after relocation?
> 
> If there was a consumer - yes. Right now this tag is used only to
> invalidate the information taken from the other tag (or to suppress
> taking values from there if that other tag came later) in case the
> framebuffer type doesn't match what we support.
> 
> >> +            break;
> >> +#endif /* CONFIG_VIDEO */
> >> +
> >>          case MULTIBOOT2_TAG_TYPE_END:
> >> -            return mbi_out;
> >> +            goto end; /* Cannot "break;" here. */
> >>  
> >>          default:
> >>              break;
> >>          }
> >>  
> >> + end:
> >> +
> >> +#ifdef CONFIG_VIDEO
> >> +    if ( video )
> >> +        video->orig_video_isVGA = 0x23;
> > 
> > I see we use this elsewhere, what's the meaning of this (magic) 0x23?
> 
> This is a value Linux uses (also as a plain number without any #define
> iirc; at least it was that way when we first inherited that value).
> Short of knowing where they took it from or what meaning they associate
> with the value it would be hard for us to give this a (meaningful) name
> and hence use a #define. And hence it's equally hard to properly answer
> your question.

OK, so it's a magic value. I'm happy with the rest, so:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.
diff mbox series

Patch

--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -53,6 +53,7 @@  typedef unsigned int u32;
 typedef unsigned long long u64;
 typedef unsigned int size_t;
 typedef u8 uint8_t;
+typedef u16 uint16_t;
 typedef u32 uint32_t;
 typedef u64 uint64_t;
 
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -562,12 +562,18 @@  trampoline_setup:
         mov     %esi, sym_esi(xen_phys_start)
         mov     %esi, sym_esi(trampoline_xen_phys_start)
 
-        mov     sym_esi(trampoline_phys), %ecx
-
         /* Get bottom-most low-memory stack address. */
+        mov     sym_esi(trampoline_phys), %ecx
         add     $TRAMPOLINE_SPACE,%ecx
 
+#ifdef CONFIG_VIDEO
+        lea     sym_esi(boot_vid_info), %edx
+#else
+        xor     %edx, %edx
+#endif
+
         /* Save Multiboot / PVH info struct (after relocation) for later use. */
+        push    %edx                /* Boot video info to be filled from MB2. */
         push    %ecx                /* Bottom-most low-memory stack address. */
         push    %ebx                /* Multiboot / PVH information address. */
         push    %eax                /* Magic number. */
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -14,9 +14,10 @@ 
 
 /*
  * This entry point is entered from xen/arch/x86/boot/head.S with:
- *   - 0x4(%esp) = MAGIC,
- *   - 0x8(%esp) = INFORMATION_ADDRESS,
- *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
+ *   - 0x04(%esp) = MAGIC,
+ *   - 0x08(%esp) = INFORMATION_ADDRESS,
+ *   - 0x0c(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
+ *   - 0x10(%esp) = BOOT_VIDEO_INFO_ADDRESS.
  */
 asm (
     "    .text                         \n"
@@ -32,6 +33,39 @@  asm (
 #include "../../../include/xen/kconfig.h"
 #include <public/arch-x86/hvm/start_info.h>
 
+#ifdef CONFIG_VIDEO
+# include "video.h"
+
+/* VESA control information */
+struct __packed vesa_ctrl_info {
+    uint8_t signature[4];
+    uint16_t version;
+    uint32_t oem_name;
+    uint32_t capabilities;
+    uint32_t mode_list;
+    uint16_t mem_size;
+    /* We don't use any further fields. */
+};
+
+/* VESA 2.0 mode information */
+struct vesa_mode_info {
+    uint16_t attrib;
+    uint8_t window[14]; /* We don't use the individual fields. */
+    uint16_t bytes_per_line;
+    uint16_t width;
+    uint16_t height;
+    uint8_t cell_width;
+    uint8_t cell_height;
+    uint8_t nr_planes;
+    uint8_t depth;
+    uint8_t memory[5]; /* We don't use the individual fields. */
+    struct boot_video_colors colors;
+    uint8_t direct_color;
+    uint32_t base;
+    /* We don't use any further fields. */
+};
+#endif /* CONFIG_VIDEO */
+
 #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t *)(tag))->member)
 #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
 
@@ -146,7 +180,7 @@  static multiboot_info_t *mbi_reloc(u32 m
     return mbi_out;
 }
 
-static multiboot_info_t *mbi2_reloc(u32 mbi_in)
+static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out)
 {
     const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
     const multiboot2_memory_map_t *mmap_src;
@@ -154,6 +188,9 @@  static multiboot_info_t *mbi2_reloc(u32
     module_t *mbi_out_mods = NULL;
     memory_map_t *mmap_dst;
     multiboot_info_t *mbi_out;
+#ifdef CONFIG_VIDEO
+    struct boot_video_info *video = NULL;
+#endif
     u32 ptr;
     unsigned int i, mod_idx = 0;
 
@@ -254,17 +291,64 @@  static multiboot_info_t *mbi2_reloc(u32
             ++mod_idx;
             break;
 
+#ifdef CONFIG_VIDEO
+        case MULTIBOOT2_TAG_TYPE_VBE:
+            if ( video_out )
+            {
+                const struct vesa_ctrl_info *ci;
+                const struct vesa_mode_info *mi;
+
+                video = _p(video_out);
+                ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
+                mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
+
+                if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
+                {
+                    video->capabilities = ci->capabilities;
+                    video->lfb_linelength = mi->bytes_per_line;
+                    video->lfb_width = mi->width;
+                    video->lfb_height = mi->height;
+                    video->lfb_depth = mi->depth;
+                    video->lfb_base = mi->base;
+                    video->lfb_size = ci->mem_size;
+                    video->colors = mi->colors;
+                    video->vesa_attrib = mi->attrib;
+                }
+
+                video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
+                video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
+            }
+            break;
+
+        case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
+            if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
+                  MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
+            {
+                video_out = 0;
+                video = NULL;
+            }
+            break;
+#endif /* CONFIG_VIDEO */
+
         case MULTIBOOT2_TAG_TYPE_END:
-            return mbi_out;
+            goto end; /* Cannot "break;" here. */
 
         default:
             break;
         }
 
+ end:
+
+#ifdef CONFIG_VIDEO
+    if ( video )
+        video->orig_video_isVGA = 0x23;
+#endif
+
     return mbi_out;
 }
 
-void * __stdcall reloc(u32 magic, u32 in, u32 trampoline)
+void *__stdcall reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
+                      uint32_t video_info)
 {
     alloc = trampoline;
 
@@ -274,7 +358,7 @@  void * __stdcall reloc(u32 magic, u32 in
         return mbi_reloc(in);
 
     case MULTIBOOT2_BOOTLOADER_MAGIC:
-        return mbi2_reloc(in);
+        return mbi2_reloc(in, video_info);
 
     case XEN_HVM_START_MAGIC_VALUE:
         if ( IS_ENABLED(CONFIG_PVH_GUEST) )
--- a/xen/arch/x86/boot/video.h
+++ b/xen/arch/x86/boot/video.h
@@ -28,4 +28,45 @@ 
 /* The "recalculate timings" flag */
 #define VIDEO_RECALC        0x8000
 
+#ifndef __ASSEMBLY__
+struct boot_video_info {
+    uint8_t  orig_x;             /* 0x00 */
+    uint8_t  orig_y;             /* 0x01 */
+    uint8_t  orig_video_mode;    /* 0x02 */
+    uint8_t  orig_video_cols;    /* 0x03 */
+    uint8_t  orig_video_lines;   /* 0x04 */
+    uint8_t  orig_video_isVGA;   /* 0x05 */
+    uint16_t orig_video_points;  /* 0x06 */
+
+    /* VESA graphic mode -- linear frame buffer */
+    uint32_t capabilities;       /* 0x08 */
+    uint16_t lfb_linelength;     /* 0x0c */
+    uint16_t lfb_width;          /* 0x0e */
+    uint16_t lfb_height;         /* 0x10 */
+    uint16_t lfb_depth;          /* 0x12 */
+    uint32_t lfb_base;           /* 0x14 */
+    uint32_t lfb_size;           /* 0x18 */
+    union {
+        struct {
+            uint8_t  red_size;   /* 0x1c */
+            uint8_t  red_pos;    /* 0x1d */
+            uint8_t  green_size; /* 0x1e */
+            uint8_t  green_pos;  /* 0x1f */
+            uint8_t  blue_size;  /* 0x20 */
+            uint8_t  blue_pos;   /* 0x21 */
+            uint8_t  rsvd_size;  /* 0x22 */
+            uint8_t  rsvd_pos;   /* 0x23 */
+        };
+        struct boot_video_colors {
+            uint8_t  rgbr[8];
+        } colors;
+    };
+    struct {
+        uint16_t seg;            /* 0x24 */
+        uint16_t off;            /* 0x26 */
+    } vesapm;
+    uint16_t vesa_attrib;        /* 0x28 */
+};
+#endif /* __ASSEMBLY__ */
+
 #endif /* __BOOT_VIDEO_H__ */
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -532,35 +532,7 @@  static void __init setup_max_pdx(unsigne
 static struct e820map __initdata boot_e820;
 
 #ifdef CONFIG_VIDEO
-struct boot_video_info {
-    u8  orig_x;             /* 0x00 */
-    u8  orig_y;             /* 0x01 */
-    u8  orig_video_mode;    /* 0x02 */
-    u8  orig_video_cols;    /* 0x03 */
-    u8  orig_video_lines;   /* 0x04 */
-    u8  orig_video_isVGA;   /* 0x05 */
-    u16 orig_video_points;  /* 0x06 */
-
-    /* VESA graphic mode -- linear frame buffer */
-    u32 capabilities;       /* 0x08 */
-    u16 lfb_linelength;     /* 0x0c */
-    u16 lfb_width;          /* 0x0e */
-    u16 lfb_height;         /* 0x10 */
-    u16 lfb_depth;          /* 0x12 */
-    u32 lfb_base;           /* 0x14 */
-    u32 lfb_size;           /* 0x18 */
-    u8  red_size;           /* 0x1c */
-    u8  red_pos;            /* 0x1d */
-    u8  green_size;         /* 0x1e */
-    u8  green_pos;          /* 0x1f */
-    u8  blue_size;          /* 0x20 */
-    u8  blue_pos;           /* 0x21 */
-    u8  rsvd_size;          /* 0x22 */
-    u8  rsvd_pos;           /* 0x23 */
-    u16 vesapm_seg;         /* 0x24 */
-    u16 vesapm_off;         /* 0x26 */
-    u16 vesa_attrib;        /* 0x28 */
-};
+# include "boot/video.h"
 extern struct boot_video_info boot_vid_info;
 #endif
 
--- a/xen/include/xen/multiboot2.h
+++ b/xen/include/xen/multiboot2.h
@@ -158,6 +158,59 @@  typedef struct {
     multiboot2_memory_map_t entries[];
 } multiboot2_tag_mmap_t;
 
+typedef struct
+{
+    uint32_t type;
+    uint32_t size;
+    uint16_t vbe_mode;
+    uint16_t vbe_interface_seg;
+    uint16_t vbe_interface_off;
+    uint16_t vbe_interface_len;
+    uint8_t vbe_control_info[512];
+    uint8_t vbe_mode_info[256];
+} multiboot2_tag_vbe_t;
+
+typedef struct
+{
+    uint8_t red;
+    uint8_t green;
+    uint8_t blue;
+} multiboot2_color_t;
+
+typedef struct
+{
+    uint32_t type;
+    uint32_t size;
+    uint64_t framebuffer_addr;
+    uint32_t framebuffer_pitch;
+    uint32_t framebuffer_width;
+    uint32_t framebuffer_height;
+    uint8_t framebuffer_bpp;
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_INDEXED  0
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_RGB      1
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_EGA_TEXT 2
+    uint8_t framebuffer_type;
+    uint16_t reserved;
+
+    union
+    {
+        struct
+        {
+            uint16_t framebuffer_palette_num_colors;
+            multiboot2_color_t framebuffer_palette[];
+        };
+        struct
+        {
+            uint8_t framebuffer_red_field_position;
+            uint8_t framebuffer_red_mask_size;
+            uint8_t framebuffer_green_field_position;
+            uint8_t framebuffer_green_mask_size;
+            uint8_t framebuffer_blue_field_position;
+            uint8_t framebuffer_blue_mask_size;
+        };
+    };
+} multiboot2_tag_framebuffer_t;
+
 typedef struct {
     u32 type;
     u32 size;