* [tip:efi/urgent] efi/fb: Avoid reconfiguration of BAR that covers the framebuffer
2017-04-04 15:27 ` [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer Ard Biesheuvel
@ 2017-04-05 7:58 ` tip-bot for Ard Biesheuvel
2017-04-05 10:30 ` tip-bot for Ard Biesheuvel
2017-05-19 16:27 ` [PATCH 2/2] efifb: " Bjorn Helgaas
2 siblings, 0 replies; 16+ messages in thread
From: tip-bot for Ard Biesheuvel @ 2017-04-05 7:58 UTC (permalink / raw)
To: linux-tip-commits
Cc: ard.biesheuvel, linux-kernel, torvalds, pjones, mingo, matt, tglx,
peterz, hpa
Commit-ID: 5a8997342183bb792fe2c15cecf371665d784dd7
Gitweb: http://git.kernel.org/tip/5a8997342183bb792fe2c15cecf371665d784dd7
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Tue, 4 Apr 2017 16:27:44 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 5 Apr 2017 09:20:18 +0200
efi/fb: 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 via 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 non-x86 quirk to the EFI fb driver to find the BAR associated
with the GOP base address, and claim the BAR resource so that the PCI
core will not move it.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: <stable@vger.kernel.org> # v4.7+
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Jones <pjones@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: leif.lindholm@linaro.org
Cc: linux-efi@vger.kernel.org
Cc: lorenzo.pieralisi@arm.com
Fixes: 9822504c1fa5 ("efifb: Enable the efi-framebuffer platform driver ...")
Link: http://lkml.kernel.org/r/20170404152744.26687-3-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
drivers/video/fbdev/efifb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 8c4dc1e..758960b 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -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>
@@ -143,6 +144,8 @@ static struct attribute *efifb_attrs[] = {
};
ATTRIBUTE_GROUPS(efifb);
+static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
+
static int efifb_probe(struct platform_device *dev)
{
struct fb_info *info;
@@ -152,7 +155,7 @@ static int efifb_probe(struct platform_device *dev)
unsigned int size_total;
char *option = NULL;
- if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
+ if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
return -ENODEV;
if (fb_get_options("efifb", &option))
@@ -360,3 +363,64 @@ static struct platform_driver efifb_driver = {
};
builtin_platform_driver(efifb_driver);
+
+#ifndef CONFIG_X86
+
+static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
+
+static void claim_efifb_bar(struct pci_dev *dev, int idx)
+{
+ u16 word;
+
+ pci_bar_found = true;
+
+ pci_read_config_word(dev, PCI_COMMAND, &word);
+ if (!(word & PCI_COMMAND_MEMORY)) {
+ pci_dev_disabled = true;
+ dev_err(&dev->dev,
+ "BAR %d: assigned to efifb but device is disabled!\n",
+ idx);
+ return;
+ }
+
+ if (pci_claim_resource(dev, idx)) {
+ pci_dev_disabled = true;
+ dev_err(&dev->dev,
+ "BAR %d: failed to claim resource for efifb!\n", idx);
+ return;
+ }
+
+ dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
+}
+
+static void efifb_fixup_resources(struct pci_dev *dev)
+{
+ u64 base = screen_info.lfb_base;
+ u64 size = screen_info.lfb_size;
+ int i;
+
+ if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
+ return;
+
+ if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+ base |= (u64)screen_info.ext_lfb_base << 32;
+
+ if (!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 <= base && res->end >= base + size - 1) {
+ claim_efifb_bar(dev, i);
+ break;
+ }
+ }
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY,
+ 16, efifb_fixup_resources);
+
+#endif
^ permalink raw reply related [flat|nested] 16+ messages in thread* [tip:efi/urgent] efi/fb: Avoid reconfiguration of BAR that covers the framebuffer
2017-04-04 15:27 ` [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer Ard Biesheuvel
2017-04-05 7:58 ` [tip:efi/urgent] efi/fb: " tip-bot for Ard Biesheuvel
@ 2017-04-05 10:30 ` tip-bot for Ard Biesheuvel
2017-05-19 16:27 ` [PATCH 2/2] efifb: " Bjorn Helgaas
2 siblings, 0 replies; 16+ messages in thread
From: tip-bot for Ard Biesheuvel @ 2017-04-05 10:30 UTC (permalink / raw)
To: linux-tip-commits
Cc: pjones, tglx, peterz, torvalds, matt, ard.biesheuvel,
linux-kernel, mingo, hpa
Commit-ID: 55d728a40d368ba80443be85c02e641fc9082a3f
Gitweb: http://git.kernel.org/tip/55d728a40d368ba80443be85c02e641fc9082a3f
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Tue, 4 Apr 2017 16:27:44 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 5 Apr 2017 12:25:53 +0200
efi/fb: 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 via 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 non-x86 quirk to the EFI fb driver to find the BAR associated
with the GOP base address, and claim the BAR resource so that the PCI
core will not move it.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: <stable@vger.kernel.org> # v4.7+
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Jones <pjones@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: leif.lindholm@linaro.org
Cc: linux-efi@vger.kernel.org
Cc: lorenzo.pieralisi@arm.com
Fixes: 9822504c1fa5 ("efifb: Enable the efi-framebuffer platform driver ...")
Link: http://lkml.kernel.org/r/20170404152744.26687-3-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
drivers/video/fbdev/efifb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 8c4dc1e..b827a81 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -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>
@@ -143,6 +144,8 @@ static struct attribute *efifb_attrs[] = {
};
ATTRIBUTE_GROUPS(efifb);
+static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
+
static int efifb_probe(struct platform_device *dev)
{
struct fb_info *info;
@@ -152,7 +155,7 @@ static int efifb_probe(struct platform_device *dev)
unsigned int size_total;
char *option = NULL;
- if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
+ if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
return -ENODEV;
if (fb_get_options("efifb", &option))
@@ -360,3 +363,64 @@ static struct platform_driver efifb_driver = {
};
builtin_platform_driver(efifb_driver);
+
+#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
+
+static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
+
+static void claim_efifb_bar(struct pci_dev *dev, int idx)
+{
+ u16 word;
+
+ pci_bar_found = true;
+
+ pci_read_config_word(dev, PCI_COMMAND, &word);
+ if (!(word & PCI_COMMAND_MEMORY)) {
+ pci_dev_disabled = true;
+ dev_err(&dev->dev,
+ "BAR %d: assigned to efifb but device is disabled!\n",
+ idx);
+ return;
+ }
+
+ if (pci_claim_resource(dev, idx)) {
+ pci_dev_disabled = true;
+ dev_err(&dev->dev,
+ "BAR %d: failed to claim resource for efifb!\n", idx);
+ return;
+ }
+
+ dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
+}
+
+static void efifb_fixup_resources(struct pci_dev *dev)
+{
+ u64 base = screen_info.lfb_base;
+ u64 size = screen_info.lfb_size;
+ int i;
+
+ if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
+ return;
+
+ if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+ base |= (u64)screen_info.ext_lfb_base << 32;
+
+ if (!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 <= base && res->end >= base + size - 1) {
+ claim_efifb_bar(dev, i);
+ break;
+ }
+ }
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY,
+ 16, efifb_fixup_resources);
+
+#endif
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer
2017-04-04 15:27 ` [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer Ard Biesheuvel
2017-04-05 7:58 ` [tip:efi/urgent] efi/fb: " tip-bot for Ard Biesheuvel
2017-04-05 10:30 ` tip-bot for Ard Biesheuvel
@ 2017-05-19 16:27 ` Bjorn Helgaas
2017-05-19 16:37 ` Ard Biesheuvel
2 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2017-05-19 16:27 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
leif.lindholm, lorenzo.pieralisi, linux-kernel, linux-pci
[+cc linux-pci]
On Tue, Apr 04, 2017 at 04:27:44PM +0100, Ard Biesheuvel wrote:
> 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 non-x86 quirk to the EFI fb driver to find the BAR associated
> with the GOP base address, and claim the BAR resource so that the PCI
> core will not move it.
I know this has already been merged as 55d728a40d36 ("efi/fb: Avoid
reconfiguration of BAR that covers the framebuffer"), and I'm not
suggesting that we revert it, but I have some misgivings.
One is the "#ifndef CONFIG_X86". In principle, there is nothing arch-
specific here. I don't think it's a good idea to build in dependencies on
things like "this arch preserves (or reprograms) PCI BARs". The PCI core
may reprogram all, some, or none of the PCI BARs, depending on what (if
anything) firmware has done.
Another is the use of pci_claim_resource() to express the idea that "this
BAR can not be moved". We have IORESOURCE_PCI_FIXED for that purpose, and
previous versions of the patch used that. I understand there was some
problem with that, but I wish we could figure out and fix that problem
instead of using a different mechanism.
I'm not even completely sold on the idea that we need to prevent the BAR
from being moved. I'm not a UEFI expert, but if this requirement is in the
spec, we should reference it. If not, it should be sufficient to remember
the boot-time BAR value, match the GOP base to *that*, and map it to
whatever the current BAR value is.
> Fixes: 9822504c1fa5 ("efifb: Enable the efi-framebuffer platform driver ...")
> Cc: <stable@vger.kernel.org> # v4.7+
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> drivers/video/fbdev/efifb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 8c4dc1e1f94f..758960b6aec9 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -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>
> @@ -143,6 +144,8 @@ static struct attribute *efifb_attrs[] = {
> };
> ATTRIBUTE_GROUPS(efifb);
>
> +static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -152,7 +155,7 @@ static int efifb_probe(struct platform_device *dev)
> unsigned int size_total;
> char *option = NULL;
>
> - if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> + if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
> return -ENODEV;
>
> if (fb_get_options("efifb", &option))
> @@ -360,3 +363,64 @@ static struct platform_driver efifb_driver = {
> };
>
> builtin_platform_driver(efifb_driver);
> +
> +#ifndef CONFIG_X86
> +
> +static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
> +
> +static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +{
> + u16 word;
> +
> + pci_bar_found = true;
> +
> + pci_read_config_word(dev, PCI_COMMAND, &word);
> + if (!(word & PCI_COMMAND_MEMORY)) {
> + pci_dev_disabled = true;
> + dev_err(&dev->dev,
> + "BAR %d: assigned to efifb but device is disabled!\n",
> + idx);
> + return;
> + }
> +
> + if (pci_claim_resource(dev, idx)) {
> + pci_dev_disabled = true;
> + dev_err(&dev->dev,
> + "BAR %d: failed to claim resource for efifb!\n", idx);
> + return;
> + }
> +
> + dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> +}
> +
> +static void efifb_fixup_resources(struct pci_dev *dev)
> +{
> + u64 base = screen_info.lfb_base;
> + u64 size = screen_info.lfb_size;
> + int i;
> +
> + if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> + return;
> +
> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> + base |= (u64)screen_info.ext_lfb_base << 32;
> +
> + if (!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 <= base && res->end >= base + size - 1) {
> + claim_efifb_bar(dev, i);
> + break;
> + }
> + }
> +}
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY,
> + 16, efifb_fixup_resources);
> +
> +#endif
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer
2017-05-19 16:27 ` [PATCH 2/2] efifb: " Bjorn Helgaas
@ 2017-05-19 16:37 ` Ard Biesheuvel
2017-05-19 20:44 ` Bjorn Helgaas
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2017-05-19 16:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-efi@vger.kernel.org, Ingo Molnar, Thomas Gleixner,
H . Peter Anvin, Leif Lindholm, Lorenzo Pieralisi,
linux-kernel@vger.kernel.org, linux-pci
Hi Bjorn,
On 19 May 2017 at 17:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc linux-pci]
>
> On Tue, Apr 04, 2017 at 04:27:44PM +0100, Ard Biesheuvel wrote:
>> 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 non-x86 quirk to the EFI fb driver to find the BAR associated
>> with the GOP base address, and claim the BAR resource so that the PCI
>> core will not move it.
>
> I know this has already been merged as 55d728a40d36 ("efi/fb: Avoid
> reconfiguration of BAR that covers the framebuffer"), and I'm not
> suggesting that we revert it, but I have some misgivings.
>
Thanks for taking a look. I have been struggling with this issue for a
while now.
> One is the "#ifndef CONFIG_X86". In principle, there is nothing arch-
> specific here. I don't think it's a good idea to build in dependencies on
> things like "this arch preserves (or reprograms) PCI BARs". The PCI core
> may reprogram all, some, or none of the PCI BARs, depending on what (if
> anything) firmware has done.
>
IIRC this was a late addition. I agree that the issue exists in theory
on x86 as well. However, I was primarily dealing with the reality of
arm64 systems that suddenly explode in inexplicable ways after
upgrading the kernel to one that happens to have EFIFB built in. The
patch went into -stable as well, so I still think adding #ifndef
CONFIG_X86 was the right choice here.
> Another is the use of pci_claim_resource() to express the idea that "this
> BAR can not be moved". We have IORESOURCE_PCI_FIXED for that purpose, and
> previous versions of the patch used that. I understand there was some
> problem with that, but I wish we could figure out and fix that problem
> instead of using a different mechanism.
>
OK. The problem was that IORESOURCE_PCI_FIXED does not prevent the PCI
subsystem from handing out the same range to another device.
> I'm not even completely sold on the idea that we need to prevent the BAR
> from being moved. I'm not a UEFI expert, but if this requirement is in the
> spec, we should reference it. If not, it should be sufficient to remember
> the boot-time BAR value, match the GOP base to *that*, and map it to
> whatever the current BAR value is.
>
There is no such requirement in the spec. The graphics output protocol
is not described in terms of PCI, BARs etc. The framebuffer is simply
a memory range with side effects that is left enabled when handing
over to the OS.
In summary, I am as unhappy with the patch as you are, but it is still
an improvement over the previous situation, so let's simply
collaborate to get this into shape going forward.
My preference would be to investigate IORESOURCE_PCI_FIXED again,
because that still seems like the best way to deal with a live
framebuffer on a PCI device that has memory decoding enabled. It
should also address the issue with the current version of the patch,
i.e., that claiming resources at this point is not possible if the
device resides behind a bridge.
So is there any guidance you can give as to how to proceed? If we set
IORESOURCE_PCI_FIXED, how should be prevent the PCI layer from
assigning this resource elsewhere if we cannot claim it yet?
Regards,
Ard.
>> Fixes: 9822504c1fa5 ("efifb: Enable the efi-framebuffer platform driver ...")
>> Cc: <stable@vger.kernel.org> # v4.7+
>> Cc: Matt Fleming <matt@codeblueprint.co.uk>
>> Cc: Peter Jones <pjones@redhat.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> drivers/video/fbdev/efifb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
>> index 8c4dc1e1f94f..758960b6aec9 100644
>> --- a/drivers/video/fbdev/efifb.c
>> +++ b/drivers/video/fbdev/efifb.c
>> @@ -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>
>> @@ -143,6 +144,8 @@ static struct attribute *efifb_attrs[] = {
>> };
>> ATTRIBUTE_GROUPS(efifb);
>>
>> +static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>> +
>> static int efifb_probe(struct platform_device *dev)
>> {
>> struct fb_info *info;
>> @@ -152,7 +155,7 @@ static int efifb_probe(struct platform_device *dev)
>> unsigned int size_total;
>> char *option = NULL;
>>
>> - if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
>> + if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
>> return -ENODEV;
>>
>> if (fb_get_options("efifb", &option))
>> @@ -360,3 +363,64 @@ static struct platform_driver efifb_driver = {
>> };
>>
>> builtin_platform_driver(efifb_driver);
>> +
>> +#ifndef CONFIG_X86
>> +
>> +static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>> +
>> +static void claim_efifb_bar(struct pci_dev *dev, int idx)
>> +{
>> + u16 word;
>> +
>> + pci_bar_found = true;
>> +
>> + pci_read_config_word(dev, PCI_COMMAND, &word);
>> + if (!(word & PCI_COMMAND_MEMORY)) {
>> + pci_dev_disabled = true;
>> + dev_err(&dev->dev,
>> + "BAR %d: assigned to efifb but device is disabled!\n",
>> + idx);
>> + return;
>> + }
>> +
>> + if (pci_claim_resource(dev, idx)) {
>> + pci_dev_disabled = true;
>> + dev_err(&dev->dev,
>> + "BAR %d: failed to claim resource for efifb!\n", idx);
>> + return;
>> + }
>> +
>> + dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
>> +}
>> +
>> +static void efifb_fixup_resources(struct pci_dev *dev)
>> +{
>> + u64 base = screen_info.lfb_base;
>> + u64 size = screen_info.lfb_size;
>> + int i;
>> +
>> + if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
>> + return;
>> +
>> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>> + base |= (u64)screen_info.ext_lfb_base << 32;
>> +
>> + if (!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 <= base && res->end >= base + size - 1) {
>> + claim_efifb_bar(dev, i);
>> + break;
>> + }
>> + }
>> +}
>> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY,
>> + 16, efifb_fixup_resources);
>> +
>> +#endif
>> --
>> 2.9.3
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer
2017-05-19 16:37 ` Ard Biesheuvel
@ 2017-05-19 20:44 ` Bjorn Helgaas
2017-05-19 22:04 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2017-05-19 20:44 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi@vger.kernel.org, Ingo Molnar, Thomas Gleixner,
H . Peter Anvin, Leif Lindholm, Lorenzo Pieralisi,
linux-kernel@vger.kernel.org, linux-pci
On Fri, May 19, 2017 at 05:37:30PM +0100, Ard Biesheuvel wrote:
> Hi Bjorn,
>
> On 19 May 2017 at 17:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc linux-pci]
> >
> > On Tue, Apr 04, 2017 at 04:27:44PM +0100, Ard Biesheuvel wrote:
> >> 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 non-x86 quirk to the EFI fb driver to find the BAR associated
> >> with the GOP base address, and claim the BAR resource so that the PCI
> >> core will not move it.
> >
> > I know this has already been merged as 55d728a40d36 ("efi/fb: Avoid
> > reconfiguration of BAR that covers the framebuffer"), and I'm not
> > suggesting that we revert it, but I have some misgivings.
> ...
> > Another is the use of pci_claim_resource() to express the idea that "this
> > BAR can not be moved". We have IORESOURCE_PCI_FIXED for that purpose, and
> > previous versions of the patch used that. I understand there was some
> > problem with that, but I wish we could figure out and fix that problem
> > instead of using a different mechanism.
>
> OK. The problem was that IORESOURCE_PCI_FIXED does not prevent the PCI
> subsystem from handing out the same range to another device.
Yes, this would definitely be a problem. There must be a path where
we start doing the reassignment before we claim the resources that
have already been assigned. That's what seems backwards to me -- it
seems like we should claim things that are valid first so we know to
avoid them.
> > I'm not even completely sold on the idea that we need to prevent the BAR
> > from being moved. I'm not a UEFI expert, but if this requirement is in the
> > spec, we should reference it. If not, it should be sufficient to remember
> > the boot-time BAR value, match the GOP base to *that*, and map it to
> > whatever the current BAR value is.
>
> There is no such requirement in the spec. The graphics output protocol
> is not described in terms of PCI, BARs etc. The framebuffer is simply
> a memory range with side effects that is left enabled when handing
> over to the OS.
>
> In summary, I am as unhappy with the patch as you are, but it is still
> an improvement over the previous situation, so let's simply
> collaborate to get this into shape going forward.
>
> My preference would be to investigate IORESOURCE_PCI_FIXED again,
> because that still seems like the best way to deal with a live
> framebuffer on a PCI device that has memory decoding enabled. It
> should also address the issue with the current version of the patch,
> i.e., that claiming resources at this point is not possible if the
> device resides behind a bridge.
>
> So is there any guidance you can give as to how to proceed? If we set
> IORESOURCE_PCI_FIXED, how should be prevent the PCI layer from
> assigning this resource elsewhere if we cannot claim it yet?
My preference would actually be to remember the boot BAR values and
map to the current values because that avoids the unnecessary
restriction. The IORESOURCE_PCI_FIXED uses that seem legitimate to me
are the legacy VGA and IDE things (stuff that's not described by BARs
and *can't* be moved) and the new "Enhanced Allocation" stuff
(basically a way to describe more non-BAR resources).
We do something sort of similar with pci_revert_fw_address(), although
it's currently only implemented for x86. I could imagine a more
generic solution that could be used for this GOP issue and could also
replace pci_revert_fw_address().
Conceptually it could be as simple as adding 7 u64 boot-time BAR
values (6 BARs + the ROM) to struct pci_dev. We went to a lot of
trouble to implement the pcibios_fwaddrmap stuff on x86, and I can't
remember why it's x86-specific. Maybe we thought the extra 56 bytes
per dev was too much overhead (it does seem like a lot for such a
limited use case). Maybe there's a clever way to store just the BARs
we actually change and keep that long enough for efifb to use it.
It *would* be nice to fix the problem with IORESOURCE_PCI_FIXED, and I
think it would help clean up PCI resource management a lot. Ideally
we would be able to claim host bridge resources even before scanning
the bus, then claim BARs immediately when we discover them. That
would allow us to automatically use any setup done by firmware, and
reassign anything that we couldn't claim.
But I think this will end up being difficult because we currently do
the PCI bus scan before looking at ACPI resources, and sometimes those
ACPI resources overlap with the host bridge windows. Every time I
look at this, I get discouraged.
Bjorn
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efifb: Avoid reconfiguration of BAR that covers the framebuffer
2017-05-19 20:44 ` Bjorn Helgaas
@ 2017-05-19 22:04 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2017-05-19 22:04 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-efi@vger.kernel.org, Ingo Molnar, Thomas Gleixner,
H . Peter Anvin, Leif Lindholm, Lorenzo Pieralisi,
linux-kernel@vger.kernel.org, linux-pci
On 19 May 2017 at 21:44, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, May 19, 2017 at 05:37:30PM +0100, Ard Biesheuvel wrote:
>> Hi Bjorn,
>>
>> On 19 May 2017 at 17:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > [+cc linux-pci]
>> >
>> > On Tue, Apr 04, 2017 at 04:27:44PM +0100, Ard Biesheuvel wrote:
>> >> 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 non-x86 quirk to the EFI fb driver to find the BAR associated
>> >> with the GOP base address, and claim the BAR resource so that the PCI
>> >> core will not move it.
>> >
>> > I know this has already been merged as 55d728a40d36 ("efi/fb: Avoid
>> > reconfiguration of BAR that covers the framebuffer"), and I'm not
>> > suggesting that we revert it, but I have some misgivings.
>> ...
>
>> > Another is the use of pci_claim_resource() to express the idea that "this
>> > BAR can not be moved". We have IORESOURCE_PCI_FIXED for that purpose, and
>> > previous versions of the patch used that. I understand there was some
>> > problem with that, but I wish we could figure out and fix that problem
>> > instead of using a different mechanism.
>>
>> OK. The problem was that IORESOURCE_PCI_FIXED does not prevent the PCI
>> subsystem from handing out the same range to another device.
>
> Yes, this would definitely be a problem. There must be a path where
> we start doing the reassignment before we claim the resources that
> have already been assigned. That's what seems backwards to me -- it
> seems like we should claim things that are valid first so we know to
> avoid them.
>
>> > I'm not even completely sold on the idea that we need to prevent the BAR
>> > from being moved. I'm not a UEFI expert, but if this requirement is in the
>> > spec, we should reference it. If not, it should be sufficient to remember
>> > the boot-time BAR value, match the GOP base to *that*, and map it to
>> > whatever the current BAR value is.
>>
>> There is no such requirement in the spec. The graphics output protocol
>> is not described in terms of PCI, BARs etc. The framebuffer is simply
>> a memory range with side effects that is left enabled when handing
>> over to the OS.
>>
>> In summary, I am as unhappy with the patch as you are, but it is still
>> an improvement over the previous situation, so let's simply
>> collaborate to get this into shape going forward.
>>
>> My preference would be to investigate IORESOURCE_PCI_FIXED again,
>> because that still seems like the best way to deal with a live
>> framebuffer on a PCI device that has memory decoding enabled. It
>> should also address the issue with the current version of the patch,
>> i.e., that claiming resources at this point is not possible if the
>> device resides behind a bridge.
>>
>> So is there any guidance you can give as to how to proceed? If we set
>> IORESOURCE_PCI_FIXED, how should be prevent the PCI layer from
>> assigning this resource elsewhere if we cannot claim it yet?
>
> My preference would actually be to remember the boot BAR values and
> map to the current values because that avoids the unnecessary
> restriction. The IORESOURCE_PCI_FIXED uses that seem legitimate to me
> are the legacy VGA and IDE things (stuff that's not described by BARs
> and *can't* be moved) and the new "Enhanced Allocation" stuff
> (basically a way to describe more non-BAR resources).
>
> We do something sort of similar with pci_revert_fw_address(), although
> it's currently only implemented for x86. I could imagine a more
> generic solution that could be used for this GOP issue and could also
> replace pci_revert_fw_address().
>
I already proposed something like this a while ago:
http://marc.info//?l=linux-fbdev&m=149190021316410&w=2
> Conceptually it could be as simple as adding 7 u64 boot-time BAR
> values (6 BARs + the ROM) to struct pci_dev. We went to a lot of
> trouble to implement the pcibios_fwaddrmap stuff on x86, and I can't
> remember why it's x86-specific. Maybe we thought the extra 56 bytes
> per dev was too much overhead (it does seem like a lot for such a
> limited use case). Maybe there's a clever way to store just the BARs
> we actually change and keep that long enough for efifb to use it.
>
> It *would* be nice to fix the problem with IORESOURCE_PCI_FIXED, and I
> think it would help clean up PCI resource management a lot. Ideally
> we would be able to claim host bridge resources even before scanning
> the bus, then claim BARs immediately when we discover them. That
> would allow us to automatically use any setup done by firmware, and
> reassign anything that we couldn't claim.
>
> But I think this will end up being difficult because we currently do
> the PCI bus scan before looking at ACPI resources, and sometimes those
> ACPI resources overlap with the host bridge windows. Every time I
> look at this, I get discouraged.
>
> Bjorn
^ permalink raw reply [flat|nested] 16+ messages in thread