Re: [PATCH v2 2/2] efi: x86: convert x86 EFI earlyprintk into generic earlycon implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 29 Jan 2019 at 15:04, Alexander Graf <agraf@xxxxxxx> wrote:
>
> On 01/29/2019 02:41 PM, Ard Biesheuvel wrote:
> > Hi Alex,
> >
> > On Tue, 29 Jan 2019 at 14:37, Alexander Graf <agraf@xxxxxxx> wrote:
> >> On 01/29/2019 10:21 AM, Ard Biesheuvel wrote:
> >>> Move the x86 EFI earlyprintk implementation to a shared location under
> >>> drivers/firmware and tweak it slightly so we can expose it as an earlycon
> >>> implementation (which is generic) rather than earlyprintk (which is only
> >>> implemented for a few architectures)
> >>>
> >>> This also involves switching to write-combine mappings by default (which
> >>> is required on ARM since device mappings lack memory semantics, and so
> >>> memcpy/memset may not be used on them), and adding support for shared
> >>> memory framebuffers on cache coherent non-x86 systems (which do not
> >>> tolerate mismatched attributes)
> >>>
> >>> Note that 32-bit ARM does not populate its struct screen_info early
> >>> enough for earlycon=efifb to work, so it is disabled there.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> >>> ---
> >>>    Documentation/admin-guide/kernel-parameters.txt |   8 +-
> >>>    arch/x86/Kconfig.debug                          |  10 -
> >>>    arch/x86/include/asm/efi.h                      |   1 -
> >>>    arch/x86/kernel/early_printk.c                  |   4 -
> >>>    arch/x86/platform/efi/Makefile                  |   1 -
> >>>    arch/x86/platform/efi/early_printk.c            | 240 --------------------
> >>>    drivers/firmware/efi/Kconfig                    |   6 +
> >>>    drivers/firmware/efi/Makefile                   |   1 +
> >>>    drivers/firmware/efi/earlycon.c                 | 208 +++++++++++++++++
> >>>    9 files changed, 222 insertions(+), 257 deletions(-)
> >>>
> >> [...]
> >>
> >>> +static int __init efi_earlycon_setup(struct earlycon_device *device,
> >>> +                                  const char *opt)
> >>> +{
> >>> +     struct screen_info *si;
> >>> +     u16 xres, yres;
> >>> +     u32 i;
> >>> +
> >>> +     if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> >>> +             return -ENODEV;
> >>> +
> >>> +     fb_base = screen_info.lfb_base;
> >>> +     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);
> >> Can you determine the default from the UEFI memory map?
> >>
> > No. This is being called way before we parse the system table and the
> > memory map. Given that this is debug code, duplicating a significant
> > chunk of that work here (and run the risk of crashing here due to
> > unexpected contents in those tables) is not a great idea imo.
>
> I see. Maybe we will want to have something there, but I tend to agree
> that for now we should keep bits as simple as possible.
>
> >
> >> Also, doesn't the current logic map it as WC on x86 too? Is that
> >> intentional?
> >>
> > Yes. As mentioned in the cover letter, this aligns it with efifb which
> > also uses WC by default (although there, it can be overridden for
> > performance reasons, but due to the debug nature of earlycon, this
> > doesn't matter, since higher performance only makes it more difficult
> > to capture the log on your phone camera)
>
> Well, the cover letter really only talks about arm :). But yeah, I think
> it's probably a good idea to map it WC regardless.
>

Fair enough.

> Overall, I would've preferred to have a larger patch set with more, but
> smaller changes that refactor the code. But it seems to be reviewable
> enough still. Let's cross our fingers it doesn't break :).
>
>
> Reviewed-by: Alexander Graf <agraf@xxxxxxx>
>

Thanks



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux