Re: [PATCH] PCI: tegra: limit MSI target address to 32-bit

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Fri, Nov 10, 2017 at 12:11:05AM +0530, Vidya Sagar wrote:

[...]

> >>>>--- a/drivers/pci/host/pci-tegra.c
> >>>>+++ b/drivers/pci/host/pci-tegra.c
> >>>>@@ -1531,7 +1531,7 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> >>>>  	}
> >>>>  	/* setup AFI/FPCI range */
> >>>>-	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> >>>>+	msi->pages = __get_free_pages(GFP_DMA, 0);
> >>>>  	msi->phys = virt_to_phys((void *)msi->pages);
> >>>Should this be GFP_DMA32?  See the comment above the GFP_DMA
> >>>definition.
> >>looking at the comments for both GFP_DMA32 and GFP_DMA, I thought GFP_DMA32
> >>is the correct one to use, but, even with that I got >32-bit addresses.
> >>GFP_DMA always gives addresses in <4GB boundary (i.e. 32-bit).
> >>I didn't dig into it to find out why is this the case.
> >This sounds worth looking into (but maybe we don't need the
> >__get_free_pages() at all; see below).  Maybe there's some underlying
> >bug.  My laptop shows this, which looks like it might be related:
> >
> >   Zone ranges:
> >     DMA      [mem 0x0000000000001000-0x0000000000ffffff]
> >     DMA32    [mem 0x0000000001000000-0x00000000ffffffff]
> >     Normal   [mem 0x0000000100000000-0x00000004217fffff]
> >     Device   empty
> >
> >What does your machine show?
> I see following in my linux box
>  Zone ranges:
>    DMA      [mem 0x0000000000001000-0x0000000000ffffff]
>    DMA32    [mem 0x0000000001000000-0x00000000ffffffff]
>    Normal   [mem 0x0000000100000000-0x000000106effffff]
>    Device   empty
> 
> and following in my T210 Tegra platform
> Zone ranges:
>   DMA      [mem 0x0000000080000000-0x00000000ffffffff]
>   Normal   [mem 0x0000000100000000-0x000000017fffffff]
> 
> >
> >>>Should we be using virt_to_phys() here?  Where exactly is the
> >>>result ("msi->phys") used, i.e., what bus will that address appear
> >>>on?  If it appears on the PCI side, this should probably use
> >>>something like pcibios_resource_to_bus().
> >>This address is written to two places.  First, into host's internal
> >>register to let it know that when an incoming memory write comes
> >>with this address, raise an MSI interrupt instead of forwarding it
> >>to memory subsystem.  Second, into 'Message Address' field of
> >>'Message Address Register for MSI' register in end point's
> >>configuration space (this is done by MSI framework) for end point to
> >>know which address to be used to generate MSI interrupt.
> >Hmmm, ISTR some past discussion about this.  Here's a little: [1, 2].
> >And this commit [3] sounds like it describes a similar hardware
> >situation with Tegra where the host bridge intercepts the MSI target
> >address, so writes to it never reach system memory.  That means that
> >Tegra doesn't need to allocate system memory at all.
> >
> >Is your system similar?  Can you just statically allocate a little bus
> >address space, use that for the MSI target address, and skip the
> >__get_free_pages()?
> It is the same system where MSI memory writes don't really make it to
> system memory. But, the addresses mentioned in that patch don't work.
> we are working with hardware folks on understanding the issue better.
> Meanwhile, using GFP_DMA is giving consistent results and thought this
> can be used as a stop gap solution before we figure out a better bus
> address to be used as MSI target address.

Vidya,

I will mark this as superseded - by Thierry's patch:

https://patchwork.ozlabs.org/patch/848569/

even though we have not reached a real conclusion yet
on that patch - we should take this discussion from the
thread above.

Thank you,
Lorenzo

> >>>Do rcar_pcie_enable_msi() and xilinx_pcie_enable_msi() have a
> >>>similar problem?  They both use GFP_KERNEL, then virt_to_phys(),
> >>>then write the result of virt_to_phys() using a 32-bit register
> >>>write.
> >>Well, if those systems deal with 64-bit addresses and when an end
> >>point is connected which supports only 32-bit MSI addresses, this
> >>problem will surface when __get_free_pages() returns an address that
> >>translates to a >32-bit address after virt_to_phys() call on it.
> >I'd like to hear from the R-Car and Xilinx folks about (1) whether
> >there's a potential issue with truncating a 64-bit address, and
> >(2) whether that hardware works like Tegra, where the MSI write never
> >reaches memory so we don't actually need to allocate a page.
> >
> >If all we need is to allocate a little bus address space for the MSI
> >target, I'd like to figure out a better way to do that than
> >__get_free_pages().  The current code seems a little buggy, and
> >it's getting propagated through several drivers.
> >
> >>>>  	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
> >[1] https://lkml.kernel.org/r/20170824134451.GA31858@bhelgaas-glaptop.roam.corp.google.com
> >[2] https://lkml.kernel.org/r/86efs3wesi.fsf@arm.com
> >[3] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d7bd554f27c9
>