efifb: avoid reconfiguration of BAR that covers the framebuffer
Subsystems (3)
PATCH 1/1 efifb: avoid reconfiguration of BAR that covers the framebuffer
On UEFI systems, the PCI subsystem is enumerated by the firmware, and if a graphical framebuffer is exposed by a PCI device, its base address and size are exposed to the OS via the Graphics Output Protocol (GOP).
On arm64 PCI systems, the entire PCI hierarchy is reconfigured from scratch at boot. This may result in the GOP framebuffer address to become stale, if the BAR covering the framebuffer is modified. This will cause the framebuffer to become unresponsive, and may in some cases result in unpredictable behavior if the range is reassigned to another device.
So add a quirk to the EFI fb driver to find the BAR associated with the GOP base address, and set the IORESOURCE_PCI_FIXED attribute so that the PCI core will leave it alone.
--- drivers/video/fbdev/efifb.c | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+)
drivers/video/fbdev/efifb.c +33 -0
@@ -10,6 +10,7 @@#include <linux/efi.h> #include <linux/errno.h> #include <linux/fb.h> +#include <linux/pci.h> #include <linux/platform_device.h> #include <linux/screen_info.h> #include <video/vga.h>
@@ -360,3 +361,35 @@ static struct platform_driver efifb_driver = { };builtin_platform_driver (efifb_driver); + +static bool resource_found; + +static void efifb_fixup_resources (struct pci_dev *dev) +{ +u64 fb_base = screen_info.lfb_base; +u64 fb_size = screen_info.lfb_size; +int i; + +if (resource_found || screen_info.orig_video_isVGA !=VIDEO_TYPE_EFI ) +return ; + +if (screen_info.capabilities &VIDEO_CAPABILITY_64BIT_BASE ) + fb_base |= (u64 )screen_info.ext_lfb_base <<32 ; + +if (!fb_base) +return ; + +for (i =0 ; i <PCI_STD_RESOURCE_END ; i++) { +struct resource *res = &dev->resource[i]; + +if (!(res->flags &IORESOURCE_MEM )) +continue ; + +if (res->start <= fb_base && res->end >= fb_base + fb_size) { + res->flags |=IORESOURCE_PCI_FIXED ; + resource_found = true; +break ; + } + } +} +DECLARE_PCI_FIXUP_HEADER (PCI_ANY_ID ,PCI_ANY_ID , efifb_fixup_resources);
Signature · 1 line
2.7.4
PATCH 1/1 efifb: avoid reconfiguration of BAR that covers the framebuffer
[+Bjorn]
Quoted 11 lines · Ard Biesheuvel
On UEFI systems, the PCI subsystem is enumerated by the firmware, and if a graphical framebuffer is exposed by a PCI device, its base address and size are exposed to the OS via the Graphics Output Protocol (GOP).
On arm64 PCI systems, the entire PCI hierarchy is reconfigured from scratch at boot. This may result in the GOP framebuffer address to become stale, if the BAR covering the framebuffer is modified. This will cause the framebuffer to become unresponsive, and may in some cases result in unpredictable behavior if the range is reassigned to another device.
How does it currently work on eg x86 ? I suspect it just works because on x86 resources are claimed at boot and if the FB memory BAR contains reasonable values it is left alone by the kernel code reassigning resources on x86.
Quoted 40 lines
So add a quirk to the EFI fb driver to find the BAR associated with the GOP base address, and set the
IORESOURCE_PCI_FIXEDattribute so that the PCI core will leave it alone.Signed-off-by Ard Biesheuvel <[email protected]>--- drivers/video/fbdev/efifb.c | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+)drivers/video/fbdev/efifb.c +16
-0@@ -10,6 +10,7 @@#include <linux/efi.h> #include <linux/errno.h> #include <linux/fb.h> +#include <linux/pci.h> #include <linux/platform_device.h> #include <linux/screen_info.h> #include <video/vga.h> @@ -360,3 +361,35 @@ static struct platform_driver efifb_driver = { };builtin_platform_driver (efifb_driver); + +static bool resource_found; + +static void efifb_fixup_resources (struct pci_dev *dev) +{ +u64 fb_base = screen_info.lfb_base; +u64 fb_size = screen_info.lfb_size; +int i; + +if (resource_found || screen_info.orig_video_isVGA !=VIDEO_TYPE_EFI ) +return ; + +if (screen_info.capabilities &VIDEO_CAPABILITY_64BIT_BASE ) + fb_base |= (u64 )screen_info.ext_lfb_base <<32 ; +
drivers/video/fbdev/efifb.c static struct platform_driver efifb_driver = { · line 388
Quoted 10 lines
drivers/video/fbdev/efifb.c +10
-0+if (!fb_base) +return ; + +for (i =0 ; i <PCI_STD_RESOURCE_END ; i++) { +struct resource *res = &dev->resource[i]; + +if (!(res->flags &IORESOURCE_MEM )) +continue ; + +if (res->start <= fb_base && res->end >= fb_base + fb_size) {
You are checking for a live resource here right (ie PCI device should be enabled) ? I am not sure that just checking the resource range is safe (I mean it would be most certainly a FW bug to have a PCI disabled device with memory BAR programmed with the FB addresses but thought it was worth mentioning).
Lorenzo
Quoted 10 lines
drivers/video/fbdev/efifb.c +7
-0+ res->flags |=IORESOURCE_PCI_FIXED ; + resource_found = true; +break ; + } + } +} +DECLARE_PCI_FIXUP_HEADER (PCI_ANY_ID ,PCI_ANY_ID , efifb_fixup_resources);Signature · 1 line
2.7.4
PATCH 1/1 efifb: avoid reconfiguration of BAR that covers the framebuffer
Quoted 15 lines · Lorenzo Pieralisi
[+Bjorn]
Quoted 11 lines · Ard Biesheuvel
On UEFI systems, the PCI subsystem is enumerated by the firmware, and if a graphical framebuffer is exposed by a PCI device, its base address and size are exposed to the OS via the Graphics Output Protocol (GOP).
On arm64 PCI systems, the entire PCI hierarchy is reconfigured from scratch at boot. This may result in the GOP framebuffer address to become stale, if the BAR covering the framebuffer is modified. This will cause the framebuffer to become unresponsive, and may in some cases result in unpredictable behavior if the range is reassigned to another device.
Quoted 5 lines
How does it currently work on eg x86 ? I suspect it just works because on x86 resources are claimed at boot and if the FB memory BAR contains reasonable values it is left alone by the kernel code reassigning resources on x86.
This is my understanding, yes.
Quoted 48 lines
Quoted 48 lines
So add a quirk to the EFI fb driver to find the BAR associated with the GOP base address, and set the
IORESOURCE_PCI_FIXEDattribute so that the PCI core will leave it alone.Signed-off-by Ard Biesheuvel <[email protected]>--- drivers/video/fbdev/efifb.c | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+)drivers/video/fbdev/efifb.c +24
-0@@ -10,6 +10,7 @@#include <linux/efi.h> #include <linux/errno.h> #include <linux/fb.h> +#include <linux/pci.h> #include <linux/platform_device.h> #include <linux/screen_info.h> #include <video/vga.h> @@ -360,3 +361,35 @@ static struct platform_driver efifb_driver = { };builtin_platform_driver (efifb_driver); + +static bool resource_found; + +static void efifb_fixup_resources (struct pci_dev *dev) +{ +u64 fb_base = screen_info.lfb_base; +u64 fb_size = screen_info.lfb_size; +int i; + +if (resource_found || screen_info.orig_video_isVGA !=VIDEO_TYPE_EFI ) +return ; + +if (screen_info.capabilities &VIDEO_CAPABILITY_64BIT_BASE ) + fb_base |= (u64 )screen_info.ext_lfb_base <<32 ; + +if (!fb_base) +return ; + +for (i =0 ; i <PCI_STD_RESOURCE_END ; i++) { +struct resource *res = &dev->resource[i]; + +if (!(res->flags &IORESOURCE_MEM )) +continue ;
Quoted 9 lines
drivers/video/fbdev/efifb.c static struct platform_driver efifb_driver = { · line 388
Quoted 2 lines
drivers/video/fbdev/efifb.c +2
-0+ +if (res->start <= fb_base && res->end >= fb_base + fb_size) {You are checking for a live resource here right (ie PCI device should be enabled) ? I am not sure that just checking the resource range is safe (I mean it would be most certainly a FW bug to have a PCI disabled device with memory BAR programmed with the FB addresses but thought it was worth mentioning).
It is implied that the device is enabled. The GOP protocol exposes a live framebuffer base/size with some metadata regarding the pixel format/color depth etc. It contains no annotations as to whether the device is PCI or simply a framebuffer mapped in system memory, and so we can only assume that the device is enabled.
Quoted 10 lines
Quoted 10 lines
drivers/video/fbdev/efifb.c +7
-1+ res->flags |=IORESOURCE_PCI_FIXED ; + resource_found = true; +break ; + } + } +} +DECLARE_PCI_FIXUP_HEADER (PCI_ANY_ID ,PCI_ANY_ID , efifb_fixup_resources); --2.7.4
PATCH 1/1 efifb: avoid reconfiguration of BAR that covers the framebuffer
On Tue, Mar 21, 2017 at 11:59:16AM +0000, Ard Biesheuvel wrote:
[...]
Quoted 30 lines
Quoted 20 lines
Quoted 20 lines
drivers/video/fbdev/efifb.c +20
-0+static void efifb_fixup_resources (struct pci_dev *dev) +{ +u64 fb_base = screen_info.lfb_base; +u64 fb_size = screen_info.lfb_size; +int i; + +if (resource_found || screen_info.orig_video_isVGA !=VIDEO_TYPE_EFI ) +return ; + +if (screen_info.capabilities &VIDEO_CAPABILITY_64BIT_BASE ) + fb_base |= (u64 )screen_info.ext_lfb_base <<32 ; + +if (!fb_base) +return ; + +for (i =0 ; i <PCI_STD_RESOURCE_END ; i++) { +struct resource *res = &dev->resource[i]; + +if (!(res->flags &IORESOURCE_MEM )) +continue ;Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + if (res->start <= fb_base && res->end >= fb_base + fb_size) {You are checking for a live resource here right (ie PCI device should be enabled) ? I am not sure that just checking the resource range is safe (I mean it would be most certainly a FW bug to have a PCI disabled device with memory BAR programmed with the FB addresses but thought it was worth mentioning).
Quoted 5 lines
It is implied that the device is enabled. The GOP protocol exposes a live framebuffer base/size with some metadata regarding the pixel format/color depth etc. It contains no annotations as to whether the device is PCI or simply a framebuffer mapped in system memory, and so we can only assume that the device is enabled.
That's why I think you should actually check that the device memory cycles are enabled to prevent setting a fixed resource for the wrong PCI device (we could argue that would be a FW bug - better play it safe though).
Thanks,
Lorenzo