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

[Arvind Sankar <nivedita@alum.mit.edu>]

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.