RE: [PATCH] i386/linux: add support for ext_lfb_base

[Nath, Arindam]

> -----Original Message-----
> From: Daniel Kiper <address@hidden>
> Sent: Friday, July 6, 2018 11:42 PM
> To: The development of GNU GRUB <address@hidden>
> Cc: Nath, Arindam <address@hidden>
> Subject: Re: [PATCH] i386/linux: add support for ext_lfb_base
> 
> On Wed, Jun 27, 2018 at 08:25:41PM +0530, Arindam Nath wrote:
> > 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>
> > ---
> >  grub-core/loader/i386/linux.c | 2 ++
> >  include/grub/i386/linux.h     | 5 ++++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> > index 083f9417c..1644e8132 100644
> > --- a/grub-core/loader/i386/linux.c
> > +++ b/grub-core/loader/i386/linux.c
> > @@ -306,6 +306,8 @@ grub_linux_setup_video (struct
> linux_kernel_params *params)
> >    params->lfb_line_len = mode_info.pitch;
> >
> >    params->lfb_base = (grub_size_t) framebuffer;
> 
> I think that you should add this:
> 
> #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
> 
> > +  params->ext_lfb_base = (grub_size_t) (((grub_uint64_t)(grub_size_t)
> framebuffer) >> 32);
> > +  params->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
> 
> #endif

That makes sense. Will add the #if in next version of patch.

> 
> .. or at least #ifdef __x86_64__...
> 
> >    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..e3f91e7c0 100644
> > --- a/include/grub/i386/linux.h
> > +++ b/include/grub/i386/linux.h
> > @@ -188,8 +188,11 @@ struct linux_kernel_params
> >    grub_uint16_t lfb_pages;         /* 32 */
> >    grub_uint16_t vesa_attrib;               /* 34 */
> >    grub_uint32_t capabilities;              /* 36 */
> > +#define VIDEO_CAPABILITY_SKIP_QUIRKS       (1 << 0)
> > +#define VIDEO_CAPABILITY_64BIT_BASE        (1 << 1)        /* Frame buffer 
> > base
> is 64-bit */
> 
> My I ask you to put this constants definitions at the
> beginning of the file together with other constants?

Will do. Thanks.

Arindam
> 
> Daniel