From patchwork Thu Mar 23 12:25:48 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 742649 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3vpm4S54yXz9s2Q for ; Thu, 23 Mar 2017 23:26:16 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="TdLcJwRT"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933563AbdCWM0P (ORCPT ); Thu, 23 Mar 2017 08:26:15 -0400 Received: from mail-io0-f177.google.com ([209.85.223.177]:33789 "EHLO mail-io0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933538AbdCWM0P (ORCPT ); Thu, 23 Mar 2017 08:26:15 -0400 Received: by mail-io0-f177.google.com with SMTP id f84so81147024ioj.0 for ; Thu, 23 Mar 2017 05:26:09 -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=NG6nwMJX8Hn5OMSOQi4Ru81I/ZD4kGruf+ay1z0SDM4=; b=TdLcJwRTMMWGPg60Pjtoj5KbXvQfD736VrUBZm2t8ESKXqLfEaXo6YgqcFDxdh581N ns7jdQAJXcAiPc/FnFXwTVc/bbek+iPpF5A06ARQJ6RHeexCsTX91bqKRDlJOjCu/Gt4 5Cd2STCaJKYDlPl7hbjdqCNs21mZx4Go54daw= 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=NG6nwMJX8Hn5OMSOQi4Ru81I/ZD4kGruf+ay1z0SDM4=; b=M7g+/Eqbb/ALgyCt1tDxQq7miLKRVFNs2/dbAMcHASzhRCZ5IKozHqW8yyFbDMSUQ3 DEh4cvqRpuS9T5tV8Qwkfb6qSFy2VOP0WG/F7LhsLNjPPKjMDAX+PFMTztjauCqbjR1W PyjEMkI1ufInFPLEBQPAN8Qe7D04Bf65rEv02lfC3+z93aF92GwNH9mfUXKJ7KLc0paC CcLnJmGILKREieSQj6WIcm5SIo9pKbwhurgEHt3oPMiCLOQEXw9OW8aKMXbEPLS5HLJv zSM7ABqUHB+gsjd6BvookyzQs+vV/FYNbjKPcJ1mmaHK/w+ikmvWb6msdVyfP9pLaKaK po+w== X-Gm-Message-State: AFeK/H1/QcoWhuO1TNzwOPRmTxK11k5CV5iWaxeurqL0HvjzXsA6uu6n1XM1tUMa9PbS4NMIrYXmTeX27RUvgE+T X-Received: by 10.107.168.21 with SMTP id r21mr2100365ioe.45.1490271949463; Thu, 23 Mar 2017 05:25:49 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Thu, 23 Mar 2017 05:25:48 -0700 (PDT) In-Reply-To: <20170323105727.GA2441@red-moon> References: <1490196629-28088-1-git-send-email-ard.biesheuvel@linaro.org> <20170322193111.GA8190@wunner.de> <20170323084819.GA23281@h08.hostsharing.net> <20170323105727.GA2441@red-moon> From: Ard Biesheuvel Date: Thu, 23 Mar 2017 12:25:48 +0000 Message-ID: Subject: Re: [PATCH v3] efifb: avoid reconfiguration of BAR that covers the framebuffer To: Lorenzo Pieralisi Cc: Lukas Wunner , "linux-arm-kernel@lists.infradead.org" , "linux-efi@vger.kernel.org" , Matt Fleming , Peter Jones , Bjorn Helgaas , Hanjun Guo , Heyi Guo , linux-pci , Yinghai Lu Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On 23 March 2017 at 10:57, Lorenzo Pieralisi wrote: > On Thu, Mar 23, 2017 at 09:04:03AM +0000, Ard Biesheuvel wrote: >> On 23 March 2017 at 08:48, Lukas Wunner wrote: >> > On Wed, Mar 22, 2017 at 07:32:43PM +0000, Ard Biesheuvel wrote: >> >> On 22 March 2017 at 19:31, Lukas Wunner 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. For the record: I applied the following hunk on top of the current version of this patch which does appear to work fine, and thinking about it, I feel there is only so much we can do to sanity check the efifb against the PCI setup performed by the firmware, so I am inclined to fold this in. diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 88f653864a01..98b7c437a448 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -380,7 +380,10 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx) return; } - if (pci_claim_resource(dev, idx)) { + if (dev->resource[idx].parent != NULL) { + dev_info(&dev->dev, "BAR %d: resource already claimed\n", idx); + while (1) { asm ("hlt"); } + } else if (pci_claim_resource(dev, idx)) { pci_dev_disabled = true; dev_err(&dev->dev, "BAR %d: failed to claim resource for efifb!\n", idx); and got the following output in the kernel log related to BDF 0/2/0 and efifb pci 0000:00:02.0: [1234:1111] type 00 class 0x030000 pci 0000:00:02.0: reg 0x10: [mem 0x80000000-0x80ffffff pref] pci 0000:00:02.0: reg 0x18: [mem 0x81020000-0x81020fff] pci 0000:00:02.0: reg 0x30: [mem 0xffff0000-0xffffffff pref] pci 0000:00:02.0: BAR 0: assigned to efifb pci 0000:00:02.0: can't claim BAR 6 [mem 0xffff0000-0xffffffff pref]: no compatible bridge window pci 0000:00:02.0: BAR 6: assigned [mem 0x08040000-0x0804ffff pref] pci 0000:00:02.0: Video device with shadowed ROM at [mem 0x000c0000-0x000dffff] efifb: probing for efifb efifb: framebuffer at 0x80000000, using 1876k, total 1875k efifb: mode is 800x600x32, linelength=3200, pages=1 efifb: scrolling: redraw efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 whereas a kernel without this patch gives me pci 0000:00:02.0: [1234:1111] type 00 class 0x030000 pci 0000:00:02.0: reg 0x10: [mem 0x80000000-0x80ffffff pref] pci 0000:00:02.0: reg 0x18: [mem 0x81020000-0x81020fff] pci 0000:00:02.0: reg 0x30: [mem 0xffff0000-0xffffffff pref] pci 0000:00:02.0: can't claim BAR 6 [mem 0xffff0000-0xffffffff pref]: no compatible bridge window pci 0000:00:02.0: BAR 6: assigned [mem 0x08040000-0x0804ffff pref] pci 0000:00:02.0: Video device with shadowed ROM at [mem 0x000c0000-0x000dffff] efifb: probing for efifb efifb: framebuffer at 0x80000000, using 1876k, total 1875k efifb: mode is 800x600x32, linelength=3200, pages=1 efifb: scrolling: redraw efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 /proc/iomem looks exactly the same. So in summary, x86 does not seem to care. Furthermore, I tested with this change, as suggested by Lukas diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 88f653864a01..c72d84590343 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -417,4 +417,5 @@ static void efifb_fixup_resources(struct pci_dev *dev) } } } -DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY, + 16, efifb_fixup_resources);