Hi Heinrich, On Tue, 21 Jul 2026 at 02:03, Heinrich Schuchardt <[email protected]> wrote: > > On 5/2/26 15:04, Simon Glass wrote: > > Hi Heinrich, Mark, > > > > On Sat, 2 May 2026 at 04:58, Mark Kettenis <[email protected]> wrote: > >> > >>> Date: Sat, 2 May 2026 09:28:56 +0200 > >>> From: Heinrich Schuchardt <[email protected]> > >> > >> 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 <[email protected]> > >>>> --- > >>>> 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. > > (resending as the the first mail had the old u-boot list mail address). > > Do you have a real x86 laptop using U-Boot?
Yes...but I haven't got around to trying this patch. I also have an x86 QEMU test so if this breaks something I will spot it eventually. It looks right to me. Regards, Simon
