[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] efifb: ignore frame buffer with physical address 0


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 18 Nov 2022 14:04:40 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=JDMxY7Qucnf0dQdZtGjTa9VTDvqXuaXH7mTj8w423DQ=; b=HoIiDrqjpPfjQoS55LldzMucCHQ4sAGxRAs23gowj3HyHfkORnddehjYguPEiGMkYUv4ripr7KjAZIo+Dk3gEv0MnnMalLM/5/imlMaHCySK1m41m0dORinh02vgkGDVRbp/FdTe/8LEIGLeipOSEzpXJKobEFmyiqWCRWTs2uVPA8NSGqGxMRF0/TmPZba6oiUEQ3tF5haaSwGg6Bw3sTzGqQc6MXDraqG0qgQBvXKPRy5qtsLPg8+vxgUmm2E1oPXVhTlylymR8jITWudftcsvvDvdLA+zF2Cxac3ap7iF9mecSRxha6FJFubsZit5Ker0GYTs1VSbI8hqi2FbdQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HawpZIVSqHL36vPume5E1DjIWvXsc5hJxoFE+au7uUmtc0+B/GC5FIqnfn4sx8U65xyCoTNFDYCQLVKOQx0Z6Bg+oVO7Jinlao34oVgOvdaRWUL+DmWpVsRIp1kHJ1LE0yK04ji7wnsaQAnPPt6SEMFdbzpyKWu3mImyttVC5wfpXYdPNyYSnckzGhAQI0klloCmSYPlBLAjpLyF/D8m7hWhGLyr0zDvZEOlEfaOTc3KB1syBMKThjsG7Q0K+t/M+mZIQ5I9Pb8kukg9ufko+vrNuWAUux3Gdc/QoaMvlMqfLdyzoYwWF9Ss19z43eIhbG4KLZfHufgPIldradzJMg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Fri, 18 Nov 2022 13:04:53 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.