Re: [PATCH kernel v9 15/32] powerpc/powernv/ioda/ioda2: Rework TCE invalidation in tce_build()/tce_free()

[David Gibson]

On Thu, Apr 30, 2015 at 12:58:12PM +1000, Alexey Kardashevskiy wrote:
> On 04/29/2015 01:18 PM, David Gibson wrote:
> >On Sat, Apr 25, 2015 at 10:14:39PM +1000, Alexey Kardashevskiy wrote:
> >>The pnv_pci_ioda_tce_invalidate() helper invalidates TCE cache. It is
> >>supposed to be called on IODA1/2 and not called on p5ioc2. It receives
> >>start and end host addresses of TCE table.
> >>
> >>IODA2 actually needs PCI addresses to invalidate the cache. Those
> >>can be calculated from host addresses but since we are going
> >>to implement multi-level TCE tables, calculating PCI address from
> >>a host address might get either tricky or ugly as TCE table remains flat
> >>on PCI bus but not in RAM.
> >>
> >>This moves pnv_pci_ioda_tce_invalidate() from generic pnv_tce_build/
> >>pnt_tce_free and defines IODA1/2-specific callbacks which call generic
> >>ones and do PHB-model-specific TCE cache invalidation. P5IOC2 keeps
> >>using generic callbacks as before.
> >>
> >>This changes pnv_pci_ioda2_tce_invalidate() to receives TCE index and
> >>number of pages which are PCI addresses shifted by IOMMU page shift.
> >>
> >>No change in behaviour is expected.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
> >>---
> >>Changes:
> >>v9:
> >>* removed confusing comment from commit log about unintentional calling of
> >>pnv_pci_ioda_tce_invalidate()
> >>* moved mechanical changes away to "powerpc/iommu: Move tce_xxx callbacks from ppc_md to iommu_table"
> >>* fixed bug with broken invalidation in pnv_pci_ioda2_tce_invalidate -
> >>@index includes @tbl->it_offset but old code added it anyway which later broke
> >>DDW
> >>---
> >>  arch/powerpc/platforms/powernv/pci-ioda.c | 86 +++++++++++++++++++++----------
> >>  arch/powerpc/platforms/powernv/pci.c      | 17 ++----
> >>  2 files changed, 64 insertions(+), 39 deletions(-)
> >>
> >>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>index 718d5cc..f070c44 100644
> >>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>@@ -1665,18 +1665,20 @@ static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe,
> >>  	}
> >>  }
> >>
> >>-static void pnv_pci_ioda1_tce_invalidate(struct pnv_ioda_pe *pe,
> >>-					 struct iommu_table *tbl,
> >>-					 __be64 *startp, __be64 *endp, bool rm)
> >>+static void pnv_pci_ioda1_tce_invalidate(struct iommu_table *tbl,
> >>+		unsigned long index, unsigned long npages, bool rm)
> >>  {
> >>+	struct pnv_ioda_pe *pe = container_of(tbl->it_table_group,
> >>+			struct pnv_ioda_pe, table_group);
> >>  	__be64 __iomem *invalidate = rm ?
> >>  		(__be64 __iomem *)pe->tce_inval_reg_phys :
> >>  		(__be64 __iomem *)tbl->it_index;
> >>  	unsigned long start, end, inc;
> >>  	const unsigned shift = tbl->it_page_shift;
> >>
> >>-	start = __pa(startp);
> >>-	end = __pa(endp);
> >>+	start = __pa((__be64 *)tbl->it_base + index - tbl->it_offset);
> >>+	end = __pa((__be64 *)tbl->it_base + index - tbl->it_offset +
> >>+			npages - 1);
> >
> >This doesn't look right.  The arguments to __pa don't appear to be
> >addresses (since index and if_offset are in units of (TCE) pages, not
> >bytes).
> 
> 
> tbl->it_base is an address and it is casted to __be64* which means:
> 
> (char*)tbl->it_base + (index - tbl->it_offset)*sizeof(__be64).
> 
> Which seems to be correct (I just removed extra braces compared to the old
> code), no?

Ah, yes, I'm just forgetting my C pointer arithmetic rules.

Reviewed-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: pgpspaNTUwO7L.pgp
Description: PGP signature