On 23 March 2017 at 14:31, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> 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@xxxxxxx> wrote: >> > On Thu, Mar 23, 2017 at 09:04:03AM +0000, Ard Biesheuvel wrote: >> >> On 23 March 2017 at 08:48, Lukas Wunner <lukas@xxxxxxxxx> wrote: >> >> > On Wed, Mar 22, 2017 at 07:32:43PM +0000, Ard Biesheuvel wrote: >> >> >> On 22 March 2017 at 19:31, Lukas Wunner <lukas@xxxxxxxxx> 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.
