| Message ID | 1490196629-28088-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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? > +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); Maybe this can be constrained to PCI_BASE_CLASS_DISPLAY? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 March 2017 at 19:31, Lukas Wunner <lukas@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. > >> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); > > Maybe this can be constrained to PCI_BASE_CLASS_DISPLAY? > How does one do that with DECLARE_PCI_FIXUP_HEADER? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/22/2017 3:31 PM, Lukas Wunner wrote: > 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. Does this code still work with the case where resources are not reassigned on ARM64? As far as I know, kernel honors resource assignments done by the UEFI BIOS if they are correct. Kernel will reassign the resources only if something is wrong. Will this code break other platforms/architectures? -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 March 2017 at 19:36, Sinan Kaya <okaya@codeaurora.org> wrote: > On 3/22/2017 3:31 PM, Lukas Wunner wrote: >> 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. > > Does this code still work with the case where resources are not reassigned on ARM64? > > As far as I know, kernel honors resource assignments done by the UEFI BIOS if > they are correct. Kernel will reassign the resources only if something is wrong. > No, the kernel always reassigns all BARs on arm64. > Will this code break other platforms/architectures? > Which platforms/architectures are you referring to? EFIFB on a PCI device is currently broken on arm64. On x86, it works, given that BARs are usually not reassigned, and so the patch should be a no-op in that case (although I'd argue it is still an improvement to check whether the device that owns the BAR actually has memory decoding enabled before we attach the framebuffer driver to it) -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/22/2017 3:41 PM, Ard Biesheuvel wrote: >> As far as I know, kernel honors resource assignments done by the UEFI BIOS if >> they are correct. Kernel will reassign the resources only if something is wrong. >> > No, the kernel always reassigns all BARs on arm64. I think this is where the problem is. I'm not seeing this happen on QDF2400 which supports ACPI + UEFI BIOS combination only. I see that kernel honored the resources assigned by UEFI BIOS if I compare the BAR addresses. I see reassignment only when something is horribly broken. Then, there would be a bridge configuration invalid message in the boot log to confirm this. > >> Will this code break other platforms/architectures? >> > Which platforms/architectures are you referring to? EFIFB on a PCI > device is currently broken on arm64. In general or on your particular platform? > On x86, it works, given that BARs > are usually not reassigned, and so the patch should be a no-op in that > case (although I'd argue it is still an improvement to check whether > the device that owns the BAR actually has memory decoding enabled > before we attach the framebuffer driver to it) > I'm fine as long as it doesn't break anything. That's why, I'm asking. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 March 2017 at 19:49, Sinan Kaya <okaya@codeaurora.org> wrote: > On 3/22/2017 3:41 PM, Ard Biesheuvel wrote: >>> As far as I know, kernel honors resource assignments done by the UEFI BIOS if >>> they are correct. Kernel will reassign the resources only if something is wrong. >>> >> No, the kernel always reassigns all BARs on arm64. > > I think this is where the problem is. > > I'm not seeing this happen on QDF2400 which supports ACPI + UEFI BIOS > combination only. > > I see that kernel honored the resources assigned by UEFI BIOS if I compare > the BAR addresses. > Well, if you are using the standard MdeModulePkg PciHostBridgeDxe driver in UEFI, the logic is quite similar, and so you usually end up with the same resource assignments. But the kernel does recompute them from scratch, and ignores the existing configuration that is present in the BARs > I see reassignment only when something is horribly broken. Then, there would > be a bridge configuration invalid message in the boot log to confirm this. > >> >>> Will this code break other platforms/architectures? >>> >> Which platforms/architectures are you referring to? EFIFB on a PCI >> device is currently broken on arm64. > > In general or on your particular platform? > >> On x86, it works, given that BARs >> are usually not reassigned, and so the patch should be a no-op in that >> case (although I'd argue it is still an improvement to check whether >> the device that owns the BAR actually has memory decoding enabled >> before we attach the framebuffer driver to it) >> > > I'm fine as long as it doesn't break anything. That's why, I'm asking. > If you have working EFIFB on your platform, it works by accident. This patch is needed to ensure that the BAR associated with the EFI framebuffer is left untouched. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/22/2017 3:52 PM, Ard Biesheuvel wrote: > On 22 March 2017 at 19:49, Sinan Kaya <okaya@codeaurora.org> wrote: >> On 3/22/2017 3:41 PM, Ard Biesheuvel wrote: >>>> As far as I know, kernel honors resource assignments done by the UEFI BIOS if >>>> they are correct. Kernel will reassign the resources only if something is wrong. >>>> >>> No, the kernel always reassigns all BARs on arm64. >> >> I think this is where the problem is. >> >> I'm not seeing this happen on QDF2400 which supports ACPI + UEFI BIOS >> combination only. >> >> I see that kernel honored the resources assigned by UEFI BIOS if I compare >> the BAR addresses. >> > > Well, if you are using the standard MdeModulePkg PciHostBridgeDxe > driver in UEFI, the logic is quite similar, and so you usually end up > with the same resource assignments. But the kernel does recompute them > from scratch, and ignores the existing configuration that is present > in the BARs Yes, standard stuff in UEFI. OK. That may explain what I'm seeing. > >> I see reassignment only when something is horribly broken. Then, there would >> be a bridge configuration invalid message in the boot log to confirm this. >> >>> >>>> Will this code break other platforms/architectures? >>>> >>> Which platforms/architectures are you referring to? EFIFB on a PCI >>> device is currently broken on arm64. >> >> In general or on your particular platform? >> >>> On x86, it works, given that BARs >>> are usually not reassigned, and so the patch should be a no-op in that >>> case (although I'd argue it is still an improvement to check whether >>> the device that owns the BAR actually has memory decoding enabled >>> before we attach the framebuffer driver to it) >>> >> >> I'm fine as long as it doesn't break anything. That's why, I'm asking. >> > > If you have working EFIFB on your platform, it works by accident. This > patch is needed to ensure that the BAR associated with the EFI > framebuffer is left untouched. > I never claimed that. I was just double checking your assumptions. I really don't like ARM64 specific PCI code in general. PCI doesn't care about CPU type. I hope to see this patch become part of common code. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 March 2017 at 19:57, Sinan Kaya <okaya@codeaurora.org> wrote: > On 3/22/2017 3:52 PM, Ard Biesheuvel wrote: [..] >> If you have working EFIFB on your platform, it works by accident. This >> patch is needed to ensure that the BAR associated with the EFI >> framebuffer is left untouched. >> > > I never claimed that. I was just double checking your assumptions. > I really don't like ARM64 specific PCI code in general. PCI doesn't care > about CPU type. I hope to see this patch become part of common code. > Good! Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 22, 2017 at 07:32:43PM +0000, Ard Biesheuvel wrote: > On 22 March 2017 at 19:31, Lukas Wunner <lukas@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. I have no opinion on executing the quirk on x86 as well, I was just confused by the discrepancy between commit message and patch, but that can easily be remedied with a copy+paste of what you replied to Sinan: "On x86, it works, given that BARs are usually not reassigned, and so the patch should be a no-op in that case, although it should still be an improvement to check whether the device that owns the BAR actually has memory decoding enabled before we attach the framebuffer driver to it." > >> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); > > > > Maybe this can be constrained to PCI_BASE_CLASS_DISPLAY? > > How does one do that with DECLARE_PCI_FIXUP_HEADER? With DECLARE_PCI_FIXUP_CLASS_HEADER(). Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 March 2017 at 08:48, Lukas Wunner <lukas@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@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. > I have no opinion on executing the quirk on x86 as well, I was just > confused by the discrepancy between commit message and patch, but that > can easily be remedied with a copy+paste of what you replied to Sinan: > > "On x86, it works, given that BARs are usually not reassigned, > and so the patch should be a no-op in that case, although it > should still be an improvement to check whether the device that > owns the BAR actually has memory decoding enabled before we > attach the framebuffer driver to it." > > OK, I will include that. >> >> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); >> > >> > Maybe this can be constrained to PCI_BASE_CLASS_DISPLAY? >> >> How does one do that with DECLARE_PCI_FIXUP_HEADER? > > With DECLARE_PCI_FIXUP_CLASS_HEADER(). > Ah ok, thanks for pointing that out. I do wonder whether it makes sense to keep the code as is, so that we spot unexpected configurations, i.e., where the GOP points into a BAR that is unrelated to graphics. In this case, I think we should disable efifb rather than proceed without claiming any PCI resources (as we did without this patch) -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 09:04:03AM +0000, Ard Biesheuvel wrote: > On 23 March 2017 at 08:48, Lukas Wunner <lukas@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@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). For all the talk about PCI being arch agnostic as I said manifold times before, that's just theory. In practice different arches treat PCI FW set-up differently, it would be ideal to make them uniform but legacy is huge and there is a massive risk of triggering regressions, it is no mean feat (if achievable at all). Lorenzo > > I have no opinion on executing the quirk on x86 as well, I was just > > confused by the discrepancy between commit message and patch, but that > > can easily be remedied with a copy+paste of what you replied to Sinan: > > > > "On x86, it works, given that BARs are usually not reassigned, > > and so the patch should be a no-op in that case, although it > > should still be an improvement to check whether the device that > > owns the BAR actually has memory decoding enabled before we > > attach the framebuffer driver to it." > > > > > > OK, I will include that. > > >> >> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); > >> > > >> > Maybe this can be constrained to PCI_BASE_CLASS_DISPLAY? > >> > >> How does one do that with DECLARE_PCI_FIXUP_HEADER? > > > > With DECLARE_PCI_FIXUP_CLASS_HEADER(). > > > > Ah ok, thanks for pointing that out. > > I do wonder whether it makes sense to keep the code as is, so that we > spot unexpected configurations, i.e., where the GOP points into a BAR > that is unrelated to graphics. In this case, I think we should disable > efifb rather than proceed without claiming any PCI resources (as we > did without this patch) -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 March 2017 at 10:57, Lorenzo Pieralisi <lorenzo.pieralisi@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@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@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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.htmldiff --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);
On Thu, Mar 23, 2017 at 12:25:48PM +0000, Ard Biesheuvel wrote: > On 23 March 2017 at 10:57, Lorenzo Pieralisi <lorenzo.pieralisi@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@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@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. 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() ? x86 implements pcibios_resource_survey_bus(), but that's called only for hotplugged bridges (?): drivers/acpi/pci_root.c -> acpi_pci_root_add() From the log below, I see two options: - x86 pcibios_resource_survey() is called before any root bus is added (so it does precious little) - x86 pcibios_resource_survey() is called after root busses are added and the PCI device fixup has been applied (and the additional claim in it succeeds silently) I am certainly missing something here, I will grab an x86 box to add a couple of logs and see if my assumptions are correct, I would like to get to the bottom of this, I think that's important. Lorenzo > For the record: I applied the following hunk on top of the current > version of this patch > > 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); > > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 March 2017 at 14:31, Lorenzo Pieralisi <lorenzo.pieralisi@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@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@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@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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 March 2017 at 15:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 23 March 2017 at 14:31, Lorenzo Pieralisi <lorenzo.pieralisi@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@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@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@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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lorenzo, On 3/23/2017 6:57 AM, Lorenzo Pieralisi wrote: >> 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). > > For all the talk about PCI being arch agnostic as I said manifold > times before, that's just theory. In practice different arches > treat PCI FW set-up differently, it would be ideal to make them > uniform but legacy is huge and there is a massive risk of triggering > regressions, it is no mean feat (if achievable at all). Can we explore having a uniform behavior across ALL ACPI bases systems by trying to reuse the resources assigned by firmware first and reassign them only if something is wrong? There are protocols like hotplug reservation in UEFI. It looks like Linux is not honoring any of these protocols by being too smart. Sinan -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 March 2017 at 22:27, Sinan Kaya <okaya@codeaurora.org> wrote: > Hi Lorenzo, > > On 3/23/2017 6:57 AM, Lorenzo Pieralisi wrote: >>> 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). >> >> For all the talk about PCI being arch agnostic as I said manifold >> times before, that's just theory. In practice different arches >> treat PCI FW set-up differently, it would be ideal to make them >> uniform but legacy is huge and there is a massive risk of triggering >> regressions, it is no mean feat (if achievable at all). > > Can we explore having a uniform behavior across ALL ACPI bases systems > by trying to reuse the resources assigned by firmware first and reassign > them only if something is wrong? > > There are protocols like hotplug reservation in UEFI. It looks like Linux > is not honoring any of these protocols by being too smart. > Could you be more specific? Which protocol do you mean exactly, and how does not reusing resources interfere with it? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/28/2017 5:39 PM, Ard Biesheuvel wrote: > On 28 March 2017 at 22:27, Sinan Kaya <okaya@codeaurora.org> wrote: >> Hi Lorenzo, >> >> On 3/23/2017 6:57 AM, Lorenzo Pieralisi wrote: >>>> 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). >>> >>> For all the talk about PCI being arch agnostic as I said manifold >>> times before, that's just theory. In practice different arches >>> treat PCI FW set-up differently, it would be ideal to make them >>> uniform but legacy is huge and there is a massive risk of triggering >>> regressions, it is no mean feat (if achievable at all). >> >> Can we explore having a uniform behavior across ALL ACPI bases systems >> by trying to reuse the resources assigned by firmware first and reassign >> them only if something is wrong? >> >> There are protocols like hotplug reservation in UEFI. It looks like Linux >> is not honoring any of these protocols by being too smart. >> > > Could you be more specific? Which protocol do you mean exactly, and > how does not reusing resources interfere with it? > Let's assume for the moment that if ARM64 adaptation of PCIE reused the firmware assigned BAR addresses, would you still have this problem that you are trying to fix by a quirk? The protocol that I was looking at specifically is called hotplug reservation protocol. http://www.intel.com/content/www/us/en/architecture-and-technology/unified-extensible-firmware-interface/efi-hot-plug-pci-initialization-protocol-specification.html The idea is to pad the PCIe bridge window by some amount in UEFI. You boot the system without any cards present. when you do to hotplug, OS uses the range reserved by the BIOS for inserting the new card. what I see is that the bridge windows get overridden by the OS. 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. If there is still a legacy problem in ACPI ARM64, generic Linux code deals with this using some DMI/SMBIOS and setting a time bomb like products after this date shall follow the new behavior. We are not in a very hardened position when it comes to changes for ACPI based systems. Sinan -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 March 2017 at 22:49, Sinan Kaya <okaya@codeaurora.org> wrote: > On 3/28/2017 5:39 PM, Ard Biesheuvel wrote: >> On 28 March 2017 at 22:27, Sinan Kaya <okaya@codeaurora.org> wrote: >>> Hi Lorenzo, >>> >>> On 3/23/2017 6:57 AM, Lorenzo Pieralisi wrote: >>>>> 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). >>>> >>>> For all the talk about PCI being arch agnostic as I said manifold >>>> times before, that's just theory. In practice different arches >>>> treat PCI FW set-up differently, it would be ideal to make them >>>> uniform but legacy is huge and there is a massive risk of triggering >>>> regressions, it is no mean feat (if achievable at all). >>> >>> Can we explore having a uniform behavior across ALL ACPI bases systems >>> by trying to reuse the resources assigned by firmware first and reassign >>> them only if something is wrong? >>> >>> There are protocols like hotplug reservation in UEFI. It looks like Linux >>> is not honoring any of these protocols by being too smart. >>> >> >> Could you be more specific? Which protocol do you mean exactly, and >> how does not reusing resources interfere with it? >> > > Let's assume for the moment that if ARM64 adaptation of PCIE reused the firmware > assigned BAR addresses, would you still have this problem that you are trying to > fix by a quirk? > Probably not. Although I should point out that the same issue exists on DT systems, and so we will need this patch regardless. > The protocol that I was looking at specifically is called hotplug reservation > protocol. > > http://www.intel.com/content/www/us/en/architecture-and-technology/unified-extensible-firmware-interface/efi-hot-plug-pci-initialization-protocol-specification.html > > The idea is to pad the PCIe bridge window by some amount in UEFI. You boot the > system without any cards present. when you do to hotplug, OS uses the range > reserved by the BIOS for inserting the new card. > > what I see is that the bridge windows get overridden by the OS. > > 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. > If there is still a legacy problem in ACPI ARM64, generic Linux code deals with > this using some DMI/SMBIOS and setting a time bomb like products after this date > shall follow the new behavior. > > We are not in a very hardened position when it comes to changes for ACPI based > systems. > > Sinan > > -- > Sinan Kaya > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-03-30 06:09, Ard Biesheuvel wrote: > On 30 March 2017 at 11:05, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> 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. My suggestion is to try to reuse the resources if possible. Otherwise, reassign the resources. There were implementation problems by the time you proposed your patch and it was delaying progess. That was my objection. Now that base pci support is in and everybody is somewhat happy, let's figure out the bugs and get this done. I am ready to test/debug. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: > On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 March 2017 at 14:50, Sinan Kaya <okaya@codeaurora.org> wrote: > On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: >> On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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 -- Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 April 2017 at 16:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 30 March 2017 at 14:50, Sinan Kaya <okaya@codeaurora.org> wrote: >> On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: >>> On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>> On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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. 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. 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? How do other architectures deal with this? Is this what the PCI bios accesses are for? 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 10, 2017 at 04:28:11PM +0100, Ard Biesheuvel wrote: > On 2 April 2017 at 16:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 30 March 2017 at 14:50, Sinan Kaya <okaya@codeaurora.org> wrote: > >> On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: > >>> On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >>>> On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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 ? > 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. > 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). 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. > 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 :) ? > 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. 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. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/10/2017 12:53 PM, Lorenzo Pieralisi wrote: > 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). > Yes for ACPI on ARM. Probably no for DT. > 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. Is it possible to move the code from arch/x86 into drivers/acpi directory so that the code is shared across multiple ACPI based archs. We get more coverage this way. Can we fallback to the allocate everything behavior if we can't use the stuff from FW or would it be too late to recover? We can prevent regressions this way and issue a lot of warnings. We can sit down and fix those warnings over time whether the issue is in UEFI BIOS or the code itself. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 April 2017 at 17:53, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Mon, Apr 10, 2017 at 04:28:11PM +0100, Ard Biesheuvel wrote: >> On 2 April 2017 at 16:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > On 30 March 2017 at 14:50, Sinan Kaya <okaya@codeaurora.org> wrote: >> >> On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: >> >>> On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>>> On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 April 2017 at 18:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 10 April 2017 at 17:53, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: >> On Mon, Apr 10, 2017 at 04:28:11PM +0100, Ard Biesheuvel wrote: >>> On 2 April 2017 at 16:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> > On 30 March 2017 at 14:50, Sinan Kaya <okaya@codeaurora.org> wrote: >>> >> On 3/30/2017 9:38 AM, Ard Biesheuvel wrote: >>> >>> On 30 March 2017 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> >>>> On 30 March 2017 at 11:05, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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] -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.htmldiff --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)
On Mon, Apr 10, 2017 at 06:29:27PM +0100, Ard Biesheuvel wrote: [...] > >>> 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? Not that I am aware of, which means that pros are thin on the ground. > >>> 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. No one is screaming for that to happen, I can implement resource claiming but at the moment I do not see the benefit. [...] > > 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 > > 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); I do not understand this code. If you reassign the whole thing before claiming it I am not sure what's the point of claiming the resources later, that's basically a nop. pci_bus_claim_resources() should suffice (if FW set-up is sane - which also reads bridge bases, BTW). Lorenzo > list_for_each_entry(child, &bus->children, node) > 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] -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 April 2017 at 14:16, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Mon, Apr 10, 2017 at 06:29:27PM +0100, Ard Biesheuvel wrote: > > [...] > >> >>> 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? > > Not that I am aware of, which means that pros are thin on the ground. > OK. Sinan? >> >>> 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. > > No one is screaming for that to happen, I can implement resource > claiming but at the moment I do not see the benefit. > > [...] > >> > 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 >> >> 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); > > I do not understand this code. If you reassign the whole thing > before claiming it I am not sure what's the point of claiming > the resources later, that's basically a nop. pci_bus_claim_resources() > should suffice (if FW set-up is sane - which also reads bridge bases, > BTW). > OK, I'm struggling a bit with this code. As you know, it is not light bedtime reading :-) In any case, I am starting to see your point. While I am able to claim most of the configuration by calling pci_bus_claim_resources() only [modulo some I/O BARs for which I will send out a bugfix shortly], the option ROM BARs are left disabled by UEFI, and so I end up with pci 0000:00:01.0: PCI bridge to [bus 01] pci 0000:00:02.0: PCI bridge to [bus 02] pci 0000:00:03.0: PCI bridge to [bus 03] pci 0000:02:01.0: can't claim BAR 6 [mem 0xffff0000-0xffffffff pref]: no compatible bridge window pci 0000:03:01.0: can't claim BAR 6 [mem 0xfffc0000-0xffffffff pref]: no compatible bridge window and everything else works fine. Both PPC and x86 have similar code to infer from the BARs themselves whether they were programmed, but in our case, it comes down to checking whether a parent resource exists that covers the range. It may still make sense to add support for the _DSM method, and leave it to the firmware to tell the kernel whether the PCI configuration is supposed to be correct, so I will send out an RFC for that as well. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 10, 2017 at 06:29:27PM +0100, Ard Biesheuvel wrote: > /* > @@ -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) > pcie_bus_configure_settings(child); > looks like those two lines are reversed. you should use: pcibios_resource_survey_bus(bus); pci_assign_unassigned_root_bus_resources(bus); please check x86 pcibios_resource_survey_bus() definition in arch/x86/pci/i386.c but pci_bus_claim_resources() should work too. Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 April 2017 at 02:45, Yinghai Lu <yinghai@kernel.org> wrote: > On Mon, Apr 10, 2017 at 06:29:27PM +0100, Ard Biesheuvel wrote: >> /* >> @@ -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) >> pcie_bus_configure_settings(child); >> > > looks like those two lines are reversed. you should use: > pcibios_resource_survey_bus(bus); > pci_assign_unassigned_root_bus_resources(bus); > > please check x86 pcibios_resource_survey_bus() definition in > arch/x86/pci/i386.c > > but pci_bus_claim_resources() should work too. > Thanks Yinghai pcibios_resource_survey_bus() is actually an empty function on arm64, but I guess that is where logic should go that checks the state of the BARs before trying to claim anything? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 27, 2017 at 6:55 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 23 April 2017 at 02:45, Yinghai Lu <yinghai@kernel.org> wrote: >> >> looks like those two lines are reversed. you should use: >> pcibios_resource_survey_bus(bus); >> pci_assign_unassigned_root_bus_resources(bus); >> >> please check x86 pcibios_resource_survey_bus() definition in >> arch/x86/pci/i386.c >> >> but pci_bus_claim_resources() should work too. >> > > pcibios_resource_survey_bus() is actually an empty function on arm64, > but I guess that is where logic should go that checks the state of the > BARs before trying to claim anything? Yes. Please copy for x86 and simplify it for arm64. Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ard, I have one comment inclined. 在 3/22/2017 11:30 PM, Ard Biesheuvel 写道: > 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 quirk to the EFI fb driver to find the BAR associated with > the GOP base address, and set the IORESOURCE_PCI_FIXED attribute so > that the PCI core will leave it alone. > > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Cc: Peter Jones <pjones@redhat.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > v3: check device is enabled before attempting to claim the resource > > drivers/video/fbdev/efifb.c | 60 +++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > index 8c4dc1e1f94f..88f653864a01 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,9 @@ static struct attribute *efifb_attrs[] = { > }; > ATTRIBUTE_GROUPS(efifb); > > +static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ > +static bool pci_dev_disabled; /* was the device disabled? */ > + > static int efifb_probe(struct platform_device *dev) > { > struct fb_info *info; > @@ -152,7 +156,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 +364,57 @@ static struct platform_driver efifb_driver = { > }; > > builtin_platform_driver(efifb_driver); > + > +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) { Have we considered PCI address translation here? I suppose the address reported by EFI GOP should be the address of CPU domain, not address of PCI domain. Address read from PCI BAR is PCI address which should add translation offset before being compared to CPU domain address. I've not familiar with Linux code, so please ignore my comment if the code has supported address translation. Regards, Gary (Heyi Guo) > + claim_efifb_bar(dev, i); > + break; > + } > + } > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources); -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Gary, Thanks for the question. On Wed, May 03, 2017 at 11:09:51AM +0800, Heyi Guo wrote: > Hi Ard, > > I have one comment inclined. > > > 在 3/22/2017 11:30 PM, Ard Biesheuvel 写道: > >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 quirk to the EFI fb driver to find the BAR associated with > >the GOP base address, and set the IORESOURCE_PCI_FIXED attribute so > >that the PCI core will leave it alone. > > > >Cc: Matt Fleming <matt@codeblueprint.co.uk> > >Cc: Peter Jones <pjones@redhat.com> > >Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >--- > >v3: check device is enabled before attempting to claim the resource > > > > drivers/video/fbdev/efifb.c | 60 +++++++++++++++++++- > > 1 file changed, 59 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > >index 8c4dc1e1f94f..88f653864a01 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,9 @@ static struct attribute *efifb_attrs[] = { > > }; > > ATTRIBUTE_GROUPS(efifb); > >+static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ > >+static bool pci_dev_disabled; /* was the device disabled? */ > >+ > > static int efifb_probe(struct platform_device *dev) > > { > > struct fb_info *info; > >@@ -152,7 +156,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 +364,57 @@ static struct platform_driver efifb_driver = { > > }; > > builtin_platform_driver(efifb_driver); > >+ > >+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) { > Have we considered PCI address translation here? I suppose the > address reported by EFI GOP should be the address of CPU domain, not > address of PCI domain. Address read from PCI BAR is PCI address > which should add translation offset before being compared to CPU > domain address. Every address in dev->resource[] is a CPU domain address, so address translation offset should be handled correctly. This is because __pci_read_base() calls pcibios_bus_to_resource() when it fills in dev->resource[], and pcibios_bus_to_resource() applies any host bridge address translation. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Many thanks for your clear answer. Regards, Gary (Heyi Guo) 在 5/18/2017 10:01 PM, Bjorn Helgaas 写道: > Hi Gary, > > Thanks for the question. > > On Wed, May 03, 2017 at 11:09:51AM +0800, Heyi Guo wrote: >> Hi Ard, >> >> I have one comment inclined. >> >> >> 在 3/22/2017 11:30 PM, Ard Biesheuvel 写道: >>> 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 quirk to the EFI fb driver to find the BAR associated with >>> the GOP base address, and set the IORESOURCE_PCI_FIXED attribute so >>> that the PCI core will leave it alone. >>> >>> Cc: Matt Fleming <matt@codeblueprint.co.uk> >>> Cc: Peter Jones <pjones@redhat.com> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> v3: check device is enabled before attempting to claim the resource >>> >>> drivers/video/fbdev/efifb.c | 60 +++++++++++++++++++- >>> 1 file changed, 59 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c >>> index 8c4dc1e1f94f..88f653864a01 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,9 @@ static struct attribute *efifb_attrs[] = { >>> }; >>> ATTRIBUTE_GROUPS(efifb); >>> +static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ >>> +static bool pci_dev_disabled; /* was the device disabled? */ >>> + >>> static int efifb_probe(struct platform_device *dev) >>> { >>> struct fb_info *info; >>> @@ -152,7 +156,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 +364,57 @@ static struct platform_driver efifb_driver = { >>> }; >>> builtin_platform_driver(efifb_driver); >>> + >>> +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) { >> Have we considered PCI address translation here? I suppose the >> address reported by EFI GOP should be the address of CPU domain, not >> address of PCI domain. Address read from PCI BAR is PCI address >> which should add translation offset before being compared to CPU >> domain address. > Every address in dev->resource[] is a CPU domain address, so address > translation offset should be handled correctly. > > This is because __pci_read_base() calls pcibios_bus_to_resource() when > it fills in dev->resource[], and pcibios_bus_to_resource() applies any > host bridge address translation. > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 8c4dc1e1f94f..88f653864a01 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,9 @@ static struct attribute *efifb_attrs[] = { }; ATTRIBUTE_GROUPS(efifb); +static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ +static bool pci_dev_disabled; /* was the device disabled? */ + static int efifb_probe(struct platform_device *dev) { struct fb_info *info; @@ -152,7 +156,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 +364,57 @@ static struct platform_driver efifb_driver = { }; builtin_platform_driver(efifb_driver); + +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_HEADER(PCI_ANY_ID, PCI_ANY_ID, efifb_fixup_resources);
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 quirk to the EFI fb driver to find the BAR associated with the GOP base address, and set the IORESOURCE_PCI_FIXED attribute so that the PCI core will leave it alone. Cc: Matt Fleming <matt@codeblueprint.co.uk> Cc: Peter Jones <pjones@redhat.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- v3: check device is enabled before attempting to claim the resource drivers/video/fbdev/efifb.c | 60 +++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html