Re: [PATCH] PCI: Add information about describing PCI in ACPI

[Bjorn Helgaas]

On Wed, Nov 23, 2016 at 09:06:33AM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 23, 2016 at 07:28:12AM +0000, Ard Biesheuvel wrote:
> > On 23 November 2016 at 01:06, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > On Tue, Nov 22, 2016 at 10:09:50AM +0000, Ard Biesheuvel wrote:
> > >> On 17 November 2016 at 17:59, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> > >
> > >> > +PCI host bridges are PNP0A03 or PNP0A08 devices.  Their _CRS should
> > >> > +describe all the address space they consume.  In principle, this would
> > >> > +be all the windows they forward down to the PCI bus, as well as the
> > >> > +bridge registers themselves.  The bridge registers include things like
> > >> > +secondary/subordinate bus registers that determine the bus range below
> > >> > +the bridge, window registers that describe the apertures, etc.  These
> > >> > +are all device-specific, non-architected things, so the only way a
> > >> > +PNP0A03/PNP0A08 driver can manage them is via _PRS/_CRS/_SRS, which
> > >> > +contain the device-specific details.  These bridge registers also
> > >> > +include ECAM space, since it is consumed by the bridge.
> > >> > +
> > >> > +ACPI defined a Producer/Consumer bit that was intended to distinguish
> > >> > +the bridge apertures from the bridge registers [4, 5].  However,
> > >> > +BIOSes didn't use that bit correctly, and the result is that OSes have
> > >> > +to assume that everything in a PCI host bridge _CRS is a window.  That
> > >> > +leaves no way to describe the bridge registers in the PNP0A03/PNP0A08
> > >> > +device itself.
> > >>
> > >> Is that universally true? Or is it still possible to do the right
> > >> thing here on new ACPI architectures such as arm64?
> > >
> > > That's a very good question.  I had thought that the ACPI spec had
> > > given up on Consumer/Producer completely, but I was wrong.  In the 6.0
> > > spec, the Consumer/Producer bit is still documented in the Extended
> > > Address Space Descriptor (sec 6.4.3.5.4).  It is documented as
> > > "ignored" in the QWord, DWord, and Word descriptors (sec 6.4.3.5.1,2,3).
> > >
> > > Linux looks at the producer_consumer bit in acpi_decode_space(), which
> > > I think is used for all these descriptors (QWord, DWord, Word, and
> > > Extended).  This doesn't quite follow the spec -- we probably should
> > > ignore it except for Extended.  In any event, acpi_decode_space() sets
> > > IORESOURCE_WINDOW for Producer descriptors, but we don't test
> > > IORESOURCE_WINDOW in the PCI host bridge code.
> > >
> > > x86 and ia64 supply their own pci_acpi_root_prepare_resources()
> > > functions that call acpi_pci_probe_root_resources(), which parses _CRS
> > > and looks at producer_consumer.  Then they do a little arch-specific
> > > stuff on the result.
> > >
> > > On arm64 we use acpi_pci_probe_root_resources() directly, with no
> > > arch-specific stuff.
> > >
> > > On all three arches, we ignore the Consumer/Producer bit, so all the
> > > resources are treated as Producers, e.g., as bridge windows.
> > >
> > > I think we *could* implement an arm64 version of
> > > pci_acpi_root_prepare_resources() that would pay attention to the
> > > Consumer/Producer bit by checking IORESOURCE_WINDOW.  To be spec
> > > compliant, we would have to use Extended descriptors for all bridge
> > > windows, even if they would fit in a DWord or QWord.
> > >
> > > Should we do that?  I dunno.  I'd like to hear your opinion(s).
> > >
> > 
> > Yes, I think we should. If the spec allows for a way for a PNP0A03
> > device to describe all of its resources unambiguously, we should not
> > be relying on workarounds that were designed for another architecture
> > in another decade (for, presumably, another OS)
> > 
> > Just for my understanding, we will need to use extended descriptors
> > for all consumed *and* produced regions, even though dword/qword are
> > implicitly produced-only, due to the fact that the bit is ignored?
> 
> From an ACPI spec point of view, I would say QWord/DWord/Word
> descriptors are implicitly *consumer*-only because ResourceConsumer
> is the default and they don't have a bit to indicate otherwise.
> 
> The current code assumes all PNP0A03 resources are producers.  If we
> implement an arm64 pci_acpi_root_prepare_resources() that pays
> attention to the Consumer/Producer bit, we would have to:
> 
>   - Reserve all producer regions in the iomem/ioport trees.  This is
>     already done via pci_acpi_root_add_resources(), but we might need
>     a new check to handle consumers differently.
> 
>   - Reserve all consumer regions.  This corresponds to what
>     pnp/system.c does for PNP0C02 devices.  This is similar to the
>     producer regions, but I think the consumer ones should be marked
>     IORESOURCE_BUSY.
> 
>   - Use every producer (IORESOURCE_WINDOW) as a host bridge window.
> 
> I think it's a bug that acpi_decode_space() looks at producer_consumer
> for QWord/DWord/Word descriptors, but I think QWord/DWord/Word
> descriptors for consumed regions should be safe, as long as they don't
> set the Consumer/Producer bit.

I'm going to post a couple very lightly-tested patches that should
make us ignore the Consumer/Producer bit for QWord/DWord/Word.  I'd
appreciate any discussion about whether that's the right approach.

Bjorn