Re: [PATCH] fbdev/efifb: ignore framebuffer memmap entries that lack any memory types

[James Hilliard <james.hilliard1@gmail.com>]

On Tue, Apr 23, 2019 at 5:38 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 23 Apr 2019 at 17:01, James Hilliard <james.hilliard1@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 4:44 PM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > Commit 38ac0287b7f4
> > >
> > >   ("fbdev/efifb: Honour UEFI memory map attributes when mapping the FB")
> > >
> > > updated the EFI framebuffer code to use memory mappings for the linear
> > > framebuffer that are permitted by the memory attributes described by the
> > > EFI memory map for the particular region, if the framebuffer happens to
> > > be covered by the EFI memory map (which is typically only the case for
> > > framebuffers in shared memory). This is required since non-X86 systems
> > > may require cacheable attributes for memory mappings that are shared
> > > with other masters (such as GPUs), and this information cannot be
> > > described by the Graphics Output Protocol (GOP) EFI protocol itself,
> > > and so we rely on the EFI memory map for this.
> > >
> > > As reported by James, this breaks some x86 systems:
> > >
> > >   [ 1.173368] efifb: probing for efifb
> > >   [ 1.173386] efifb: abort, cannot remap video memory 0x1d5000 @ 0xcf800000
> > >   [ 1.173395] Trying to free nonexistent resource <00000000cf800000-00000000cf9d4bff>
> > >   [ 1.173413] efi-framebuffer: probe of efi-framebuffer.0 failed with error -5
> > >
> > > The problem turns out to be that the memory map entry that describes the
> > > framebuffer has no memory attributes listed at all, and so we end up with
> > > a mem_flags value of 0x0.
> > >
> > > So work around this by ensuring that the memory map entry's attribute field
> > > has a sane value before using it to mask the set of usable attributes.
> > >
> > > Reported-by: James Hilliard <james.hilliard1@gmail.com>
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >
> > > James, could you give this a try please? Thanks.
> > I can confirm this fixes the regression on my system, thanks.
>
> Thanks James, I will add your tested-by.
>
> I forgot to include EFI_MEMORY_UC in the first mask, so I will respin
> and resend.
Is there an updated patch I should test?
>
> Ingo, Bartlomiej, may i suggest that we take this into tip directly as
> a fix? Thanks.
>
>
> > >
> > >  drivers/video/fbdev/efifb.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > > index ba906876cc45..1f9949f50900 100644
> > > --- a/drivers/video/fbdev/efifb.c
> > > +++ b/drivers/video/fbdev/efifb.c
> > > @@ -476,8 +476,11 @@ static int efifb_probe(struct platform_device *dev)
> > >                  * If the UEFI memory map covers the efifb region, we may only
> > >                  * remap it using the attributes the memory map prescribes.
> > >                  */
> > > -               mem_flags |= EFI_MEMORY_WT | EFI_MEMORY_WB;
> > > -               mem_flags &= md.attribute;
> > > +               md.attribute &= EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB;
> > > +               if (md.attribute) {
> > > +                       mem_flags |= EFI_MEMORY_WT | EFI_MEMORY_WB;
> > > +                       mem_flags &= md.attribute;
> > > +               }
> > >         }
> > >         if (mem_flags & EFI_MEMORY_WC)
> > >                 info->screen_base = ioremap_wc(efifb_fix.smem_start,
> > > --
> > > 2.20.1
> > >