Re: [PATCH 2/2] x86: Set up framebuffer given by Multiboot2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2022 14:09, Tu Dinh Ngoc wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -551,16 +551,55 @@ struct boot_video_info {
>  extern struct boot_video_info boot_vid_info;
>  #endif
>  
> -static void __init parse_video_info(void)
> +static void __init parse_video_info(multiboot_info_t *mbi)

The parameter can be pointer-to-const afaict.

>  {
>  #ifdef CONFIG_VIDEO
>      struct boot_video_info *bvi = &bootsym(boot_vid_info);
>  
> +    /*
> +     * fb detection will be in this order:
> +     *  - efifb (as efifb probe sets a new GOP mode before parse_video_info 
> is called,
> +     *    we must use this mode instead of the one given by mbifb)
> +     *  - mbifb
> +     *  - vesafb
> +     */

This ordering needs justification, first and foremost because this way
you risk regressions when VESAFB data is also available. There would be
no such risk if you made mbifb lowest priority.

Style: Comments should start with an upper case letter. There are very
few exceptions to this (e.g. when a comment starts with an
identifier referring elsewhere), but here there's no problem with
starting the comment "FB detection ...".

>      /* vga_console_info is filled directly on EFI platform. */
>      if ( efi_enabled(EFI_BOOT) )
>          return;
>  
> -    if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) )
> +    if ( mbi->flags & MBI_FB )

Even with MBI_FB's value corrected in patch 1 - what about the flag
being set from an MB1 bootloader? I'd be hesitant to trust that the
data is dependable upon everywhere.

> +    {
> +        uint64_t lfb_rgb_bitmap = 0;

I don't think you really need this initializer.

> +        vga_console_info.video_type = XEN_VGATYPE_VESA_LFB;
> +        vga_console_info.u.vesa_lfb.width = mbi->fb.width;
> +        vga_console_info.u.vesa_lfb.height = mbi->fb.height;
> +        vga_console_info.u.vesa_lfb.bytes_per_line = mbi->fb.pitch;
> +        vga_console_info.u.vesa_lfb.bits_per_pixel = mbi->fb.bpp;
> +        vga_console_info.u.vesa_lfb.lfb_base = mbi->fb.addr;
> +        vga_console_info.u.vesa_lfb.lfb_size = (mbi->fb.pitch * 
> mbi->fb.height + 0xffff) >> 16;
> +
> +        vga_console_info.u.vesa_lfb.red_pos = mbi->fb.red_pos;
> +        vga_console_info.u.vesa_lfb.red_size = mbi->fb.red_size;
> +        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.red_size) - 1) << 
> mbi->fb.red_pos;

1ull is both shorter and avoids using a cast.

> +        vga_console_info.u.vesa_lfb.green_pos = mbi->fb.green_pos;
> +        vga_console_info.u.vesa_lfb.green_size = mbi->fb.green_size;
> +        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.green_size) - 1) << 
> mbi->fb.green_pos;
> +        vga_console_info.u.vesa_lfb.blue_pos = mbi->fb.blue_pos;
> +        vga_console_info.u.vesa_lfb.blue_size = mbi->fb.blue_size;
> +        lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.blue_size) - 1) << 
> mbi->fb.blue_pos;
> +
> +        /* assume non-weird bit format */
> +        vga_console_info.u.vesa_lfb.rsvd_pos = 
> find_first_zero_bit(&lfb_rgb_bitmap, sizeof(lfb_rgb_bitmap) * __CHAR_BIT__);
> +        vga_console_info.u.vesa_lfb.rsvd_size = mbi->fb.bpp - 
> mbi->fb.red_size - mbi->fb.green_size - mbi->fb.blue_size;

The comment really is about this 2nd assignment afaict, so it would better
move down. I'm not convinced "assume" is enough, though. I think the data
should simply not be used if the color placement doesn't match our
expectations.

Also these are overly long lines again.

Finally if lfb_rgb_bitmap was "unsigned long" (and hence still at least
64 bits, as we're on a 64-bit architecture) you could use the simpler
(because not requiring the address to be taken)
find_first_set_bit(~lfb_rgb_bitmap), provided of course lfb_rgb_bitmap
doesn't have all bits set.

> +        if (vga_console_info.u.vesa_lfb.rsvd_pos >= mbi->fb.bpp || 
> vga_console_info.u.vesa_lfb.rsvd_size < 0) {

Style: Missing blanks, line length, and brace placement.

Jan