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 <jbeul...@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.
