From patchwork Mon Apr 10 17:29:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 9673759 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 35833600CB for ; Mon, 10 Apr 2017 17:29:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 16246204C2 for ; Mon, 10 Apr 2017 17:29:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0A89E277D9; Mon, 10 Apr 2017 17:29:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 07F3B204C2 for ; Mon, 10 Apr 2017 17:29:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: References:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=V7qZ9+FyGoyRjuLzRZojpxiUTJNH7UL3hQJH43Qdvpc=; b=CCjKTJSufN0su6 apiV8W2SqvHMmAcuiOW/pjPZM7m9q/AHr6WPd/LDISp5ObTL09UdXzFSnKvApfPxG6O4HeAnpYgf8 QD1/LerU6au3r9NlJHSzOGv6OCkkLT0Giljg0lF9DWm2hSdB1sH7bsGkf/kXWIlh5yrgVYuWIemRI D+A2LvBiQUMukf/omPH7feI2jmXplaZANCdj3BzjCTbMwoMg/RfHRShy+AM/lAujLTy8LtbwTlU// 2psxIluMBl30cYxO//Cm0Bmc3NUo+w/jMLQ5uKk3/JVtvYBE0EiooMzHrO8zlpnI30mjgCXBoqbGY y7gBc+jpZa1zfKJx48gw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cxd8T-0006na-R3; Mon, 10 Apr 2017 17:29:53 +0000 Received: from mail-io0-x22d.google.com ([2607:f8b0:4001:c06::22d]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cxd8O-0006lb-V3 for linux-arm-kernel@lists.infradead.org; Mon, 10 Apr 2017 17:29:51 +0000 Received: by mail-io0-x22d.google.com with SMTP id a103so40064414ioj.1 for ; Mon, 10 Apr 2017 10:29:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=vkTs5xcsnuAyCeFLp6fhAuUpblE8rqIx5juzltKqu3Y=; b=kBmVBiMV5d4SkFxRHcUoNneuAU7c9Lxv+m6P5Kbxr75q4moW5p3z9vkQe9AQdM/Gf/ SXWNvtUTSU/zFuOC+lNfqwEoE6wl1XUNYLSRvq4Y0ta/RpjJZfESd3wlcl8PfCnHLT5v N3uXo8VRJny2sxIMkmJ4m8o9I7h1fEd6s7S/A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=vkTs5xcsnuAyCeFLp6fhAuUpblE8rqIx5juzltKqu3Y=; b=QREdoVgiikTbKLFCZR+y+1P3J+xupLg9NEB9PuixvxU+bdclL9KE4B+WQlrMFBsm9i KQdIhQ0VcYKyF9S0SLvyDqmHowXcWFaPAt4U0dIHkhy1JCgkrP9713Ts7A/0Dp5Ak1yU kcRWA2OQPFjV0HOoPh+OX0qqulV+y3Ps/CcLzEvDIHulLbd1a94+QwF4iRRfKsGEtR7I WriKkKyeY8A+TSa6PmxMGCbJWvBrYExaP4mPCxR9MJuIQevt/GZjpUYuLyJPmY64+9KC JQfVSddO94qEl+7ahFzY7JKzc+WYZ1Ul9AHvW0cXUh5TQX2FWYiu+mzDMlbXcHkBCtHF 22tg== X-Gm-Message-State: AN3rC/6Z0tI6XwCUOkl327P3v7IZduHxlXdg9ezVSdvqG/Aa/ta3v8TIVrdCKCH8rjPScrhNHN0pE9qbA1MIxi2J X-Received: by 10.36.110.18 with SMTP id w18mr3052686itc.37.1491845367769; Mon, 10 Apr 2017 10:29:27 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.201.76 with HTTP; Mon, 10 Apr 2017 10:29:27 -0700 (PDT) In-Reply-To: References: <466dcd2b-a781-2fe7-6ef0-5a3767c793e0@codeaurora.org> <27f50de3-721e-e8ec-00c8-b7a9d3cff0d6@codeaurora.org> <20170330100524.GA22801@red-moon> <20170410165328.GA5248@red-moon> From: Ard Biesheuvel Date: Mon, 10 Apr 2017 18:29:27 +0100 Message-ID: Subject: Re: [PATCH v3] efifb: avoid reconfiguration of BAR that covers the framebuffer To: Lorenzo Pieralisi X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170410_102949_088913_9EF0F4E7 X-CRM114-Status: GOOD ( 39.44 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "linux-efi@vger.kernel.org" , Matt Fleming , linux-pci , Peter Jones , Sinan Kaya , Heyi Guo , Lukas Wunner , Hanjun Guo , Bjorn Helgaas , Yinghai Lu , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 10 April 2017 at 18:13, Ard Biesheuvel wrote: > On 10 April 2017 at 17:53, Lorenzo Pieralisi wrote: >> On Mon, Apr 10, 2017 at 04:28:11PM +0100, Ard Biesheuvel wrote: >>> On 2 April 2017 at 16:16, Ard Biesheuvel wrote: >>> > On 30 March 2017 at 14:50, Sinan Kaya wrote: >>> >> On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: >>> >>> On 30 March 2017 at 11:09, Ard Biesheuvel wrote: >>> >>>> On 30 March 2017 at 11:05, Lorenzo Pieralisi wrote: >>> >>>>> On Thu, Mar 30, 2017 at 09:46:39AM +0100, Ard Biesheuvel wrote: >>> >>>>> >>> >>>>> [...] >>> >>>>> >>> >>>>>>> I'm asking why we don't fix the actual problem in PCIe ARM64 adaptation instead >>> >>>>>>> of working around it by quirks. >>> >>>>>>> >>> >>>>>>> I don't see any reason why ACPI ARM64 should carry the burden of legacy systems. >>> >>>>>>> >>> >>>>>>> Legacy only applies to DT based systems. >>> >>>>>>> >>> >>>>>> >>> >>>>>> I fully agree with this point: ACPI implies firmware, and so we should >>> >>>>>> be able to rely on firmware to have initialized the PCIe subsystem by >>> >>>>>> the time the kernel gets to access it. >>> >>>>> >>> >>>>> https://lkml.org/lkml/2016/3/3/458 >>> >>>>> >>> >>>> >>> >>>> I don't think the fact that at least one system existed over a year >>> >>>> ago whose UEFI assigned resources incorrectly should prevent us from >>> >>>> being normative in this case. >>> >>> >>> >>> In any case, given that EFIFB is enabled by default on some distros, >>> >>> and the fact that DT boot is affected as well, I should get this patch >>> >>> in to prevent serious potential issues that could arise when someone >>> >>> with a graphical UEFI stack updates to such a new kernel. >>> >>> >>> >>> So I think we are in agreement that this is needed on both ARM and >>> >>> arm64, since their PCI configuration is usually not preserved. The >>> >>> open question is whether there is any harm in enabling it for x86 as >>> >>> well. >>> >>> >>> >> >>> >> Agreed, the other issue is about compatibility with UEFI and future >>> >> proofing Linux for other potential issues like hotplug reservation. >>> >> >>> > >>> > OK, given the lack of feedback regarding the suitability of this patch >>> > for x86, I am going to rework it as a ARM/arm64 only patch, and queue >>> > it as a fix for v4.11. This way, we can backport it to stable (which >>> > is arguably appropriate, given that upgrading to a EFIFB enabled >>> > kernel may cause severe breakage for existing systems that implement >>> > the GOP protocol), and easily change the patch to apply to x86 going >>> > forwards, by removing the #ifdefs >>> > >>> >>> As it turns out, this patch does not solve the problem completely. >>> >>> For EFI framebuffers that are backed by a PCI bar of a device residing >>> on bus 0, things work happily. However, claiming resources for devices >>> behind bridges doesn't work. >> >> May I ask you to elaborate on this please ? It is because we do not >> claim the bridge windows upstream the device and they are reassigned ? >> > > The pci_claim_resource() call fails like this > > pci 0000:01:01.0: can't claim BAR 0 [mem 0x10000000-0x10ffffff pref]: > no compatible bridge window > pci 0000:01:01.0: BAR 0: failed to claim resource for efifb! > > which is caused by the fact that the parent resources are all zeroes. > It appears the BAR configuration is never read from the bridges, so > the only way we will ever have meaningful values in there is if we > allocate them ourselves. > >>> Given that we have not made the situation worse, fixing it is less >>> urgent than it was before. I.e., there is no longer a risk of >>> inadvertently poking the wrong BAR when writing to the framebuffer. >>> There is a regression in functionality, though, since EFI fb devices >>> that happened to work (because the firmware BAR == the kernel BAR) >>> have stopped working if they are behind a bridge, which is of course >>> always the case for PCIe. >>> >>> So before starting the next round of hacking to work around this, I >>> would like rekindle the discussion regarding the way we blindly >>> reassign all resources on ACPI/arm64 systems, and whether there is a >>> way imaginable to avoid doing that. >> >> There is a way if the whole ARM ecosystem work together to sort this >> out and we think that's the right way to do; I am personally not >> entirely convinced about that. >> > > So what are the pros and cons here? EFI fb is not a hugely important > use case, but it is one that relies on BARs staying where they are. > Are there others like that? > >>> I suppose the state of the BARs as we inherit it from the firmware >>> cannot be blindly trusted (and IIUC, this is Lorenzo's primary issue >>> with it). So should there be some side channel (UEFI config table >>> perhaps?) to describe this? >> >> PCI firmware specifications rev 3.1, 4.6.5, "_DSM for Ignoring PCI >> Boot Configurations". >> >> Do we want to enforce it on ARM ? I do not know to be honest (and it >> still would not solve the DT firmware case). >> > > No, it doesn't. But that doesn't mean we shouldn't solve it on ACPI if > the pros outweigh the cons. > >> Whatever we do, it is not going to be clean cut IMO. I think that >> what x86 does is sensible (well, minus the link ordering dependency we >> discovered), I can do it for ARM64 but get ready for regressions and >> I still think we have no real FW binding support that would make this >> behaviour robust. >> >> I can provide you with examples where, by simply claiming resources >> on an ARM system you trigger resources allocation regressions by >> preventing the kernel from allocating bigger bridge windows than >> the ones set-up by FW. >> > > So how is this specific to ARM then? If we are running the same > resource allocation routines, why should we end up with a firmware > allocation that the kernel cannot use? > >>> How do other architectures deal with this? >> >> On an arch specific basis that make things work. >> >>> Is this what the PCI bios accesses are for? >> >> Which ones :) ? >> > > Well, some of them :-) > > I guess the question was if the overridden __weak methods are supposed > to hook into tables or other BIOS structures that contain information > about the PCI resource allocations by the firmware. > >>> In any case, I have updated the UEFI firmware we have for ARM Juno to >>> use EDK2's generic PCI host bridge driver instead of one that was >>> specially written for ARM Juno, and may deviate in the way it >>> allocates PCI resources. >> >> As long as the kernel is free to reallocate them and that FW quiesces >> devices at FW->OS handover I see no issues with that. >> > > The reason is to eliminate another unknown from the discussion whether > UEFI can be expected to leave the entire PCI hierarchy in a sane > state. > >> If we want to try to claim the whole resource tree on boot (in ACPI) >> I can send a patch for that but there will be regressions. >> > > I would like to see it, yes. FWIW, the following minimal [naive] patch pcie_bus_configure_settings(child); running under QEMU+UEFI with the following PCI topology -[0000:00]-+-00.0 Red Hat, Inc. Device 0008 +-01.0-[01]----01.0 Device 1234:1111 +-02.0-[02]--+-02.0 Red Hat, Inc Virtio RNG | \-03.0 Red Hat, Inc Virtio block device \-03.0-[03]----04.0 Red Hat, Inc Virtio network device results in the log below, preserving the configuration created by UEFI pci_bus 0000:00: root bus resource [mem 0x10000000-0x3efeffff window] pci_bus 0000:00: root bus resource [io 0x0000-0xffff window] pci_bus 0000:00: root bus resource [mem 0x8000000000-0xffffffffff window] pci_bus 0000:00: root bus resource [bus 00-0f] pci 0000:00:00.0: [1b36:0008] type 00 class 0x060000 pci 0000:00:01.0: [1b36:0001] type 01 class 0x060400 pci 0000:00:01.0: reg 0x10: [mem 0x8000202000-0x80002020ff 64bit] pci 0000:00:02.0: [1b36:0001] type 01 class 0x060400 pci 0000:00:02.0: reg 0x10: [mem 0x8000201000-0x80002010ff 64bit] pci 0000:00:03.0: [1b36:0001] type 01 class 0x060400 pci 0000:00:03.0: reg 0x10: [mem 0x8000200000-0x80002000ff 64bit] pci 0000:01:01.0: [1234:1111] type 00 class 0x030000 pci 0000:01:01.0: reg 0x10: [mem 0x10000000-0x10ffffff pref] pci 0000:01:01.0: reg 0x18: [mem 0x11200000-0x11200fff] pci 0000:01:01.0: reg 0x30: [mem 0xffff0000-0xffffffff pref] pci 0000:01:01.0: can't claim BAR 0 [mem 0x10000000-0x10ffffff pref]: no compatible bridge window pci 0000:01:01.0: BAR 0: failed to claim resource for efifb! pci 0000:00:01.0: PCI bridge to [bus 01] pci 0000:00:01.0: bridge window [mem 0x11200000-0x112fffff] pci 0000:00:01.0: bridge window [mem 0x10000000-0x10ffffff 64bit pref] pci 0000:02:02.0: [1af4:1005] type 00 class 0x00ff00 pci 0000:02:02.0: reg 0x10: [io 0x1040-0x105f] pci 0000:02:02.0: reg 0x20: [mem 0x8000004000-0x8000007fff 64bit pref] pci 0000:02:03.0: [1af4:1001] type 00 class 0x010000 pci 0000:02:03.0: reg 0x10: [io 0x1000-0x103f] pci 0000:02:03.0: reg 0x14: [mem 0x11100000-0x11100fff] pci 0000:02:03.0: reg 0x20: [mem 0x8000000000-0x8000003fff 64bit pref] pci 0000:00:02.0: PCI bridge to [bus 02] pci 0000:00:02.0: bridge window [io 0x1000-0x1fff] pci 0000:00:02.0: bridge window [mem 0x11100000-0x111fffff] pci 0000:00:02.0: bridge window [mem 0x8000000000-0x80000fffff 64bit pref] pci 0000:03:04.0: [1af4:1000] type 00 class 0x020000 pci 0000:03:04.0: reg 0x10: [io 0x0000-0x001f] pci 0000:03:04.0: reg 0x14: [mem 0x11000000-0x11000fff] pci 0000:03:04.0: reg 0x20: [mem 0x8000100000-0x8000103fff 64bit pref] pci 0000:03:04.0: reg 0x30: [mem 0xfffc0000-0xffffffff pref] pci 0000:00:03.0: PCI bridge to [bus 03] pci 0000:00:03.0: bridge window [io 0x0000-0x0fff] pci 0000:00:03.0: bridge window [mem 0x11000000-0x110fffff] pci 0000:00:03.0: bridge window [mem 0x8000100000-0x80001fffff 64bit pref] pci 0000:00:01.0: BAR 14: assigned [mem 0x10000000-0x117fffff] pci 0000:00:01.0: BAR 15: assigned [mem 0x8000000000-0x8000ffffff 64bit pref] pci 0000:00:02.0: BAR 14: assigned [mem 0x11800000-0x118fffff] pci 0000:00:02.0: BAR 15: assigned [mem 0x8001000000-0x80010fffff 64bit pref] pci 0000:00:03.0: BAR 14: assigned [mem 0x11900000-0x119fffff] pci 0000:00:03.0: BAR 15: assigned [mem 0x8001100000-0x80011fffff 64bit pref] pci 0000:00:02.0: BAR 13: assigned [io 0x1000-0x1fff] pci 0000:00:03.0: BAR 13: assigned [io 0x2000-0x2fff] pci 0000:00:01.0: BAR 0: assigned [mem 0x8001200000-0x80012000ff 64bit] pci 0000:00:02.0: BAR 0: assigned [mem 0x8001200100-0x80012001ff 64bit] pci 0000:00:03.0: BAR 0: assigned [mem 0x8001200200-0x80012002ff 64bit] pci 0000:01:01.0: BAR 0: assigned [mem 0x10000000-0x10ffffff pref] pci 0000:01:01.0: BAR 6: assigned [mem 0x11000000-0x1100ffff pref] pci 0000:01:01.0: BAR 2: assigned [mem 0x11010000-0x11010fff] pci 0000:00:01.0: PCI bridge to [bus 01] pci 0000:00:01.0: bridge window [mem 0x10000000-0x117fffff] pci 0000:00:01.0: bridge window [mem 0x8000000000-0x8000ffffff 64bit pref] pci 0000:02:02.0: BAR 4: assigned [mem 0x8001000000-0x8001003fff 64bit pref] pci 0000:02:03.0: BAR 4: assigned [mem 0x8001004000-0x8001007fff 64bit pref] pci 0000:02:03.0: BAR 1: assigned [mem 0x11800000-0x11800fff] pci 0000:02:03.0: BAR 0: assigned [io 0x1000-0x103f] pci 0000:02:02.0: BAR 0: assigned [io 0x1040-0x105f] pci 0000:00:02.0: PCI bridge to [bus 02] pci 0000:00:02.0: bridge window [io 0x1000-0x1fff] pci 0000:00:02.0: bridge window [mem 0x11800000-0x118fffff] pci 0000:00:02.0: bridge window [mem 0x8001000000-0x80010fffff 64bit pref] pci 0000:03:04.0: BAR 6: assigned [mem 0x11900000-0x1193ffff pref] pci 0000:03:04.0: BAR 4: assigned [mem 0x8001100000-0x8001103fff 64bit pref] pci 0000:03:04.0: BAR 1: assigned [mem 0x11940000-0x11940fff] pci 0000:03:04.0: BAR 0: assigned [io 0x2000-0x201f] pci 0000:00:03.0: PCI bridge to [bus 03] pci 0000:00:03.0: bridge window [io 0x2000-0x2fff] pci 0000:00:03.0: bridge window [mem 0x11900000-0x119fffff] pci 0000:00:03.0: bridge window [mem 0x8001100000-0x80011fffff 64bit pref] pci_bus 0000:00: resource 4 [mem 0x10000000-0x3efeffff window] pci_bus 0000:00: resource 5 [io 0x0000-0xffff window] pci_bus 0000:00: resource 6 [mem 0x8000000000-0xffffffffff window] pci_bus 0000:01: resource 1 [mem 0x10000000-0x117fffff] pci_bus 0000:01: resource 2 [mem 0x8000000000-0x8000ffffff 64bit pref] pci_bus 0000:02: resource 0 [io 0x1000-0x1fff] pci_bus 0000:02: resource 1 [mem 0x11800000-0x118fffff] pci_bus 0000:02: resource 2 [mem 0x8001000000-0x80010fffff 64bit pref] pci_bus 0000:03: resource 0 [io 0x2000-0x2fff] pci_bus 0000:03: resource 1 [mem 0x11900000-0x119fffff] pci_bus 0000:03: resource 2 [mem 0x8001100000-0x80011fffff 64bit pref] pci 0000:00:01.0: PCI bridge to [bus 01] pci 0000:00:01.0: bridge window [mem 0x10000000-0x117fffff] pci 0000:00:01.0: bridge window [mem 0x8000000000-0x8000ffffff 64bit pref] pci 0000:00:02.0: PCI bridge to [bus 02] pci 0000:00:02.0: bridge window [io 0x1000-0x1fff] pci 0000:00:02.0: bridge window [mem 0x11800000-0x118fffff] pci 0000:00:02.0: bridge window [mem 0x8001000000-0x80010fffff 64bit pref] pci 0000:00:03.0: PCI bridge to [bus 03] pci 0000:00:03.0: bridge window [io 0x2000-0x2fff] pci 0000:00:03.0: bridge window [mem 0x11900000-0x119fffff] pci 0000:00:03.0: bridge window [mem 0x8001100000-0x80011fffff 64bit pref] diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 4f0e3ebfea4b..37c4d2f116a4 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -27,7 +27,7 @@ */ void pcibios_fixup_bus(struct pci_bus *bus) { - /* nothing to do, expected to be removed in the future */ + pci_read_bridge_bases(bus); } /* @@ -208,8 +208,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) if (!bus) return NULL; - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); + pci_bus_claim_resources(bus); list_for_each_entry(child, &bus->children, node)