diff mbox

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

Message ID CAKv+Gu9dS4OhLbBw59yKYQmoJ8SpFexzk9yH=XfXnzn8NJ4mcg@mail.gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Ard Biesheuvel April 10, 2017, 5:29 p.m. UTC 
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]

Comments

Lorenzo Pieralisi April 11, 2017, 1:16 p.m. UTC | #1 
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]
Ard Biesheuvel April 11, 2017, 4:06 p.m. UTC | #2 
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.
Yinghai Lu April 23, 2017, 1:45 a.m. UTC | #3 
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
Ard Biesheuvel April 27, 2017, 1:55 p.m. UTC | #4 
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?
Yinghai Lu April 28, 2017, 8:51 p.m. UTC | #5 
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
diff mbox

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);

        list_for_each_entry(child, &bus->children, node)