Re: [PATCH] efi: Downgrade "EFI_MEMMAP is not enabled" message

[Ard Biesheuvel]

On Fri, 1 Mar 2019 at 15:14, Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Fri, 01 Mar 2019 15:02:23 +0100,
> Ard Biesheuvel wrote:
> >
> > On Fri, 1 Mar 2019 at 15:01, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > >
> > > On Fri, 01 Mar 2019 14:53:39 +0100,
> > > Ard Biesheuvel wrote:
> > > >
> > > > On Fri, 1 Mar 2019 at 14:40, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > > > >
> > > > > Since 38ac0287b7f4 ("fbdev/efifb: Honour UEFI memory map attributes
> > > > > when mapping the FB"), efifb_probe() checks its memory range via
> > > > > efi_mem_desc_lookup(), and this leads to a spurious error message
> > > > > "EFI_MEMMAP is not enabled" at every boot on KVM.  This is quite
> > > > > annoying since the error message appears even if you set "quiet" boot
> > > > > option.
> > > > >
> > > > > Actually there are only a few places that call efi_mem_desc_lookup()
> > > > > function, and the other callers do give the explicit error messages
> > > > > when the function returns an error in anyway.  That is, the error
> > > > > message in the function is more or less moot.
> > > > >
> > > > > So let's downgrade the error message for stop annoying users.
> > > > >
> > > > > Fixes: 38ac0287b7f4 ("fbdev/efifb: Honour UEFI memory map attributes when mapping the FB")
> > > > > Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1127339
> > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > > > > ---
> > > > >  drivers/firmware/efi/efi.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > index 55b77c576c42..50ac33097458 100644
> > > > > --- a/drivers/firmware/efi/efi.c
> > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > @@ -409,7 +409,7 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > >         efi_memory_desc_t *md;
> > > > >
> > > > >         if (!efi_enabled(EFI_MEMMAP)) {
> > > > > -               pr_err_once("EFI_MEMMAP is not enabled.\n");
> > > > > +               pr_debug("EFI_MEMMAP is not enabled.\n");
> > > > >                 return -EINVAL;
> > > > >         }
> > > > >
> > > >
> > > > efifb_probe() only calls efi_mem_desc_lookup() if
> > > > screen_info.orig_video_isVGA == VIDEO_TYPE_EFI, which only gets
> > > > assigned on a EFI boot.
> > > >
> > > > So even though I don't object to the patch as is, I would like to
> > > > understand where this error message is coming from, given that it
> > > > means that you are running on a UEFI system without the EFI memory
> > > > map.
> > > >
> > > > Is this system booting via GRUB in EFI mode?
> > >
> > > No, it's booted in legacy boot mode.  But the primary fb is efifb, and
> > > that's why the message appears.
> > >
> >
> > So how are we ending up with
> >
> > screen_info.orig_video_isVGA == VIDEO_TYPE_EFI
> >
> > ??
>
> Ah, sorry, my description was too ambiguous.
>
> Actually our GRUB2 default setup boots the Linux kernel with linuxefi.
> What I meant was that I invoked qemu-kvm without any -bios option, so
> it's no EFI BIOS.
>

Some from the link here

https://openqa.opensuse.org/tests/864184/file/journal_check-full_journal.log

I got

Feb 27 13:13:41 linux-e2c3 kernel: efifb: probing for efifb
Feb 27 13:13:41 linux-e2c3 kernel: efi: EFI_MEMMAP is not enabled.
Feb 27 13:13:41 linux-e2c3 kernel: fbcon: Taking over console
Feb 27 13:13:41 linux-e2c3 kernel: efifb: No BGRT, not showing boot graphics
Feb 27 13:13:41 linux-e2c3 kernel: efifb: framebuffer at 0xfc000000,
using 1408k, total 1408k
Feb 27 13:13:41 linux-e2c3 kernel: efifb: mode is 800x600x24,
linelength=2400, pages=1
Feb 27 13:13:41 linux-e2c3 kernel: efifb: scrolling: redraw
Feb 27 13:13:41 linux-e2c3 kernel: efifb: Truecolor: size=0:8:8:8,
shift=0:16:8:0
Feb 27 13:13:41 linux-e2c3 kernel: Console: switching to colour frame
buffer device 100x37
Feb 27 13:13:41 linux-e2c3 kernel: fb0: EFI VGA frame buffer device

So if we are doing legacy, there is something else that is taking,
GRUB perhaps, that is taking the framebuffer parameters and putting
them in screen_info and marking them as VIDEO_TYPE_EFI.

If this is a reasonable thing to do, it implies that the efifb driver
may run on otherwise non-EFI systems, and if this is the case, I'd
rather fix it like this:

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ba906876cc45..9e529cc2b4ff 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -464,7 +464,8 @@ static int efifb_probe(struct platform_device *dev)
        info->apertures->ranges[0].base = efifb_fix.smem_start;
        info->apertures->ranges[0].size = size_remap;

-       if (!efi_mem_desc_lookup(efifb_fix.smem_start, &md)) {
+       if (efi_enabled(EFI_BOOT) &&
+           !efi_mem_desc_lookup(efifb_fix.smem_start, &md)) {
                if ((efifb_fix.smem_start + efifb_fix.smem_len) >
                    (md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT))) {
                        pr_err("efifb: video memory @ 0x%lx spans
multiple EFI memory regions\n",

Could you please confirm whether this works around the issue as well?