Re: [PATCH 1/1] efi_loader: set correct frame buffer address
On 5/2/26 15:04, Simon Glass wrote:
Hi Heinrich, Mark,
On Sat, 2 May 2026 at 04:58, Mark Kettenis wrote:
Date: Sat, 2 May 2026 09:28:56 +0200
From: Heinrich Schuchardt
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
---
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?
Yes, that is what we can do together with PixelBltOnly.
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
The EFI spec does not specify how you may detect or access the
framebuffer after Exit boot services.
Operating systems have their own graphics drivers. Once they are loaded
you should get output.
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 use the shadow frame
Re: [PATCH 1/1] efi_loader: set correct frame buffer address
Hi Heinrich, Mark,
On Sat, 2 May 2026 at 04:58, Mark Kettenis wrote:
>
> > Date: Sat, 2 May 2026 09:28:56 +0200
> > From: Heinrich Schuchardt
>
> 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
> > > ---
> > > 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 framebuf
Re: [PATCH 1/1] efi_loader: set correct frame buffer address
> Date: Sat, 2 May 2026 09:28:56 +0200
> From: Heinrich Schuchardt
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
> > ---
> > 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;
>
> 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.
> 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?
> 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;
>
>
Re: [PATCH 1/1] efi_loader: set correct frame buffer address
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
---
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;
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.
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.
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.
So if plat->copy_base is set we should pass PixelBltOnly as PixelFormat
and copy_base as FrameBufferBase.
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;
