Re: [PATCH] i386/linux: add support for ext_lfb_base (v2)

[Daniel Kiper]

On Thu, Jul 12, 2018 at 03:09:12PM +0530, Arindam Nath wrote:
> From: Arindam Nath <address@hidden>
>
> v2: changes suggested by Daniel
>
>   - added #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
>   - moved constant definitions to the beginning of header file
>
> v1:

This should go...

> The EFI Graphics Output Protocol can return a 64-bit
> linear frame buffer address in some firmware/BIOS
> implementations. We currently only store the lower
> 32-bits in the lfb_base. This will eventually be
> passed to Linux kernel and the efifb driver will
> incorrectly interpret the framebuffer address as
> 32-bit address.
>
> The Linux kernel has already added support to handle
> 64-bit linear framebuffer address in the efifb driver
> since quite some time now.
>
> This patch adds the support for 64-bit linear frame
> buffer address in GRUB to address the above mentioned
> scenario.
>
> Signed-off-by: Arindam Nath <address@hidden>
> ---

...here. You should add "---" after SOB and put info
about the versions after it. You can find good examples
on the list.

>  grub-core/loader/i386/linux.c |  4 ++++
>  include/grub/i386/linux.h     | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 083f9417c..246945abe 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -306,6 +306,10 @@ grub_linux_setup_video (struct linux_kernel_params 
> *params)
>    params->lfb_line_len = mode_info.pitch;
>
>    params->lfb_base = (grub_size_t) framebuffer;
> +#if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)

Please add one empty line before "#if ...".

> +  params->ext_lfb_base = (grub_size_t) (((grub_uint64_t)(grub_size_t) 
> framebuffer) >> 32);
> +  params->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
> +#endif

Please add one empty line after "#endif".

>    params->lfb_size = ALIGN_UP (params->lfb_line_len * params->lfb_height, 
> 65536);
>
>    params->red_mask_size = mode_info.red_mask_size;
> diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
> index da0ca3b83..04d277b4e 100644
> --- a/include/grub/i386/linux.h
> +++ b/include/grub/i386/linux.h
> @@ -43,6 +43,13 @@
>
>  #define GRUB_LINUX_CL_MAGIC          0xA33F
>
> +#if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
> +
> +#define VIDEO_CAPABILITY_SKIP_QUIRKS (1 << 0)
> +#define VIDEO_CAPABILITY_64BIT_BASE  (1 << 1)        /* Frame buffer base is 
> 64-bit */
> +
> +#endif

Please drop this #if/#endif. It is not needed here.

> +
>  #ifdef __x86_64__
>
>  #define GRUB_LINUX_EFI_SIGNATURE     \
> @@ -188,8 +195,11 @@ struct linux_kernel_params
>    grub_uint16_t lfb_pages;           /* 32 */
>    grub_uint16_t vesa_attrib;         /* 34 */
>    grub_uint32_t capabilities;                /* 36 */
> +#if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
> +  grub_uint32_t ext_lfb_base;                /* 3a */
> +#endif

Ditto.

>
> -  grub_uint8_t padding3[0x40 - 0x3a];
> +  grub_uint8_t padding3[0x40 - 0x3e];

And this will be wrong if you do not drop #if/#endif above.

Daniel