Re: [PATCH 05/11] x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code out of arch/x86/ directory

[Tomasz Nowicki]

Hi Lorenzo,

On 09/14/2015 04:55 PM, Tomasz Nowicki wrote:
> On 11.09.2015 13:20, Lorenzo Pieralisi wrote:
> > On Wed, Sep 09, 2015 at 02:47:55PM +0100, Tomasz Nowicki wrote:
> >
> > [...]
> >
> > > > I think (but I am happy to be corrected) that the map_bus() hook
> > > > (ie that's why struct pci_bus is required in eg
> > > > pci_generic_config_write)
> > > > is there to ensure that when the generic accessors are called
> > > > a) we have a valid bus b) the host controllers implementing it
> > > > has been initialized.
> > > >
> > > > I had another look and I noticed you are trying to solve multiple
> > > > things at once:
> > > >
> > > > 1) ACPICA seems to need PCI config space on bus 0 to be working
> > > >      before PCI enumerates (ie before we have a root bus), we need to
> > > >      countercheck on that, but you can't use the generic PCI accessors
> > > >      for that reasons (ie root bus might not be available, you do not
> > > >      have a pci_bus struct)
> > > > 2) the raw_pci_read/write require _generic_ mmio back-ends, since AMD
> > > >      can't cope with standard x86 read/write{b,w,l}
> > > >
> > > > Overall, it seems to me that we can avoid code duplication by
> > > > shuffling your code a bit.
> > > >
> > > > You could modify the generic accessors in drivers/pci/access.c to
> > > > use your mmio back-end instead of using plain read/write{b,w,l}
> > > > functions (we should check if RobH is ok with that there can be
> > > > reasons that prevent this from happening). This would solve the
> > > > AMD mmio issue.
> > > >
> > > > By factoring out the code that actually carries out the reads
> > > > and writes in the accessors basically you decouple the functions
> > > > requiring the struct pci_bus from the ones that does not require it
> > > > (ie raw_pci_{read/write}.
> > > >
> > > > The generic MMIO layer belongs in the drivers/pci/access.c file, it has
> > > > nothing to do with ECAM.
> > > >
> > > > The mmcfg interface should probably live in pci-acpi.c, I do not think
> > > > you need an extra file in there but that's a detail.
> > > >
> > > > Basically the generic accessors would become something like eg:
> > > >
> > > > int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
> > > >                             int where, int size, u32 val)
> > > > {
> > > >        void __iomem *addr;
> > > >
> > > >        addr = bus->ops->map_bus(bus, devfn, where);
> > > >        if (!addr)
> > > >                return PCIBIOS_DEVICE_NOT_FOUND;
> > > >
> > > >        pci_mmio_write(size, addr + where, value);
> > > >
> > > >        return PCIBIOS_SUCCESSFUL;
> > > > }
> > > >
> > > > With that in place using raw_pci_write/read or the generic accessors
> > > > becomes almost identical, with code requiring the pci_bus to be
> > > > created using the generic accessors and ACPICA using the raw version.
> > > >
> > > > I might be missing something, so apologies if that's the case.
> > >
> > > Actually, I think you showed me the right direction :) Here are some
> > > conclusions/comments/concerns. Please correct me if I am wrong:
> > >
> > > 1. We need raw_pci_write/read accessors (based on ECAM) for ARM64 too
> > > but only up to the point where buses are enumerated. From that point on,
> > > we should reuse generic accessors from access.c file, right?
> >
> > Well, I still have not figured out whether on arm64 the raw accessors
> > required by ACPICA make sense.
> >
> > So either arm64 relies on the generic MCFG based raw read and writes
> > or we define the global raw read and writes as empty (ie x86 overrides
> > them anyway).
>
> My concerns/ideas related to raw accessors for ARM64, please correct me
> at any point.
>
> ACPI spec - chapter: 19.5.96 OperationRegion (Declare Operation Region)
> defines PCI_Config as one of region types. Every time ASL opcode
> operates on corresponding PCI config space region, ASL interpreter is
> dispatching address space to our raw accessors, please see
> acpi_ex_pci_config_space_handler, acpi_ev_pci_config_region_setup calls.
> What is more important, such operations may happen after (yes after) bus
> enumeration, but always raw accessors are called at the end with the
> {segment, bus, dev, fn} tuple.
>
> Giving above, here are some ideas:
> 1. We force somehow vendors to avoid operations on PCI config regions in
> ASL code. PCI config region definitions still fall into Hardware Reduced
> profile, so new ACPICA special subset for ARM64 is need. Then raw ACPI
> accessors can be empty (and overridden by x86).
> 2. We provide raw accessors which translate {segment, bus, dev, fn}
> tuple to Linux generic accessors (this can be considered only if PCI
> config accesses happened after bus enumeration for HR profile, thus
> tuple to bus structure map is possible).
> 4. We rely on the generic MCFG based raw read and writes.

I will appreciate your opinion on above ideas.

Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/