| Message ID | 20221118123925.25363-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efifb: ignore frame buffer with physical address 0 | expand |
On 18.11.2022 13:39, Roger Pau Monne wrote: > On one of my boxes when the HDMI cable is not plugged in the > FrameBufferBase of the EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE structure is > set to 0 by the firmware (while some of the other fields looking > plausible). > > Such (bogus address) ends up mapped in vesa_init(), and since it > overlaps with a RAM region the whole system goes down pretty badly, > see: > > (XEN) vesafb: framebuffer at 0x0000000000000000, mapped to 0xffff82c000201000, using 35209k, total 35209k > (XEN) vesafb: mode is 0x37557x32, linelength=960, font 8x16 Interesting mode - should we check for non-zero values there as well, perhaps? > (XEN) vesafb: Truecolor: size=8:8:8:8, shift=24:0:8:16 > (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) �ERROR: Class:0; Subclass:0; Operation: 0 > ERROR: No ConOut > ERROR: No ConIn > > Do like Linux and prevent using the EFI Frame Buffer if the base > address is 0. This is inline with the logic in Linuxes > fb_base_is_valid() function at drivers/video/fbdev/efifb.c v6.0.9. > > See also Linux commit 133bb070e94ab41d750c6f2160c8843e46f11b78 for > further reference. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Other options would be doing the check in vesa_init(), but that would > also then apply to other framebuffers and won't be strictly limited to > the EFI fb. Well, zero is wrong uniformly, so it wouldn't seem unreasonable to put the check there. But I'm happy to keep it in EFI code for now. > We could also check in vesa_init() whether the framebuffer overlaps > with any RAM region, but I think that should be in addition to the > change done here. Indeed. > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, > bpp = 0; > break; > } > - if ( bpp > 0 ) > + if ( bpp > 0 && gop->Mode->FrameBufferBase ) > { > vga_console_info.video_type = XEN_VGATYPE_EFI_LFB; > vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */ A few lines up from here, just out of patch context, there is a PrintErr() which imo is bogus/misleading when also encountering a zero fb base. I'd like to suggest that you put the new check early in the function (perhaps extended by a zero check of other applicable fields, as per above), returning right away alongside another new PrintErr(). Jan
On Fri, Nov 18, 2022 at 02:04:40PM +0100, Jan Beulich wrote: > On 18.11.2022 13:39, Roger Pau Monne wrote: > > On one of my boxes when the HDMI cable is not plugged in the > > FrameBufferBase of the EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE structure is > > set to 0 by the firmware (while some of the other fields looking > > plausible). > > > > Such (bogus address) ends up mapped in vesa_init(), and since it > > overlaps with a RAM region the whole system goes down pretty badly, > > see: > > > > (XEN) vesafb: framebuffer at 0x0000000000000000, mapped to 0xffff82c000201000, using 35209k, total 35209k > > (XEN) vesafb: mode is 0x37557x32, linelength=960, font 8x16 > > Interesting mode - should we check for non-zero values there as well, > perhaps? We could, yes, I went for what Linux currently does, but a height or width of 0 is also likely wrong. We already check for bpp != 0. > > (XEN) vesafb: Truecolor: size=8:8:8:8, shift=24:0:8:16 > > (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) �ERROR: Class:0; Subclass:0; Operation: 0 > > ERROR: No ConOut > > ERROR: No ConIn > > > > Do like Linux and prevent using the EFI Frame Buffer if the base > > address is 0. This is inline with the logic in Linuxes > > fb_base_is_valid() function at drivers/video/fbdev/efifb.c v6.0.9. > > > > See also Linux commit 133bb070e94ab41d750c6f2160c8843e46f11b78 for > > further reference. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Other options would be doing the check in vesa_init(), but that would > > also then apply to other framebuffers and won't be strictly limited to > > the EFI fb. > > Well, zero is wrong uniformly, so it wouldn't seem unreasonable to > put the check there. But I'm happy to keep it in EFI code for now. > > > We could also check in vesa_init() whether the framebuffer overlaps > > with any RAM region, but I think that should be in addition to the > > change done here. > > Indeed. > > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, > > bpp = 0; > > break; > > } > > - if ( bpp > 0 ) > > + if ( bpp > 0 && gop->Mode->FrameBufferBase ) > > { > > vga_console_info.video_type = XEN_VGATYPE_EFI_LFB; > > vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */ > > A few lines up from here, just out of patch context, there is a > PrintErr() which imo is bogus/misleading when also encountering a > zero fb base. I'd like to suggest that you put the new check early > in the function (perhaps extended by a zero check of other > applicable fields, as per above), returning right away alongside > another new PrintErr(). Would you be fine with the new message being "Invalid Frame Buffer configuration found"? Thanks, Roger.
On 18.11.2022 14:44, Roger Pau Monné wrote: > On Fri, Nov 18, 2022 at 02:04:40PM +0100, Jan Beulich wrote: >> On 18.11.2022 13:39, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/efi/efi-boot.h >>> +++ b/xen/arch/x86/efi/efi-boot.h >>> @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, >>> bpp = 0; >>> break; >>> } >>> - if ( bpp > 0 ) >>> + if ( bpp > 0 && gop->Mode->FrameBufferBase ) >>> { >>> vga_console_info.video_type = XEN_VGATYPE_EFI_LFB; >>> vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */ >> >> A few lines up from here, just out of patch context, there is a >> PrintErr() which imo is bogus/misleading when also encountering a >> zero fb base. I'd like to suggest that you put the new check early >> in the function (perhaps extended by a zero check of other >> applicable fields, as per above), returning right away alongside >> another new PrintErr(). > > Would you be fine with the new message being "Invalid Frame Buffer > configuration found"? Yeah, that'll probably do. Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index e82ac9daa7..a68091d82a 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -552,7 +552,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, bpp = 0; break; } - if ( bpp > 0 ) + if ( bpp > 0 && gop->Mode->FrameBufferBase ) { vga_console_info.video_type = XEN_VGATYPE_EFI_LFB; vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
On one of my boxes when the HDMI cable is not plugged in the FrameBufferBase of the EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE structure is set to 0 by the firmware (while some of the other fields looking plausible). Such (bogus address) ends up mapped in vesa_init(), and since it overlaps with a RAM region the whole system goes down pretty badly, see: (XEN) vesafb: framebuffer at 0x0000000000000000, mapped to 0xffff82c000201000, using 35209k, total 35209k (XEN) vesafb: mode is 0x37557x32, linelength=960, font 8x16 (XEN) vesafb: Truecolor: size=8:8:8:8, shift=24:0:8:16 (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) (XEN) �ERROR: Class:0; Subclass:0; Operation: 0 ERROR: No ConOut ERROR: No ConIn Do like Linux and prevent using the EFI Frame Buffer if the base address is 0. This is inline with the logic in Linuxes fb_base_is_valid() function at drivers/video/fbdev/efifb.c v6.0.9. See also Linux commit 133bb070e94ab41d750c6f2160c8843e46f11b78 for further reference. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Other options would be doing the check in vesa_init(), but that would also then apply to other framebuffers and won't be strictly limited to the EFI fb. We could also check in vesa_init() whether the framebuffer overlaps with any RAM region, but I think that should be in addition to the change done here. --- xen/arch/x86/efi/efi-boot.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)