Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader
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
> >>
> >> +#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
Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader
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
>>
>> +#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;
>> }
>>
Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader
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
> ---
> 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
>
> +#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 =
[PATCH v4 2/8] x86/boot: obtain video info from boot loader
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
---
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
+#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
+#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
