lkml.org 
[lkml]   [2016]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers
Date
Hi Bjorn, Lorenzo

Many Thanks for your replies and suggestions

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: 25 February 2016 19:59
> To: Lorenzo Pieralisi
> Cc: Gabriele Paoloni; 'Mark Rutland'; Guohanjun (Hanjun Guo); Wangzhou
> (B); liudongdong (C); Linuxarm; qiujiang; 'bhelgaas@google.com';
> 'arnd@arndb.de'; 'tn@semihalf.com'; 'linux-pci@vger.kernel.org';
> 'linux-kernel@vger.kernel.org'; xuwei (O); 'linux-
> acpi@vger.kernel.org'; 'jcm@redhat.com'; zhangjukuo; Liguozhu
> (Kenneth); 'linux-arm-kernel@lists.infradead.org'
> Subject: Re: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for
> HiSilicon SoCs Host Controllers
>
> On Thu, Feb 25, 2016 at 12:07:50PM +0000, Lorenzo Pieralisi wrote:
> > On Thu, Feb 25, 2016 at 03:01:19AM +0000, Gabriele Paoloni wrote:
> >
> > [...]
> >
> > > > I think the relevant spec is the PCI Firmware Spec, r3.0, sec
> 4.1.2.
> > > > Note 2 in that section says the address range of an MMCFG region
> > > > must be reserved by declaring a motherboard resource, i.e.,
> included
> > > > 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
> > 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?

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?

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...?

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

>
> > Have a look at drivers/pnp/system.c for PNP0c02
> >
> > > So probably I can use acpi_get_devices("PNP0C02",...) to retrieve
> it
> > > from the quirk match function, I will look into this...
> > >
> > > >
> > > > > On the other side, since this is an exception only for the
> config
> > > > > space address of our host controller (as said before all the
> buses
> > > > > below the root one support ECAM), I think that it is right to
> put
> > > > > 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
> > > > system correctly. That means we must have a generic way to learn
> what
> > > > address space is consumed by the host controller so we don't try
> to
> > > > 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
> > > without our host controller support and running on our platform?
> >
> > I *think* the point here is that your host controller config space
> should be
> > reserved through PNP0c02 so that the kernel will reserve it through
> the
> > generic PNP0c02 driver even if your host controller driver (and
> related
> > _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.
>
> > 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),
> > 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.
>
> > 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
> > kernel did not even register the PNP0c02 driver
> (drivers/pnp/system.c)
> > (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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

\
 
 \ /
  Last update: 2016-02-27 10:21    [W:0.122 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site