[PATCH v3] efifb: avoid reconfiguration of BAR that covers the framebuffer

[Ard Biesheuvel]

On 23 March 2017 at 15:15, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
> On 23 March 2017 at 14:31, Lorenzo Pieralisi <lorenzo.pieralisi at arm.com> wrote:
>> On Thu, Mar 23, 2017 at 12:25:48PM +0000, Ard Biesheuvel wrote:
>>> On 23 March 2017 at 10:57, Lorenzo Pieralisi <lorenzo.pieralisi at arm.com> wrote:
>>> > On Thu, Mar 23, 2017 at 09:04:03AM +0000, Ard Biesheuvel wrote:
>>> >> On 23 March 2017 at 08:48, Lukas Wunner <lukas at wunner.de> wrote:
>>> >> > On Wed, Mar 22, 2017 at 07:32:43PM +0000, Ard Biesheuvel wrote:
>>> >> >> On 22 March 2017 at 19:31, Lukas Wunner <lukas at wunner.de> wrote:
>>> >> >> > On Wed, Mar 22, 2017 at 03:30:29PM +0000, 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.
>>> >> >> >
>>> >> >> > Hm, commit message seems to indicate the issue is restricted to arm64,
>>> >> >> > yet there's no IS_ENABLED(ARM64) to constrain the added code to that arch?
>>> >> >>
>>> >> >> True. I am eager to get some x86 coverage for this, since I would
>>> >> >> expect this not to do any harm. But I'm fine with making it ARM/arm64
>>> >> >> specific in the final version.
>>> >> >
>>> >> > I see.  IIUC, this is only a problem because pci_bus_assign_resources()
>>> >> > is called from arch/arm64/kernel/pci.c:pci_acpi_scan_root() (as well as
>>> >> > the host drivers) and x86 isn't affected because it doesn't do that.
>>> >> >
>>> >>
>>> >> Correct. But on x86 (or rather, on a PC), you can be sure that UEFI
>>> >> (or the legacy PCI bios) performed the resource assignment already.
>>> >> One could argue that this is equally the case when running arm64 in
>>> >> ACPI mode, but in general, you cannot assume the presence of firmware
>>> >> on ARM/arm64 that has already taken care of this, and so the state of
>>> >> the BARs has to be presumed invalid.
>>> >
>>> > The story is a bit more convoluted than that owing to x86 (and other
>>> > arches) legacy.
>>> >
>>> > x86 tries to claim all PCI resources (in two passes - first enabled
>>> > devices, second disabled devices) and that predates ACPI/UEFI.
>>> >
>>> > Mind, x86 reassign resources that can't be claimed too, the only
>>> > difference with ARM64 is that, for the better or the worse, we
>>> > have decided not to claim the FW PCI set-up on ARM64 even if it
>>> > is sane, we do not even try, it was a deliberate choice.
>>> >
>>> > This patch should be harmless on x86 since if the FB PCI BAR is set
>>> > up sanely, claiming it again should be a nop (to be checked).
>>> >
>>>
>>> I have checked this with OVMF under QEMU. Claiming the resource early
>>> like we do this in this patch does not result in any diagnostic output
>>> or other symptoms that would suggest that anything unexpected occurs.
>>
>> There is something that I do not understand on how the resource
>> claiming works on x86. IIUC on x86, resource claiming is done in:
>>
>> arch/x86/pci/legacy.c
>>
>> pci_subsys_init()
>>  -> pcibios_init()
>>   -> pcibios_resource_survey()
>>
>> pci_subsys_init() is run in a subsys_initcall.
>>
>
> Yes, the call trace I get for the resource claim for the efifb BAR
> without this patch is
>
> pci_subsys_init+0x3f/0x43
> - pcibios_init+0x2c/0x3d
> -- pcibios_resource_survey+0x38/0x6a
> --- pci_legacy_init+0x2e/0x2e
> ---- pcibios_allocate_resources+0x8a/0x240
> ----- pci_claim_resource+0xdc/0x140
>
>
>> Now, how do we ensure that resource claiming is carried out _after_
>> the PCI root busses have been actually scanned ?
>>
>> The ACPI scan handler interface (so the interface to actually scan
>> a PCI root bridge in ACPI) is initialized in acpi_init() (which
>> is a subsys_initcall), how do we guarantee that is run before
>> pci_subsys_init() ?
>>
>
> $ nm vmlinux |grep -E '__initcall_(acpi_init|pci_subsys_init)'
> ffffffff81d540b8 t __initcall_acpi_init4
> ffffffff81d54158 t __initcall_pci_subsys_init4
>
> so it appears to depend on link order currently.

Bjorn, what is your take on this?

Claiming the BAR resources associated with the efifb region from a
DECLARE_PCI_FIXUP_HEADER() handler does not seem to interfere with the
way resources are claimed later on. For arm64, we need this to ensure
that the BAR doesn't move, but for x86 this does not seem to be an
issue. This means we could potentially make the quirk ARM-only, but I
am a bit reluctant to do so and create even more divergence.

Thanks,
Ard.