Skip to contentSkip to conversation

efi/earlycon: Remap entire framebuffer after page initialization

PATCH In review Ard Biesheuvel 3 participants 7 messages Dec 5, 2019 EFI

lore
Share & tools
lore

Ard Biesheuvel

author maintainer
#0 lore raw

PATCH v4 1/1 efi/earlycon: Remap entire framebuffer after page initialization

From: Andy Shevchenko <[email protected]>

When commit 69c1f396f25b

"efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation"

moved the x86 specific EFI earlyprintk implementation to a shared location, it also tweaked the behaviour. In particular, it dropped a trick with full framebuffer remapping after page initialization, leading to two regressions:
1) very slow scrolling after page initialization,
2) kernel hang when the 'keep_bootcon' command line argument is passed.

Putting the tweak back fixes #2 and mitigates #1, i.e., it limits the slow behavior to the early boot stages, 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")
Signed-off-by Andy Shevchenko <[email protected]>
[ardb: ensure efifb is unmapped again unless keep_bootcon is in effect]
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/earlycon.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/firmware/efi/earlycon.c +40 -0
index c9a0efca17b0..0bc4fe741415 100644
@@ -13,18 +13,57 @@
 
 #include <asm/early_ioremap.h>
 
+static const struct console *earlycon_console __initdata;
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
 static u64 fb_base;
 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 efi_earlycon_remap_fb(void)
+{
+	/* bail if there is no bootconsole or it has been disabled already */
+	if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+		return 0;
+
+	if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
+		efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WB);
+	else
+		efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WC);
+
+	return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(efi_earlycon_remap_fb);
+
+static int __init efi_earlycon_unmap_fb(void)
+{
+	/* unmap the bootconsole fb unless keep_bootcon has left it enabled */
+	if (efi_fb && !(earlycon_console->flags & CON_ENABLED))
+		memunmap(efi_fb);
+	return 0;
+}
+late_initcall(efi_earlycon_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);
 }
 

@@ -201,6 +240,7 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
 		efi_earlycon_scroll_up();
 
 	device->con->write = efi_earlycon_write;
+	earlycon_console = device->con;
 	return 0;
 }
 EARLYCON_DECLARE(efifb, efi_earlycon_setup);
Signature · 1 line
2.17.1
#1 lore raw
Quoted 69 lines · Ard Biesheuvel

From: Andy Shevchenko <[email protected]>

When commit 69c1f396f25b

"efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation"

moved the x86 specific EFI earlyprintk implementation to a shared location, it also tweaked the behaviour. In particular, it dropped a trick with full framebuffer remapping after page initialization, leading to two regressions:
1) very slow scrolling after page initialization,
2) kernel hang when the 'keep_bootcon' command line argument is passed.

Putting the tweak back fixes #2 and mitigates #1, i.e., it limits the slow behavior to the early boot stages, 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")
Signed-off-by Andy Shevchenko <[email protected]>
[ardb: ensure efifb is unmapped again unless keep_bootcon is in effect]
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/earlycon.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/firmware/efi/earlycon.c +32 -0
index c9a0efca17b0..0bc4fe741415 100644
@@ -13,18 +13,57 @@
 
 #include <asm/early_ioremap.h>
 
+static const struct console *earlycon_console __initdata;
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
 static u64 fb_base;
 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 efi_earlycon_remap_fb(void)
+{
+       /* bail if there is no bootconsole or it has been disabled already */
+       if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+               return 0;
+
+       if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
+               efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WB);
+       else
+               efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WC);
+
+       return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(efi_earlycon_remap_fb);
+
+static int __init efi_earlycon_unmap_fb(void)
+{
+       /* unmap the bootconsole fb unless keep_bootcon has left it enabled */
+       if (efi_fb && !(earlycon_console->flags & CON_ENABLED))
+               memunmap(efi_fb);
+       return 0;
+}

drivers/firmware/efi/earlycon.c line 61

Quoted 10 lines
drivers/firmware/efi/earlycon.c +4 -0
+late_initcall(efi_earlycon_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));
 }

Actually, now I'm not so sure we really need the early map/unmap for every line. If we switch from an early mapping to an ordinary mapping at early_initcall() time, we can just map it once, and tear it down when we do the ordinary memremap()

Quoted 19 lines
drivers/firmware/efi/earlycon.c +4 -1
 static __ref void efi_earlycon_unmap(void *addr, unsigned long len)
 {
+       if (efi_fb)
+               return;
+
        early_memunmap(addr, len);
 }
 

@@ -201,6 +240,7 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
                efi_earlycon_scroll_up();
 
        device->con->write = efi_earlycon_write;
+       earlycon_console = device->con;
        return 0;
 }
 EARLYCON_DECLARE(efifb, efi_earlycon_setup);
--

2.17.1

#2 lore raw
Quoted 73 lines · Ard Biesheuvel
Quoted 71 lines · Ard Biesheuvel

From: Andy Shevchenko <[email protected]>

When commit 69c1f396f25b

"efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation"

moved the x86 specific EFI earlyprintk implementation to a shared location, it also tweaked the behaviour. In particular, it dropped a trick with full framebuffer remapping after page initialization, leading to two regressions:
1) very slow scrolling after page initialization,
2) kernel hang when the 'keep_bootcon' command line argument is passed.

Putting the tweak back fixes #2 and mitigates #1, i.e., it limits the slow behavior to the early boot stages, 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")
Signed-off-by Andy Shevchenko <[email protected]>
[ardb: ensure efifb is unmapped again unless keep_bootcon is in effect]
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/earlycon.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/firmware/efi/earlycon.c +33 -0
index c9a0efca17b0..0bc4fe741415 100644
@@ -13,18 +13,57 @@
 
 #include <asm/early_ioremap.h>
 
+static const struct console *earlycon_console __initdata;
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
 static u64 fb_base;
 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 efi_earlycon_remap_fb(void)
+{
+       /* bail if there is no bootconsole or it has been disabled already */
+       if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+               return 0;
+
+       if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
+               efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WB);
+       else
+               efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WC);
+
+       return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(efi_earlycon_remap_fb);
+
+static int __init efi_earlycon_unmap_fb(void)
+{
+       /* unmap the bootconsole fb unless keep_bootcon has left it enabled */
+       if (efi_fb && !(earlycon_console->flags & CON_ENABLED))
+               memunmap(efi_fb);
+       return 0;
+}
+late_initcall(efi_earlycon_unmap_fb);
Quoted 14 lines

drivers/firmware/efi/earlycon.c line 61

Quoted 8 lines
drivers/firmware/efi/earlycon.c +3 -0
 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));
 }

Actually, now I'm not so sure we really need the early map/unmap for every line. If we switch from an early mapping to an ordinary mapping at early_initcall() time, we can just map it once, and tear it down when we do the ordinary memremap()

Never mind ... early_memremap() cannot map the entire framebuffer in one go.

Quoted 20 lines
Quoted 20 lines
drivers/firmware/efi/earlycon.c +4 -1
 static __ref void efi_earlycon_unmap(void *addr, unsigned long len)
 {
+       if (efi_fb)
+               return;
+
        early_memunmap(addr, len);
 }
 

@@ -201,6 +240,7 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
                efi_earlycon_scroll_up();
 
        device->con->write = efi_earlycon_write;
+       earlycon_console = device->con;
        return 0;
 }
 EARLYCON_DECLARE(efifb, efi_earlycon_setup);
--

2.17.1

#3 lore raw
In reply to Ard Biesheuvel
Quoted 56 lines · Ard Biesheuvel

From: Andy Shevchenko <[email protected]>

When commit 69c1f396f25b

"efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation"

moved the x86 specific EFI earlyprintk implementation to a shared location, it also tweaked the behaviour. In particular, it dropped a trick with full framebuffer remapping after page initialization, leading to two regressions:
1) very slow scrolling after page initialization,
2) kernel hang when the 'keep_bootcon' command line argument is passed.

Putting the tweak back fixes #2 and mitigates #1, i.e., it limits the slow behavior to the early boot stages, 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")
Signed-off-by Andy Shevchenko <[email protected]>
[ardb: ensure efifb is unmapped again unless keep_bootcon is in effect]
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/earlycon.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/firmware/efi/earlycon.c +20 -0
index c9a0efca17b0..0bc4fe741415 100644
@@ -13,18 +13,57 @@
 
 #include <asm/early_ioremap.h>
 
+static const struct console *earlycon_console __initdata;
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
 static u64 fb_base;
 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 efi_earlycon_remap_fb(void)
+{
+	/* bail if there is no bootconsole or it has been disabled already */
+	if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+		return 0;
+
+	if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
+		efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WB);
+	else
+		efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WC);

drivers/firmware/efi/earlycon.c line 49

Quoted 10 lines
drivers/firmware/efi/earlycon.c +9 -0
+
+	return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(efi_earlycon_remap_fb);
+
+static int __init efi_earlycon_unmap_fb(void)
+{
+	/* unmap the bootconsole fb unless keep_bootcon has left it enabled */
 
+	if (efi_fb && !(earlycon_console->flags & CON_ENABLED))

Isn't efi_fb test superfluous here?

drivers/firmware/efi/earlycon.c line 52

Quoted 5 lines
drivers/firmware/efi/earlycon.c +4 -0
+		memunmap(efi_fb);
 
+	return 0;
+}
+late_initcall(efi_earlycon_unmap_fb);

I still think we need __exitcall() to clean up the stuff in any case.

Quoted 28 lines
drivers/firmware/efi/earlycon.c +7 -0
 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);
 }

@@ -201,6 +240,7 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
 		efi_earlycon_scroll_up();
 
 	device->con->write = efi_earlycon_write;
+	earlycon_console = device->con;
 	return 0;
 }
 EARLYCON_DECLARE(efifb, efi_earlycon_setup);
Signature · 1 line
2.17.1
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw
Quoted 67 lines · Andy Shevchenko
Quoted 64 lines · Ard Biesheuvel

From: Andy Shevchenko <[email protected]>

When commit 69c1f396f25b

"efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation"

moved the x86 specific EFI earlyprintk implementation to a shared location, it also tweaked the behaviour. In particular, it dropped a trick with full framebuffer remapping after page initialization, leading to two regressions:
1) very slow scrolling after page initialization,
2) kernel hang when the 'keep_bootcon' command line argument is passed.

Putting the tweak back fixes #2 and mitigates #1, i.e., it limits the slow behavior to the early boot stages, 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")
Signed-off-by Andy Shevchenko <[email protected]>
[ardb: ensure efifb is unmapped again unless keep_bootcon is in effect]
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/earlycon.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/firmware/efi/earlycon.c +28 -0
index c9a0efca17b0..0bc4fe741415 100644
@@ -13,18 +13,57 @@
 
 #include <asm/early_ioremap.h>
 
+static const struct console *earlycon_console __initdata;
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
 static u64 fb_base;
 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 efi_earlycon_remap_fb(void)
+{
+     /* bail if there is no bootconsole or it has been disabled already */
+     if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+             return 0;
+
+     if (pgprot_val(fb_prot) == pgprot_val(PAGE_KERNEL))
+             efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WB);
+     else
+             efi_fb = memremap(fb_base, screen_info.lfb_size, MEMREMAP_WC);
+
+     return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(efi_earlycon_remap_fb);
+
+static int __init efi_earlycon_unmap_fb(void)
+{
+     /* unmap the bootconsole fb unless keep_bootcon has left it enabled */
Quoted 4 lines

drivers/firmware/efi/earlycon.c line 48

Quoted 1 line
drivers/firmware/efi/earlycon.c +1 -0
+     if (efi_fb && !(earlycon_console->flags & CON_ENABLED))

Isn't efi_fb test superfluous here?

Well, it could be NULL so it is not superfluous. The initcall() is always executed, even if you are not using earlycon=efifb at all.

Quoted 8 lines

drivers/firmware/efi/earlycon.c line 52

Quoted 5 lines
drivers/firmware/efi/earlycon.c +4 -0
+             memunmap(efi_fb);
 
+     return 0;
+}
+late_initcall(efi_earlycon_unmap_fb);

I still think we need __exitcall() to clean up the stuff in any case.

Why? As far as I can tell, exitcalls go straight into a /DISCARD/ section, and I really don't see the point of carrying dead code in this file.

Quoted 34 lines
Quoted 26 lines
drivers/firmware/efi/earlycon.c +7 -1
 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);
 }
 

@@ -201,6 +240,7 @@ static int __init efi_earlycon_setup(struct earlycon_device *device,
              efi_earlycon_scroll_up();
 
      device->con->write = efi_earlycon_write;
+     earlycon_console = device->con;
      return 0;
 }
 EARLYCON_DECLARE(efifb, efi_earlycon_setup);
--
Quoted 2 lines

2.17.1

--
With Best Regards,
Andy Shevchenko

#5 lore raw
Quoted 8 lines · Ard Biesheuvel
Quoted 6 lines · Andy Shevchenko
Quoted 3 lines · Ard Biesheuvel
drivers/firmware/efi/earlycon.c +3 -0
+static int __init efi_earlycon_unmap_fb(void)
+{
+     /* unmap the bootconsole fb unless keep_bootcon has left it enabled */
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

+ if (efi_fb && !(earlycon_console->flags & CON_ENABLED))

Isn't efi_fb test superfluous here?

Well, it could be NULL so it is not superfluous. The initcall() is always executed, even if you are not using earlycon=efifb at all.

My point is that memunmap() is no-op for NULL pointer, isn't it?

Quoted 9 lines
Quoted 8 lines

drivers/firmware/efi/earlycon.c line 52

Quoted 5 lines
drivers/firmware/efi/earlycon.c +4 -0
+             memunmap(efi_fb);
 
+     return 0;
+}
+late_initcall(efi_earlycon_unmap_fb);

I still think we need __exitcall() to clean up the stuff in any case.

Quoted 3 lines

Why? As far as I can tell, exitcalls go straight into a /DISCARD/ section, and I really don't see the point of carrying dead code in this file.

Okay.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#6 lore raw
Quoted 12 lines · Andy Shevchenko
Quoted 10 lines · Ard Biesheuvel
Quoted 8 lines · Andy Shevchenko
Quoted 5 lines · Ard Biesheuvel
drivers/firmware/efi/earlycon.c +4 -0
+static int __init efi_earlycon_unmap_fb(void)
+{
+     /* unmap the bootconsole fb unless keep_bootcon has left it enabled */
 
+     if (efi_fb && !(earlycon_console->flags & CON_ENABLED))
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Isn't efi_fb test superfluous here?

Well, it could be NULL so it is not superfluous. The initcall() is always executed, even if you are not using earlycon=efifb at all.

My point is that memunmap() is no-op for NULL pointer, isn't it?

Right. So I could check for earlycon_console != NULL instead, since that is implied by efi_fb != NULL, but it could be NULL otherwise, in which case I should not dereference it to get at the flags. But I need two checks in any case, so I think this is OK.

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

drivers/firmware/efi/earlycon.c line 52

Quoted 5 lines
drivers/firmware/efi/earlycon.c +4 -0
+             memunmap(efi_fb);
 
+     return 0;
+}
+late_initcall(efi_earlycon_unmap_fb);

I still think we need __exitcall() to clean up the stuff in any case.

Quoted 6 lines
Quoted 3 lines

Why? As far as I can tell, exitcalls go straight into a /DISCARD/ section, and I really don't see the point of carrying dead code in this file.

Okay.

Thanks,