Commit 4f2e2280862a ("RFC: arm: pci: Add PCI cam support to PCI-E ecam driver")

[Mark Kettenis]

> From: Alistair Delva <adelva at google.com>
> Date: Wed, 19 Jan 2022 14:48:21 -0800
> 
> Hi Pali,
> 
> Sorry for the late reply..
> 
> On Thu, Jan 13, 2022 at 4:34 AM Pali Rohár <pali at kernel.org> wrote:
> >
> > Hello!
> >
> > Now I see that you have merged commit 4f2e2280862a ("RFC: arm: pci: Add
> > PCI cam support to PCI-E ecam driver"). It adds some "PCI cam support"
> > with generic DT binding "pci-host-cam-generic".
> >
> > I have tried to find some information about it, but in PCIe
> > specification there is nothing like PCI CAM. And neither in old PCI
> > local bus 2.x or 3.x specs.
> 
> I can't really help you with documentation, but "pci-host-cam-generic"
> isn't something we made up, it is the same name used upstream by
> Linux: https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L60
> 
> We don't have specs, we just reverse engineered what was happening in
> the crosvm vm manager emulation of this device
> (https://chromium.googlesource.com/chromiumos/platform/crosvm/+/refs/heads/master/aarch64/src/fdt.rs).
> 
> > This access looks like a mix of "PCI Configuration Mechanism #1" and
> > "PCI Configuration Mechanism #2" from PCI Local Bus Specification
> > (rev 2.1, sections 3.7.4.1 and 3.7.4.2) and incompatible with both of
> > them. It has layout similar to Mechanism #1 and access similar to #2.
> >
> > PCI Configuration Mechanism #1 uses two registers, one which select
> > config address and second for accessing config space (selected address).
> > But that U-Boot "PCI CAM" is implemented as memory mapped address space,
> > something similar to PCI Configuration Mechanism #2 but with different
> > layout. Also that "PCI CAM" does not set "enable" bit which is per PCI
> > Configuration Mechanism #1 required to access PCI config space.
> >
> > Recently I converted all PCI drivers in U-Boot which uses PCI
> > Configuration Mechanism #1 to use PCI_CONF1_ADDRESS() macro for
> > accessing PCI config space. Basically every HW which uses PCI
> > Configuration Mechanism #1 requires to set "enable" bit like it is
> > described in PCI local bus spec. There is only one exception pci_msc01.c
> > which requires to have "enable" bit unset. And I'm not sure if this is
> > not rather bug in U-Boot driver (but it is in U-Boot in this state for a
> > long time).
> >
> > Do you have some references to this "PCI CAM" specification? Because for
> > me it looks like some vendor/proprietary undocumented API and
> > incompatible with everything which I saw.
> >
> > Therefore I would suggest to not call it "pci-host-cam-generic" or
> > TYPE_PCI as it is not generic for sure (like PCIe ECAM which is
> > documented in PCIe base spec) and also because it is not PCI type (does
> > not match neither PCI Mechanism #1 nor Mechanism #2).
> >
> > Anyway, I would like to know, which hardware uses this unusual PCI
> > config space access?
> 
> I don't know what real hardware uses it, but it is used by crosvm
> (https://chromium.googlesource.com/chromiumos/platform/crosvm)
> 
> > Btw, that commit probably does not work. It uses construction:
> >
> >   (PCI_FUNC(bdf) << 8) | offset
> >
> > offset passed by U-Boot is number between 0..4095 and therefore it
> > overlaps with PCI function number. Either shift by 8 is wrong and it
> > should be shift by 12 or offset needs to be limited just to 0..255. But
> > then there would be no access to PCIe extended space (256..4095), only
> > PCI and I doubt that somebody in 2022 is still doing new development for
> > Conventional PCI local bus hardware.
> 
> I think that's the case for this device, unfortunately. Perhaps we
> should cap offset between 0..255.
> 
> Our change does work; without it, U-Boot can't see any PCI devices.
> With it, they are all shown.
> 
> The other shifts in the change look the same as the Linux driver which
> adjusts the shift from 20 to 16 here:
> https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L18
> 
> I admit, the added logic looks different though:
> https://elixir.bootlin.com/linux/latest/source/drivers/pci/ecam.c#L187
> 
> > Also in my opinion as this "PCI CAM" does not set "enable" bit, there is
> > needed some other code which sets it via vendor-specific API.
> 
> What should we do for now? Do you need any help getting set up with
> this environment? I think we could look at adding the pcie ecam device
> to crosvm in parallel.

CAM is just a version of ECAM that only gives you access to the
classic PCI config space (register offsets < 256).  This has very
little to do with the classic "mode 1" and "mode 2" config space
access methods of the x86 PCI host bridges.  I don't think there is a
CAM standard at all, but some of the PCI host bridges found on PowerPC
and SPARC hardware implemented a straight mapping of PCI config space
into mmio space like that.

It is a little bit strange that crosvm implements CAM instead of ECAM,
but I guess they don't care about passthrough of arbitrary PCIe
devices.  And as long as all (emulated) PCIe devices only have
registers with offsets < 256, this will work just fine.

And yes, you should check that the register offset is limited to
0..255.