[PATCH] efi/earlycon: Fix write-combine mapping on x86

[PATCH] efi/earlycon: Fix write-combine mapping on x86

[Arvind Sankar]

On x86, until PAT is initialized, WC translates into UC-. Since we
calculate and store pgprot_writecombine(PAGE_KERNEL) when earlycon is
initialized, this means we actually use UC- mappings instead of WC
mappings, which makes scrolling very slow.

Instead store a boolean flag to indicate whether we want to use
writeback or write-combine mappings, and recalculate the actual pgprot_t
we need on every mapping. Once PAT is initialized, we will start using
write-combine mappings, which speeds up the scrolling considerably.

Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
This applies on top of
	efi/earlycon: Remap entire framebuffer after page initialization
	https://git.kernel.org/tip/b418d660bb9798d2249ac6a46c844389ef50b6a5

 drivers/firmware/efi/earlycon.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/efi/earlycon.c b/drivers/firmware/efi/earlycon.c
index ee5a6431fb9c..18f6a61f3e17 100644
--- a/drivers/firmware/efi/earlycon.c
+++ b/drivers/firmware/efi/earlycon.c
@@ -17,7 +17,7 @@ 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 bool fb_wb;
 static void *efi_fb;
 
 /*
@@ -33,10 +33,8 @@ static int __init efi_earlycon_remap_fb(void)
 	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);
+	efi_fb = memremap(fb_base, screen_info.lfb_size,
+			  fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
 
 	pr_info("Mapped earlycon framebuffer\n");
 
@@ -58,9 +56,12 @@ late_initcall(efi_earlycon_unmap_fb);
 
 static __ref void *efi_earlycon_map(unsigned long start, unsigned long len)
 {
+	pgprot_t fb_prot;
+
 	if (efi_fb)
 		return efi_fb + start;
 
+	fb_prot = fb_wb ? PAGE_KERNEL : pgprot_writecombine(PAGE_KERNEL);
 	return early_memremap_prot(fb_base + start, len, pgprot_val(fb_prot));
 }
 
@@ -220,10 +221,7 @@ 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;
 
-	if (opt && !strcmp(opt, "ram"))
-		fb_prot = PAGE_KERNEL;
-	else
-		fb_prot = pgprot_writecombine(PAGE_KERNEL);
+	fb_wb = opt && !strcmp(opt, "ram");
 
 	si = &screen_info;
 	xres = si->lfb_width;
-- 
2.23.0

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Ard Biesheuvel]

(+ Andy)

On Wed, 11 Dec 2019 at 00:24, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On x86, until PAT is initialized, WC translates into UC-. Since we
> calculate and store pgprot_writecombine(PAGE_KERNEL) when earlycon is
> initialized, this means we actually use UC- mappings instead of WC
> mappings, which makes scrolling very slow.
>
> Instead store a boolean flag to indicate whether we want to use
> writeback or write-combine mappings, and recalculate the actual pgprot_t
> we need on every mapping. Once PAT is initialized, we will start using
> write-combine mappings, which speeds up the scrolling considerably.
>
> Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
> This applies on top of
>         efi/earlycon: Remap entire framebuffer after page initialization
>         https://git.kernel.org/tip/b418d660bb9798d2249ac6a46c844389ef50b6a5
>
>  drivers/firmware/efi/earlycon.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/efi/earlycon.c b/drivers/firmware/efi/earlycon.c
> index ee5a6431fb9c..18f6a61f3e17 100644
> --- a/drivers/firmware/efi/earlycon.c
> +++ b/drivers/firmware/efi/earlycon.c
> @@ -17,7 +17,7 @@ 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 bool fb_wb;
>  static void *efi_fb;
>
>  /*
> @@ -33,10 +33,8 @@ static int __init efi_earlycon_remap_fb(void)
>         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);
> +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
>
>         pr_info("Mapped earlycon framebuffer\n");
>
> @@ -58,9 +56,12 @@ late_initcall(efi_earlycon_unmap_fb);
>
>  static __ref void *efi_earlycon_map(unsigned long start, unsigned long len)
>  {
> +       pgprot_t fb_prot;
> +
>         if (efi_fb)
>                 return efi_fb + start;
>
> +       fb_prot = fb_wb ? PAGE_KERNEL : pgprot_writecombine(PAGE_KERNEL);
>         return early_memremap_prot(fb_base + start, len, pgprot_val(fb_prot));
>  }
>
> @@ -220,10 +221,7 @@ 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;
>
> -       if (opt && !strcmp(opt, "ram"))
> -               fb_prot = PAGE_KERNEL;
> -       else
> -               fb_prot = pgprot_writecombine(PAGE_KERNEL);
> +       fb_wb = opt && !strcmp(opt, "ram");
>
>         si = &screen_info;
>         xres = si->lfb_width;
> --
> 2.23.0
>

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Andy Shevchenko]

On Wed, Dec 11, 2019 at 08:22:56AM +0000, Ard Biesheuvel wrote:
> (+ Andy)
> 
> On Wed, 11 Dec 2019 at 00:24, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On x86, until PAT is initialized, WC translates into UC-. Since we
> > calculate and store pgprot_writecombine(PAGE_KERNEL) when earlycon is
> > initialized, this means we actually use UC- mappings instead of WC
> > mappings, which makes scrolling very slow.
> >
> > Instead store a boolean flag to indicate whether we want to use
> > writeback or write-combine mappings, and recalculate the actual pgprot_t
> > we need on every mapping. Once PAT is initialized, we will start using
> > write-combine mappings, which speeds up the scrolling considerably.
> >
> > Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Make sense.
One comment below.

> > -       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);
> > +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> > +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);

I would really like to keep the style with if-else.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Ard Biesheuvel]

On Wed, 11 Dec 2019 at 12:04, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 11, 2019 at 08:22:56AM +0000, Ard Biesheuvel wrote:
> > (+ Andy)
> >
> > On Wed, 11 Dec 2019 at 00:24, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On x86, until PAT is initialized, WC translates into UC-. Since we
> > > calculate and store pgprot_writecombine(PAGE_KERNEL) when earlycon is
> > > initialized, this means we actually use UC- mappings instead of WC
> > > mappings, which makes scrolling very slow.
> > >
> > > Instead store a boolean flag to indicate whether we want to use
> > > writeback or write-combine mappings, and recalculate the actual pgprot_t
> > > we need on every mapping. Once PAT is initialized, we will start using
> > > write-combine mappings, which speeds up the scrolling considerably.
> > >
> > > Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
> > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
>
> Make sense.
> One comment below.
>
> > > -       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);
> > > +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> > > +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
>
> I would really like to keep the style with if-else.
>

Can't we just set a global flag to MEMREMAP_WB or MEMREMAP_WC in the
init code, and use that directly?

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Arvind Sankar]

On Wed, Dec 11, 2019 at 03:00:46PM +0100, Ard Biesheuvel wrote:
> On Wed, 11 Dec 2019 at 12:04, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Dec 11, 2019 at 08:22:56AM +0000, Ard Biesheuvel wrote:
> > > (+ Andy)
> > >
> > > On Wed, 11 Dec 2019 at 00:24, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On x86, until PAT is initialized, WC translates into UC-. Since we
> > > > calculate and store pgprot_writecombine(PAGE_KERNEL) when earlycon is
> > > > initialized, this means we actually use UC- mappings instead of WC
> > > > mappings, which makes scrolling very slow.
> > > >
> > > > Instead store a boolean flag to indicate whether we want to use
> > > > writeback or write-combine mappings, and recalculate the actual pgprot_t
> > > > we need on every mapping. Once PAT is initialized, we will start using
> > > > write-combine mappings, which speeds up the scrolling considerably.
> > > >
> > > > Fixes: 69c1f396f25b ("efi/x86: Convert x86 EFI earlyprintk into generic earlycon implementation")
> > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> >
> > Make sense.
> > One comment below.
> >
> > > > -       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);
> > > > +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> > > > +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
> >
> > I would really like to keep the style with if-else.
> >
I could change this part back to the if/else. Are you ok with the
ternary operator in efi_earlycon_map?
> 
> Can't we just set a global flag to MEMREMAP_WB or MEMREMAP_WC in the
> init code, and use that directly?
We can't use it directly in efi_earlycon_map though, so we still need to
translate there, and we'd need an if/else in the init code, so this
would just move the if/else from here to the init code.

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Arvind Sankar]

On Wed, Dec 11, 2019 at 01:04:35PM +0200, Andy Shevchenko wrote:
> 
> Make sense.
> One comment below.
> 
> > > -       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);
> > > +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> > > +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
> 
> I would really like to keep the style with if-else.
> 
I edited this back to the if/else and then realized why I chose the
ternary. It makes it easier for the reader to see that the only thing
that depends on fb_wb is the MEMREMAP_ flag that gets used, while with
the if/else the reader needs to compare both function invocations to see
that that's the only difference.

It's not a big deal, so if you still prefer the if/else I'll revise the
patch.

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Andy Shevchenko]

On Wed, Dec 11, 2019 at 12:37:46PM -0500, Arvind Sankar wrote:
> On Wed, Dec 11, 2019 at 01:04:35PM +0200, Andy Shevchenko wrote:
> > 
> > Make sense.
> > One comment below.
> > 
> > > > -       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);
> > > > +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> > > > +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
> > 
> > I would really like to keep the style with if-else.
> > 
> I edited this back to the if/else and then realized why I chose the
> ternary. It makes it easier for the reader to see that the only thing
> that depends on fb_wb is the MEMREMAP_ flag that gets used, while with
> the if/else the reader needs to compare both function invocations to see
> that that's the only difference.
> 
> It's not a big deal, so if you still prefer the if/else I'll revise the
> patch.

Perhaps comment near to if can explain this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Ard Biesheuvel]

On Wed, 11 Dec 2019 at 19:03, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 11, 2019 at 12:37:46PM -0500, Arvind Sankar wrote:
> > On Wed, Dec 11, 2019 at 01:04:35PM +0200, Andy Shevchenko wrote:
> > >
> > > Make sense.
> > > One comment below.
> > >
> > > > > -       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);
> > > > > +       efi_fb = memremap(fb_base, screen_info.lfb_size,
> > > > > +                         fb_wb ? MEMREMAP_WB : MEMREMAP_WC);
> > >
> > > I would really like to keep the style with if-else.
> > >
> > I edited this back to the if/else and then realized why I chose the
> > ternary. It makes it easier for the reader to see that the only thing
> > that depends on fb_wb is the MEMREMAP_ flag that gets used, while with
> > the if/else the reader needs to compare both function invocations to see
> > that that's the only difference.
> >
> > It's not a big deal, so if you still prefer the if/else I'll revise the
> > patch.
>
> Perhaps comment near to if can explain this.
>

I'm fine with the ternary, actually. What do you feel is wrong with it?

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Andy Shevchenko]

On Wed, Dec 11, 2019 at 06:06:28PM +0000, Ard Biesheuvel wrote:
> On Wed, 11 Dec 2019 at 19:03, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Dec 11, 2019 at 12:37:46PM -0500, Arvind Sankar wrote:

> > Perhaps comment near to if can explain this.

> I'm fine with the ternary, actually. What do you feel is wrong with it?

I don't like ternary when it takes more than one line. I found them hard to
follow.

It's anyway style, so, go ahead with it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Arvind Sankar]

On Wed, Dec 11, 2019 at 10:00:07PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 11, 2019 at 06:06:28PM +0000, Ard Biesheuvel wrote:
> > On Wed, 11 Dec 2019 at 19:03, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Dec 11, 2019 at 12:37:46PM -0500, Arvind Sankar wrote:
> 
> > > Perhaps comment near to if can explain this.
> 
> > I'm fine with the ternary, actually. What do you feel is wrong with it?
> 
> I don't like ternary when it takes more than one line. I found them hard to
> follow.
> 
> It's anyway style, so, go ahead with it.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Thanks. 
> > Can't we just set a global flag to MEMREMAP_WB or MEMREMAP_WC in the
> > init code, and use that directly?
> We can't use it directly in efi_earlycon_map though, so we still need to
> translate there, and we'd need an if/else in the init code, so this
> would just move the if/else from here to the init code.

Ard, re your comment, does that cover it and the patch is ok as it
stands then?

Re: [PATCH] efi/earlycon: Fix write-combine mapping on x86

[Ard Biesheuvel]

On Thu, 12 Dec 2019 at 17:56, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Dec 11, 2019 at 10:00:07PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 11, 2019 at 06:06:28PM +0000, Ard Biesheuvel wrote:
> > > On Wed, 11 Dec 2019 at 19:03, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Dec 11, 2019 at 12:37:46PM -0500, Arvind Sankar wrote:
> >
> > > > Perhaps comment near to if can explain this.
> >
> > > I'm fine with the ternary, actually. What do you feel is wrong with it?
> >
> > I don't like ternary when it takes more than one line. I found them hard to
> > follow.
> >
> > It's anyway style, so, go ahead with it.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> Thanks.
> > > Can't we just set a global flag to MEMREMAP_WB or MEMREMAP_WC in the
> > > init code, and use that directly?
> > We can't use it directly in efi_earlycon_map though, so we still need to
> > translate there, and we'd need an if/else in the init code, so this
> > would just move the if/else from here to the init code.
>
> Ard, re your comment, does that cover it and the patch is ok as it
> stands then?

Yes.

I have queued it up and will send it out shortly.