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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150430041642.GC15238@voom.redhat.com>
Date:	Thu, 30 Apr 2015 14:16:42 +1000
From:	David Gibson <david@...son.dropbear.id.au>
To:	Alexey Kardashevskiy <aik@...abs.ru>
Cc:	linuxppc-dev@...ts.ozlabs.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	Alex Williamson <alex.williamson@...hat.com>,
	Gavin Shan <gwshan@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH kernel v9 15/32] powerpc/powernv/ioda/ioda2: Rework TCE
 invalidation in tce_build()/tce_free()

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@...abs.ru>
> >>---
> >>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@...son.dropbear.id.au>

-- 
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

Content of type "application/pgp-signature" skipped