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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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