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 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.

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,

Alex




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux