[PATCH 1/1] efi_loader: set correct frame buffer address

[Simon Glass]

Hi Heinrich, Mark,

On Sat, 2 May 2026 at 04:58, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > Date: Sat, 2 May 2026 09:28:56 +0200
> > From: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>
> Hi Heinrich,
>
> > On 5/1/26 17:05, Heinrich Schuchardt wrote:
> > > If we use video copy, bit image transfers need to write to the in memory
> > > copy of the physical frame buffer. Damage control will sync the changes
> > > to the physical frame buffer.
> > >
> > > Cyclic video copy will catch all changes done by EFI applications directly
> > > accessing the frame buffer copy.
> > >
> > > gopobj->mode.fb_base must be a valid pointer to memory and not a virtual
> > > sandbox address.
> > >
> > > With this change the block image transfer test works again on the sandbox.
> > >
> > >      setenv efi_selftest block image transfer
> > >      bootefi selftest
> > >
> > > Fixes: a75cf70d23ac ("efi: Correct handling of frame buffer")
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > > ---
> > >   lib/efi_loader/efi_gop.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> > > index 9403e09691e..ae44d140289 100644
> > > --- a/lib/efi_loader/efi_gop.c
> > > +++ b/lib/efi_loader/efi_gop.c
> > > @@ -471,7 +471,7 @@ efi_status_t efi_gop_register(void)
> > >   {
> > >     struct efi_gop_obj *gopobj;
> > >     u32 bpix, format, col, row;
> > > -   u64 fb_base, fb_size;
> > > +   u64 fb_size;
> > >     efi_status_t ret;
> > >     struct udevice *vdev;
> > >     struct video_priv *priv;
> > > @@ -490,7 +490,6 @@ efi_status_t efi_gop_register(void)
> > >     row = video_get_ysize(vdev);
> > >
> > >     plat = dev_get_uclass_plat(vdev);
> > > -   fb_base = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base;
> > >     fb_size = plat->size;
> > >
> > >     switch (bpix) {
> > > @@ -528,7 +527,7 @@ efi_status_t efi_gop_register(void)
> > >     gopobj->mode.info = &gopobj->info;
> > >     gopobj->mode.info_size = sizeof(gopobj->info);
> > >
> > > -   gopobj->mode.fb_base = fb_base;
> > > +   gopobj->mode.fb_base = (uintptr_t)priv->fb;

Don't you want to use copy_fb (if VIDEO_COPY is enabled) so you get
the hardware address?

> >
> > I discussed the value of FrameBufferBase with Simon.
> >
> > The code before the patch is incorrect, as on the sandbox it does not
> > provide a pointer value but a sandbox virtual address. EFI applications
> > may use the pointer for directly writing to the frame buffer. This would
> > lead to a crash on the sandbox.
>
> I still think this means that EFI on sandbox is fundamentally broken.
> The identity virtual-to-physical mapping assumption is just too deeply
> engrained in the ecosystem since that is the documented handoff state
> for most architectures.

The ACPI tables have the same issue (needing to pass a pointer within
sandbox's emulated RAM buffer) and we use nomap_sysmem() to handle
that.

In what way is it broken?

>
> > For the case of CONFIG_VIDEO_COPY=y it is problematic to pass the memory
> > buffer address here, as Linux has an EFI framebuffer driver and would
> > write to the buffer instead of the physical framebuffer. But our memory
> > buffer is neither reserved memory not is it copied to the physical
> > framebuffer after ExitBootServices.
>
> Right.
>
> > If we pass the physical framebuffer address here, then an EFI
> > application might write to it and the cyclic video sync would overwrite
> > the changed pixels.
> >
> > Setting PixelBltOnly in field Mode.PixelFormat forbids EFI applications
> > to directly write to the framebuffer.
>
> That means the framebuffer becomes unusable as soon as the EFI
> application calls ExitBootServices() since an EFI application can't
> call Blt() after that
>
> > So if plat->copy_base is set we should pass PixelBltOnly as PixelFormat
> > and copy_base as FrameBufferBase.
>
> If PixelFormat is PixelBtlOnly, FrameBufferBase is meaningless and
> probably should be set to 0 instead.
>
> Anyway, as far as I can tell there aren't a lot of OSes that support
> PixelBltOnly.  OpenBSD certainly doesn't.  And as far as my reading of
> Linux's efistub code is correct, it doesn't support it either.  And neither does u-boot's own efi_video driver.
>
> I've always seen PixelBltOnly as a way to support weird hardware that
> didn't support a linear framebuffer with a sane pixel format.  Not as
> a way to support a shadow framebuffer like CONFIG_VIDEO_COPY
> implements.  Note that CONFIG_VIDEO_COPY itself assumes a linear
> framebuffer with a sane pixel format as it just does a straight copy
> of the pixels.
>
> Instead of using PixelBltOnly, can we just disable the shadow
> framebuffer as soon as we hand over control to an EFI application?  Or
> maybe at the point where the EFI application uses the GOP protocol?

This seems like a good idea to me - the latter would be best since it
won't impact apps which only use console output. The impact would be
that U-Boot will need to read from the hardware framebuffer with
write-combining enabled (slow on x86) but at least it is correct.

>
>
> > We the aforementioned change we will need to change function
> > gop_get_bpp() to not rely on PixelFormat. Instead we need to pass
> > priv->format in a private field of the graphics protocol.
> >
> >
> > >     gopobj->mode.fb_size = fb_size;
> > >
> > >     gopobj->info.version = 0;
> > > @@ -553,7 +552,7 @@ efi_status_t efi_gop_register(void)
> > >     }
> > >     gopobj->info.pixels_per_scanline = col;
> > >     gopobj->bpix = bpix;
> > > -   gopobj->fb = map_sysmem(fb_base, fb_size);
> > > +   gopobj->fb = priv->fb;
> >
> > priv->fb is also the value used by the TrueType drivers.
> >
> > Best regards
> >
> > Heinrich
> >
> > >     gopobj->vdev = vdev;
> > >
> > >     return EFI_SUCCESS;

It would be worth testing this on a real x86 laptop to make sure it
can still boot Ubuntu, etc.

Regards,
Simon