Re: [PATCH] iommu: io-pgtable: Support non-coherent page tables

[Will Deacon]

On Mon, Jun 24, 2019 at 12:53:49PM +0100, Will Deacon wrote:
> On Tue, Jun 18, 2019 at 06:39:33PM +0100, Will Deacon wrote:
> > On Wed, May 15, 2019 at 04:32:34PM -0700, Bjorn Andersson wrote:
> > > Describe the memory related to page table walks as non-cachable for iommu
> > > instances that are not DMA coherent.
> > > 
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > ---
> > >  drivers/iommu/io-pgtable-arm.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > > index 4e21efbc4459..68ff22ffd2cb 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -803,9 +803,15 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
> > >  		return NULL;
> > >  
> > >  	/* TCR */
> > > -	reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
> > > -	      (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) |
> > > -	      (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
> > > +	if (cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA) {
> > > +		reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
> > > +		      (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) |
> > > +		      (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
> > > +	} else {
> > > +		reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
> > 
> > Nit: this should be outer-shareable (ARM_LPAE_TCR_SH_OS).
> > 
> > > +		      (ARM_LPAE_TCR_RGN_NC << ARM_LPAE_TCR_IRGN0_SHIFT) |
> > > +		      (ARM_LPAE_TCR_RGN_NC << ARM_LPAE_TCR_ORGN0_SHIFT);
> > > +	}
> > 
> > Should we also be doing something similar for the short-descriptor code
> > in io-pgtable-arm-v7s.c? Looks like you just need to use ARM_V7S_RGN_NC
> > instead of ARM_V7S_RGN_WBWA when initialising ttbr0 for non-coherent
> > SMMUs.
> 
> Do you plan to respin this? I'll need it this week if you're shooting for
> 5.3.

I started having a crack at this myself, but in doing so I realised that
using IO_PGTABLE_QUIRK_NO_DMA for this isn't quite right based on its
current description. When that flag is set, we can rely on the page-table walker
being coherent, but I don't think it works the other way around: you can't
rely on the flag being clear meaning that the page-table walker is not
coherent.

Ideally, we'd use something like is_device_dma_coherent, but that's a Xen
thing and it doesn't look reliable for the IOMMU.

Looking at the users of io-pgtable, we have:

panfrost_mmu.c	- I can't see where the page-table even gets installed...
arm-smmu.c	- IO_PGTABLE_QUIRK_NO_DMA is reliable
arm-smmu-v3.c	- IO_PGTABLE_QUIRK_NO_DMA is reliable
ipmmu-vmsa.c	- Always sets TTBCR as cacheable (ignores tcr)
msm_iommu.c	- Always non-coherent
mtk_iommu.c	- Ignores tcr
qcom_iommu.c	- Always non-coherent

so we could go ahead and change IO_PGTABLE_QUIRK_NO_DMA to do what you want
without breaking any drivers. In any case, the driver is free to override
the control register if it knows better, as seems to be the case for some
of these already.

See patch below. I'll rework your patch on top.

Will

--->8