[PATCH v3] efi/earlycon: Remap entire framebuffer after page initialization

[PATCH v3] efi/earlycon: Remap entire framebuffer after page initialization

[Andy Shevchenko]

When the commit 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk
into generic earlycon implementation") moved x86 specific EFI earlyprintk
implementation to shared location it also tweaked the behaviour. In particular
it dropped a trick with full framebuffer remapping after page initialization.
This lead to two regressions:
1) very slow scrolling after page initialization;
2) kernel hang when keep_bootcon parameter is being provided.

Returning the trick back fixes #2 and mitigates, i.e. reduces the window when
slowness appears, #1 presumably due to eliminating heavy map()/unmap()
operations per each pixel line on the screen.

Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Alexander Graf <agraf@suse.de>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3:
- assign size in efi_earlycon_setup()
- check base and size before mapping in early_efi_map_fb()
 drivers/firmware/efi/earlycon.c | 37 +++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/firmware/efi/earlycon.c b/drivers/firmware/efi/earlycon.c
index c9a0efca17b0..60b5350c6105 100644
--- a/drivers/firmware/efi/earlycon.c
+++ b/drivers/firmware/efi/earlycon.c
@@ -16,15 +16,50 @@
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
 static u64 fb_base;
+static u32 fb_size;
 static pgprot_t fb_prot;
+static void *efi_fb;
+
+/*
+ * efi earlycon needs to use early_memremap() to map the framebuffer.
+ * But early_memremap() is not usable for 'earlycon=efifb keep_bootcon',
+ * memremap() should be used instead. memremap() will be available after
+ * paging_init() which is earlier than initcall callbacks. Thus adding this
+ * early initcall function early_efi_map_fb() to map the whole efi framebuffer.
+ */
+static int __init early_efi_map_fb(void)
+{
+	if (!fb_base || !fb_size)
+		return NULL;
+
+	if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
+		efi_fb = memremap(fb_base, fb_size, MEMREMAP_WB);
+	else
+		efi_fb = memremap(fb_base, fb_size, MEMREMAP_WC);
+
+	return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(early_efi_map_fb);
+
+static void __exit early_efi_unmap_fb(void)
+{
+	memunmap(efi_fb);
+}
+__exitcall(early_efi_unmap_fb);
 
 static __ref void *efi_earlycon_map(unsigned long start, unsigned long len)
 {
+	if (efi_fb)
+		return efi_fb + start;
+
 	return early_memremap_prot(fb_base + start, len, pgprot_val(fb_prot));
 }
 
 static __ref void efi_earlycon_unmap(void *addr, unsigned long len)
 {
+	if (efi_fb)
+		return;
+
 	early_memunmap(addr, len);
 }
 
@@ -176,6 +211,8 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
 	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
 		fb_base |= (u64)screen_info.ext_lfb_base << 32;
 
+	fb_size = screen_info.lfb_size;
+
 	if (opt && !strcmp(opt, "ram"))
 		fb_prot = PAGE_KERNEL;
 	else
-- 
2.24.0

Re: [PATCH v3] efi/earlycon: Remap entire framebuffer after page initialization

[Ard Biesheuvel]

On Fri, 15 Nov 2019 at 15:31, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> When the commit 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk
> into generic earlycon implementation") moved x86 specific EFI earlyprintk
> implementation to shared location it also tweaked the behaviour. In particular
> it dropped a trick with full framebuffer remapping after page initialization.
> This lead to two regressions:
> 1) very slow scrolling after page initialization;
> 2) kernel hang when keep_bootcon parameter is being provided.
>
> Returning the trick back fixes #2 and mitigates, i.e. reduces the window when
> slowness appears, #1 presumably due to eliminating heavy map()/unmap()
> operations per each pixel line on the screen.
>
> Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v3:
> - assign size in efi_earlycon_setup()
> - check base and size before mapping in early_efi_map_fb()
>  drivers/firmware/efi/earlycon.c | 37 +++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/firmware/efi/earlycon.c b/drivers/firmware/efi/earlycon.c
> index c9a0efca17b0..60b5350c6105 100644
> --- a/drivers/firmware/efi/earlycon.c
> +++ b/drivers/firmware/efi/earlycon.c
> @@ -16,15 +16,50 @@
>  static const struct font_desc *font;
>  static u32 efi_x, efi_y;
>  static u64 fb_base;
> +static u32 fb_size;
>  static pgprot_t fb_prot;
> +static void *efi_fb;
> +
> +/*
> + * efi earlycon needs to use early_memremap() to map the framebuffer.
> + * But early_memremap() is not usable for 'earlycon=efifb keep_bootcon',
> + * memremap() should be used instead. memremap() will be available after
> + * paging_init() which is earlier than initcall callbacks. Thus adding this
> + * early initcall function early_efi_map_fb() to map the whole efi framebuffer.
> + */
> +static int __init early_efi_map_fb(void)
> +{
> +       if (!fb_base || !fb_size)
> +               return NULL;
> +

'return 0' please.

I'll test this on actual arm64 hardware somewhere next week, and queue
it if it doesn't break anything (no need to resend for the above)


> +       if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
> +               efi_fb = memremap(fb_base, fb_size, MEMREMAP_WB);
> +       else
> +               efi_fb = memremap(fb_base, fb_size, MEMREMAP_WC);
> +
> +       return efi_fb ? 0 : -ENOMEM;
> +}
> +early_initcall(early_efi_map_fb);
> +
> +static void __exit early_efi_unmap_fb(void)
> +{
> +       memunmap(efi_fb);
> +}
> +__exitcall(early_efi_unmap_fb);
>
>  static __ref void *efi_earlycon_map(unsigned long start, unsigned long len)
>  {
> +       if (efi_fb)
> +               return efi_fb + start;
> +
>         return early_memremap_prot(fb_base + start, len, pgprot_val(fb_prot));
>  }
>
>  static __ref void efi_earlycon_unmap(void *addr, unsigned long len)
>  {
> +       if (efi_fb)
> +               return;
> +
>         early_memunmap(addr, len);
>  }
>
> @@ -176,6 +211,8 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
>         if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>                 fb_base |= (u64)screen_info.ext_lfb_base << 32;
>
> +       fb_size = screen_info.lfb_size;
> +
>         if (opt && !strcmp(opt, "ram"))
>                 fb_prot = PAGE_KERNEL;
>         else
> --
> 2.24.0
>

Re: [PATCH v3] efi/earlycon: Remap entire framebuffer after page initialization

[Andy Shevchenko]

On Fri, Nov 15, 2019 at 04:28:10PM +0000, Ard Biesheuvel wrote:
> On Fri, 15 Nov 2019 at 15:31, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > When the commit 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk
> > into generic earlycon implementation") moved x86 specific EFI earlyprintk
> > implementation to shared location it also tweaked the behaviour. In particular
> > it dropped a trick with full framebuffer remapping after page initialization.
> > This lead to two regressions:
> > 1) very slow scrolling after page initialization;
> > 2) kernel hang when keep_bootcon parameter is being provided.
> >
> > Returning the trick back fixes #2 and mitigates, i.e. reduces the window when
> > slowness appears, #1 presumably due to eliminating heavy map()/unmap()
> > operations per each pixel line on the screen.

> > +/*
> > + * efi earlycon needs to use early_memremap() to map the framebuffer.
> > + * But early_memremap() is not usable for 'earlycon=efifb keep_bootcon',
> > + * memremap() should be used instead. memremap() will be available after
> > + * paging_init() which is earlier than initcall callbacks. Thus adding this
> > + * early initcall function early_efi_map_fb() to map the whole efi framebuffer.
> > + */
> > +static int __init early_efi_map_fb(void)
> > +{
> > +       if (!fb_base || !fb_size)
> > +               return NULL;
> > +
> 
> 'return 0' please.

Ah, sorry. Though I think we rather return -ENOVEV here.

> I'll test this on actual arm64 hardware somewhere next week, and queue
> it if it doesn't break anything (no need to resend for the above)

Thanks!

> > +       if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
> > +               efi_fb = memremap(fb_base, fb_size, MEMREMAP_WB);
> > +       else
> > +               efi_fb = memremap(fb_base, fb_size, MEMREMAP_WC);
> > +
> > +       return efi_fb ? 0 : -ENOMEM;
> > +}
> > +early_initcall(early_efi_map_fb);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] efi/earlycon: Remap entire framebuffer after page initialization

[Ard Biesheuvel]

On Mon, 18 Nov 2019 at 12:45, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Nov 15, 2019 at 04:28:10PM +0000, Ard Biesheuvel wrote:
> > On Fri, 15 Nov 2019 at 15:31, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > When the commit 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk
> > > into generic earlycon implementation") moved x86 specific EFI earlyprintk
> > > implementation to shared location it also tweaked the behaviour. In particular
> > > it dropped a trick with full framebuffer remapping after page initialization.
> > > This lead to two regressions:
> > > 1) very slow scrolling after page initialization;
> > > 2) kernel hang when keep_bootcon parameter is being provided.
> > >
> > > Returning the trick back fixes #2 and mitigates, i.e. reduces the window when
> > > slowness appears, #1 presumably due to eliminating heavy map()/unmap()
> > > operations per each pixel line on the screen.
>
> > > +/*
> > > + * efi earlycon needs to use early_memremap() to map the framebuffer.
> > > + * But early_memremap() is not usable for 'earlycon=efifb keep_bootcon',
> > > + * memremap() should be used instead. memremap() will be available after
> > > + * paging_init() which is earlier than initcall callbacks. Thus adding this
> > > + * early initcall function early_efi_map_fb() to map the whole efi framebuffer.
> > > + */
> > > +static int __init early_efi_map_fb(void)
> > > +{
> > > +       if (!fb_base || !fb_size)
> > > +               return NULL;
> > > +
> >
> > 'return 0' please.
>
> Ah, sorry. Though I think we rather return -ENOVEV here.
>

fb_base and fb_size will only be set if earlycon=efifb is passed, so
this is not an error condition, and so returning an error is
incorrect.

> > I'll test this on actual arm64 hardware somewhere next week, and queue
> > it if it doesn't break anything (no need to resend for the above)
>
> Thanks!
>

Works fine for me

Tested-by: Ard Biesheuvel <ardb@kernel.org> # arm64

> > > +       if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
> > > +               efi_fb = memremap(fb_base, fb_size, MEMREMAP_WB);
> > > +       else
> > > +               efi_fb = memremap(fb_base, fb_size, MEMREMAP_WC);
> > > +
> > > +       return efi_fb ? 0 : -ENOMEM;
> > > +}
> > > +early_initcall(early_efi_map_fb);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>