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

[Ard Biesheuvel]

On Tue, 26 Mar 2019 at 16:25, Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Fri, 01 Mar 2019 16:27:24 +0100,
> Takashi Iwai wrote:
> >
> > On Fri, 01 Mar 2019 15:57:03 +0100,
> > Ard Biesheuvel wrote:
> > >
> > > 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?
> >
> > Yeah, that makes sense, and I confirmed that your patch worked.
> > Both EFI_BOOT and EFI_MEMMAP are 0 on the default boot.
> >
> > Reported-and-tested-by: Takashi Iwai <tiwai@xxxxxxx>
>
> Ard, what is the status of this fix?
>

I will get it queued for v5.2