Thread (27 messages) 27 messages, 5 authors, 2016-03-15

[RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers

From: Lorenzo Pieralisi <hidden>
Date: 2016-02-29 11:35:58
Also in: linux-acpi, linux-pci, lkml

quoted
quoted
quoted
quoted
I think the relevant spec is the PCI Firmware Spec, r3.0, sec
4.1.2.
quoted
quoted
quoted
Note 2 in that section says the address range of an MMCFG region
must be reserved by declaring a motherboard resource, i.e.,
included
quoted
quoted
quoted
in the _CRS of a PNP0C02 or similar device.
I had a look a this. So yes the specs says that we should use the
PNP0C02 device if MCFG is not supported.
AFAIK, PNP0C02 is a resource reservation mechanism, the spec says
that
quoted
MCFG regions must be reserved using PNP0C02, even though its
current usage on x86 is a bit unfathomable to me, in particular
in relation to MCFG resources retrieved for hotpluggable bridges (ie
through _CBA, which I think consider the MCFG region as reserved
by default, regardless of PNP0c02):

see pci_mmcfg_check_reserved() arch/x86/pci/mmconfig-shared.c
Yes I checked this and it seems to check if an area of memory from
MCFG is overlapping with any area of memory specified by PNP0C02 
_CRS...

However (maybe I am wrong) it looks to me that this part works
independently of the PNP0c02 driver. It seems that goes directly
to walk the ACPI namespace and look for PNP0C02 HID; as it finds it,
it checks the range of memory specified in the _CRS method and see
if it overlaps with the MCFG resource...am I missing something? 
Well, if I understand the code correctly, the x86 MCFG code, for static
MCFG tables, check that the MCFG regions are part of motherboard
resources (by walking the ACPI namespace as you said). If that's the case,
it does not insert resources into the resource tree till a late_initcall,
(pci_mmcfg_late_insert_resources()) that should run after the PNP0C02 driver
was initialized (?) (I guess, that's a nightmare to understand these initcall
ordering unwritten rules dependencies, if they exist). If the MCFG region is
not part of motherboard resources it is discarded (ie pci_mmconfig_insert()
in arch/x86/pci/mmconfig-shared.c).

I really do not know why x86 code has been implemented like that
and whatever I say it is just speculation, honestly it is not easy
to understand.
If my interpretation is correct, couldn't we just modify 
pci_mmconfig_map_resource() in the latest Nowicki patchset and add
a similar check before insert_resource_conflict() is called?
Yes, but I am not sure that would help much. We can prevent adding
MCFG resources to the resource tree if they are not declared as motherboard
resources, but if they do it is totally unclear to me what we should do.

Should we insert the MCFG region resources into the resource tree before
PNP0C02 driver has a chance to be probed ? Or do what x86 does (ie it does
not insert resources into the resource tree till late_initcall
pci_mmcfg_late_insert_resources()) ? I do not know. If the PNP0C02
driver stays as it is, whatever we do with PNP0C02 regions does not make
much sense to me, that's the gist of what I am saying, I don't know why
x86 code was implemented like that, I will go through the commits history
to find some light here.

Lorenzo
On the other side HiSilicon host bridge quirks could use the address
retrieved by the _CRS method of PNP0C02 for our root complex config
rd/wr...?
 
quoted
I don't know how _CBA-related resources would be reserved.  I haven't
personally worked with any host bridges that supply _CBA, so I don't
know whether or how they handled it.

I think the spec intent was that the Consumer/Producer bit (Extended
Address Space Descriptor, General Flags Bit[0], see ACPI spec sec
6.4.3.5.4) would be used.  Resources such as ECAM areas would be
marked "Consumer", meaning the bridge consumed that space itself, and
windows passed down to the PCI bus would be marked "Producer".

But BIOSes didn't use that bit consistently, so we couldn't rely on
it.  I verified experimentally that Windows didn't pay attention to
that bit either, at least for DWord descriptors:
https://bugzilla.kernel.org/show_bug.cgi?id=15701

It's conceivable that we could still use that bit in Extended Address
Space descriptors, or maybe some hack like pay attention if the bridge
has _CBA, or some such.  Or maybe a BIOS could add a PNP0C02 device
along with the PNP0A03 device that uses _CBA, with the PNP0C02 _CRS
describing the ECAM area referenced by _CBA.  Seeems hacky no matter
how we slice it.
Well about this I don't know much but, having looked at the bugzilla
and considering the current mechanism used by pci_mmcfg_check_reserved()
I have the feeling that this last one is easier to implement and it seems
the one currently used (in mmconfig-shared.c )

Cheers

Gab
quoted
quoted
Have a look at drivers/pnp/system.c for PNP0c02
quoted
So probably I can use acpi_get_devices("PNP0C02",...) to retrieve
it
quoted
quoted
from the quirk match function, I will look into this...
quoted
quoted
On the other side, since this is an exception only for the
config
quoted
quoted
quoted
quoted
space address of our host controller (as said before all the
buses
quoted
quoted
quoted
quoted
below the root one support ECAM), I think that it is right to
put
quoted
quoted
quoted
quoted
this address as a device specific data (in fact the rest of the
config space addresses will be parsed from MCFG).
A kernel with no support for your host controller (and thus no
knowledge of its _DSD) should still be able to operate the rest
of the
quoted
quoted
quoted
system correctly.  That means we must have a generic way to learn
what
quoted
quoted
quoted
address space is consumed by the host controller so we don't try
to
quoted
quoted
quoted
assign it to other devices.
This is something I don't understand much...
Are you talking about a scenario where we have a Kernel image
compiled
quoted
quoted
without our host controller support and running on our platform?
I *think* the point here is that your host controller config space
should be
quoted
reserved through PNP0c02 so that the kernel will reserve it through
the
quoted
generic PNP0c02 driver even if your host controller driver (and
related
quoted
_DSD) is not supported in the kernel.
Right.  Assume you have two top-level devices:

  PNP0A03  PCI host bridge
    _CRS     describes windows
    ????     describes ECAM space consumed
  PNPxxxx  another ACPI device, currently disabled
    _PRS     specifies possible resource settings, may specify no
restrictions
    _SRS     assign resources and enable device
    _CRS     empty until device enabled

When the OS enables PNPxxxx, it must first assign resources to it
using _PRS and _SRS.  We evaluate _PRS to find out what the addresses
PNPxxxx can support.  This tells us things like how wide the address
decoder is, the size of the region required, and any alignment
restrictions -- basically the same information we get by sizing a PCI
BAR.

Now, how do we assign space for PNPxxxx?  In a few cases, _PRS has
only a few specific possibilities, e.g., an x86 legacy serial port
that can be at 0x3f8 or 0x2f8.  But in general, _PRS need not impose
any restrictions.

So in general the OS can use any space that can be routed to PNPxxxx.
If there's an upstream bridge, it may define windows that restrict the
possibilities.  But in this case, there *is* no upstream bridge, so
the possible choices are the entire physical address space of the
platform, except for other things that are already allocated: RAM, the
_CRS settings for other ACPI devices, things reserved by the E820
table (at least on x86), etc.

If PNP0A03 consumes address space for ECAM, that space must be
reported *somewhere* so the OS knows not to place PNPxxxx there.  This
reporting must be generic (not device-specific like _DSD).  The ACPI
core (not drivers) is responsible for managing this address space
because:

  a) the OS is not guaranteed to have drivers for all devices, and

  b) even it *did* have drivers for all devices, the PNPxxxx space may
  be assigned before drivers are initialized.
quoted
I do not understand how PNP0c02 works, currently, by the way.

If I read x86 code correctly, the unassigned PCI bus resources are
assigned in arch/x86/pci/i386.c (?)
fs_initcall(pcibios_assign_resources),
quoted
with a comment:

/**
 * called in fs_initcall (one below subsys_initcall),
 * give a chance for motherboard reserve resources
 */

Problem is, motherboard resources are requested through (?):

drivers/pnp/system.c

which is also initialized at fs_initcall, so it might be called after
core x86 code reassign resources, defeating the purpose PNP0c02 was
designed for, namely, request motherboard regions before resources
are assigned, am I wrong ?
I think you're right.  This is a long-standing screwup in Linux.
IMHO, ACPI resources should be parsed and reserved by the ACPI core,
before any PCI resource management (since PCI host bridges are
represented in ACPI).  But historically PCI devices have enumerated
before ACPI got involved.  And the ACPI core doesn't really pay
attention to _CRS for most devices (with the exception of PNP0C02).

IMO the PNP0C02 code in drivers/pnp/system.c should really be done in
the ACPI core for all ACPI devices, similar to the way the PCI core
reserves BAR space for all PCI devices, even if we don't have drivers
for them.  I've tried to fix this in the past, but it is really a
nightmare to unravel everything.

Because the ACPI core doesn't reserve resources for the _CRS of all
ACPI devices, we're already vulnerable to the problem of placing a
device on top of another ACPI device.  We don't see problems because
on x86, at least, most ACPI devices are already configured by the BIOS
to be enabled and non-overlapping.  But x86 has the advantage of
having extensive test coverage courtesy of Windows, and as long as
_CRS has the right stuff in it, we at least have the potential of
fixing problems in Linux.

If the platform doesn't report resource usage correctly on ARM, we may
not find problems (because we don't have the Windows test suite) and
if we have resource assignment problems because _CRS is lacking, we'll
have no way to fix them.
quoted
As per last Tomasz's patchset, we claim and assign unassigned PCI
resources upon ACPI PCI host bridge probing (which happens at
subsys_initcall time, courtesy of ACPI current code); at that time
the
quoted
kernel did not even register the PNP0c02 driver
(drivers/pnp/system.c)
quoted
(it does that at fs_initcall). On the other hand, we insert MCFG
regions into the resource tree upon MCFG parsing, so I do not
see why we need to rely on PNP0c02 to do that for us (granted, the
mechanism is part of the PCI fw specs, which are x86 centric anyway
ie we can't certainly rely on Int15 e820 to detect reserved memory
on ARM :D)

There is lots of legacy x86 here and Bjorn definitely has more
visibility into that than I have, the ARM world must understand
how this works to make sure we have an agreement.
As you say, there is lots of unpleasant x86 legacy here.  Possibly ARM
has a chance to clean this up and do it more sanely; I'm not sure
whether it's feasible to reverse the ACPI/PCI init order there or not.

Rafael, any thoughts on this whole thing?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help