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

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

[Ard Biesheuvel]

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.

 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

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

[James Hilliard]

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

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

[Ard Biesheuvel]

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.

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

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

[James Hilliard]

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

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

[Ard Biesheuvel]

On Thu, 9 May 2019 at 15:23, James Hilliard <james.hilliard1@gmail.com> wrote:
>
> 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?

No, apologies for the delay - I have updated the patch and will send
it upstream as a fix once I have confirmed that it doesn't cause any
regressions.