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

[Alexander Graf <agraf@suse.de>]

On 01/29/2019 02:41 PM, Ard Biesheuvel wrote:
> Hi Alex,
>
> On Tue, 29 Jan 2019 at 14:37, Alexander Graf <agraf@suse.de> 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@linaro.org>
>>> ---
>>>    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@suse.de>

Thanks,

Alex