[RFC PATCH] powerpc: fsl_pci: fix inbound ATMU entries for systems with >4G RAM

[Scott Wood]

On 08/26/2016 12:26 AM, Tillmann Heidsieck wrote:
> Hello Scott,
> 
> thanks for the fast reply!
> 
> On 2016-08-24 23:39, Scott Wood wrote:
> [..]
>>
>> The second inbound window is at 256G, and it maps all of RAM.  Note 
>> that
>> for accesses in this window, the PCI address is different from the host
>> address.
> 
> I probably really misunderstand the manual here ...
> 
> 1 << 40 = 0x100_00000000 right? So if I put
> 
> (1 << 40) >> 44 = 0 in PEXIWBEAR and
> (1 << 40) >> 12 = 0x10000000 in PEXIWBAR
> 
> the window base (extended) address would be (1 << 40) right? And at the
> risk of showing my complete lack of skill doing basic arithmetic ...
> isn't (1 << 40) = 128GB? So what am I missing here?

Sorry, I meant 1 TiB, not 256 GiB.  1 << 40 = 1 TiB.

> I also read that the maximum allowed window size for PCIe inbound 
> windows
> is 64G this would result in the need for more ATMU entries in case of 
>  >68G
> system memory (the question is whether this needs to be fixed because
> maybe nobody wants to build such a computer).

Hmm, it does say that.  This code was written when the physical address
size was 36 bits rather than 40.  I wonder if there's a real 64 GiB
limitation or if the manual just wasn't updated when the physical
address size grew... though in any case we should follow what the manual
says unless it's confirmed otherwise.

>> By trying to identity-map everything you also break the assumption that
>> we don't need swiotlb to avoid the PEXCSRBAR region on PCI devices
>> capable of generating 64-bit addresses.
> 
> Can you point me to where I might read up on this? I know far to little
> about all of this, that's for sure.

PEXCSRBAR (called PCICSRBAR in the Linux driver) is an inbound window
intended for MSIs, configured via a config space BAR rather than the
MMIO ATMU registers.  It only accepts 32-bit PCI addresses and can't be
disabled or pointed at anything but CCSR.  Any RAM that aliases the
PEXCSRBAR PCI address can't be DMAed to without either using swiotlb or
a non-identity mapping.

>>> Tested on a T4240 with 12G of RAM and an AMD E6760 PCIe card working 
>>> with
>>> the in-tree radeon driver.
>>
>> I have no problem using an e1000e card on a t4240 with 24 GiB RAM.
>>
>> Looking at the radeon driver I see that there's a possibility of a dma
>> mask that is exactly 40 bits.  I think there's an off-by-one error in
>> fsl_pci_dma_set_mask().  If you change "dma_mask >=
>> DMA_BIT_MASK(MAX_PHYS_ADDR_BITS)" to "dma_mask >
>> DMA_BIT_MASK(MAX_PHYS_ADDR_BITS)", does that make radeon work without
>> this patch?
> 
> This works in the sense that the radeon driver uses only 32bit dma 
> addresses
> after applying it.
> I don't think that is what was intended since the card clearly works 
> which
> higher addresses.

Right, step one is to fix the bug.  Step two is to make things faster. :-)

For the second step I suggest setting the high window's address/size
based on the amount of RAM present (rounded up to a power of 2) rather
than trying to map the entire 40-bit address space.  Thus if you have 12
GiB RAM, you'd get a 16 GiB window at PCI address 16 GiB, and any PCI
device with a DMA mask of at least 35 bits could use direct DMA to the
high window.  I'll put a patch together.

>>
>> This change is unrelated.
> 
> yeah sorry for sneaking that one in here, are you interested in this 
> change?
> It cleans up the code a little bit by using existing functions. I think 
> it
> makes the intend more clear but that's up for interpretation ;-)

Sure.  If you resend it, please CC me at oss at buserror.net rather than
the NXP address.

BTW, we can probably just drop that "Setting PCI inbound window greater
than memory size" message.  We routinely silently map beyond the end of
RAM with the high mapping...

>> BTW, for some reason your patch is not showing up in Patchwork.
> 
> Are there some known pitfalls when sending patches to Patchwork?

It's not the first time I've seen certain people's patches not show up
there, but I don't know what the root cause is.

-Scott