Re: [PATCH 1/2] x86: Parse Multiboot2 framebuffer information

[Jan Beulich <jbeulich@xxxxxxxx>]

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