Re: [RFC PATCH v4 12/15] iommu/io-pgtable-arm-v7s: Implement arm_v7s_unmap_pages()

Re: [RFC PATCH v4 12/15] iommu/io-pgtable-arm-v7s: Implement arm_v7s_unmap_pages()

[Will Deacon]

On Wed, Apr 07, 2021 at 09:52:38PM -0700, Isaac J. Manjarres wrote:
> Implement the unmap_pages() callback for the ARM v7s io-pgtable
> format.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>  drivers/iommu/io-pgtable-arm-v7s.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index d4004bcf333a..5e203e03c352 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -710,15 +710,32 @@ static size_t __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
>  	return __arm_v7s_unmap(data, gather, iova, size, lvl + 1, ptep);
>  }
>  
> -static size_t arm_v7s_unmap(struct io_pgtable_ops *ops, unsigned long iova,
> -			    size_t size, struct iommu_iotlb_gather *gather)
> +static size_t arm_v7s_unmap_pages(struct io_pgtable_ops *ops, unsigned long iova,
> +				  size_t pgsize, size_t pgcount,
> +				  struct iommu_iotlb_gather *gather)
>  {
>  	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
> +	size_t unmapped = 0, ret;
>  
>  	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
>  		return 0;
>  
> -	return __arm_v7s_unmap(data, gather, iova, size, 1, data->pgd);
> +	while (pgcount--) {
> +		ret = __arm_v7s_unmap(data, gather, iova, pgsize, 1, data->pgd);
> +		if (!ret)
> +			break;
> +
> +		unmapped += pgsize;
> +		iova += pgsize;
> +	}
> +
> +	return unmapped;
> +}

Wait -- don't you need to hook this up somewhere (likewise for ->map_pages)?
How are you testing this?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v4 06/15] iommu: Split 'addr_merge' argument to iommu_pgsize() into separate parts

[Will Deacon]

On Wed, Apr 07, 2021 at 09:52:32PM -0700, Isaac J. Manjarres wrote:
> From: Will Deacon <will@kernel.org>
> 
> The 'addr_merge' parameter to iommu_pgsize() is a fabricated address
> intended to describe the alignment requirements to consider when
> choosing an appropriate page size. On the iommu_map() path, this address
> is the logical OR of the virtual and physical addresses.
> 
> Subsequent improvements to iommu_pgsize() will need to check the
> alignment of the virtual and physical components of 'addr_merge'
> independently, so pass them in as separate parameters and reconstruct
> 'addr_merge' locally.
> 
> No functional change.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>  drivers/iommu/iommu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bcd623862bf9..ab689611a03b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2357,12 +2357,13 @@ phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
>  }
>  EXPORT_SYMBOL_GPL(iommu_iova_to_phys);
>  
> -static size_t iommu_pgsize(struct iommu_domain *domain,
> -			   unsigned long addr_merge, size_t size)
> +static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
> +			   phys_addr_t paddr, size_t size)
>  {
>  	unsigned int pgsize_idx;
>  	unsigned long pgsizes;
>  	size_t pgsize;
> +	phys_addr_t addr_merge = paddr | iova;

^^^ this needs to be 'unsigned long' as it was before (otherwise using
GENMASK _is_ a problem).

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v4 09/15] iommu/io-pgtable-arm: Prepare PTE methods for handling multiple entries

[Will Deacon]

On Wed, Apr 07, 2021 at 09:52:35PM -0700, Isaac J. Manjarres wrote:
> The PTE methods currently operate on a single entry. In preparation
> for manipulating multiple PTEs in one map or unmap call, allow them
> to handle multiple PTEs.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/io-pgtable-arm.c | 78 +++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 87def58e79b5..ea66b10c04c4 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -232,20 +232,23 @@ static void __arm_lpae_free_pages(void *pages, size_t size,
>  	free_pages((unsigned long)pages, get_order(size));
>  }
>  
> -static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep,
> +static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep, int num_entries,
>  				struct io_pgtable_cfg *cfg)
>  {
>  	dma_sync_single_for_device(cfg->iommu_dev, __arm_lpae_dma_addr(ptep),
> -				   sizeof(*ptep), DMA_TO_DEVICE);
> +				   sizeof(*ptep) * num_entries, DMA_TO_DEVICE);
>  }

Have you tested this with CONFIG_DMA_API_DEBUG=y? I _think_ it should be
ok as long as we don't attempt to sync across a page boundary, but it would
be good to give it a spin just to check.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v4 09/15] iommu/io-pgtable-arm: Prepare PTE methods for handling multiple entries

[Christoph Hellwig]

On Thu, Apr 08, 2021 at 02:59:26PM +0100, Will Deacon wrote:
> > -static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep,
> > +static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep, int num_entries,
> >  				struct io_pgtable_cfg *cfg)
> >  {
> >  	dma_sync_single_for_device(cfg->iommu_dev, __arm_lpae_dma_addr(ptep),
> > -				   sizeof(*ptep), DMA_TO_DEVICE);
> > +				   sizeof(*ptep) * num_entries, DMA_TO_DEVICE);
> >  }
> 
> Have you tested this with CONFIG_DMA_API_DEBUG=y? I _think_ it should be
> ok as long as we don't attempt to sync across a page boundary, but it would
> be good to give it a spin just to check.

syncing over a page boundary is perfectly fine.  It just needs to say in
the bounds of the original mapping.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v4 09/15] iommu/io-pgtable-arm: Prepare PTE methods for handling multiple entries

[Will Deacon]

On Thu, Apr 08, 2021 at 03:02:30PM +0100, Christoph Hellwig wrote:
> On Thu, Apr 08, 2021 at 02:59:26PM +0100, Will Deacon wrote:
> > > -static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep,
> > > +static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep, int num_entries,
> > >  				struct io_pgtable_cfg *cfg)
> > >  {
> > >  	dma_sync_single_for_device(cfg->iommu_dev, __arm_lpae_dma_addr(ptep),
> > > -				   sizeof(*ptep), DMA_TO_DEVICE);
> > > +				   sizeof(*ptep) * num_entries, DMA_TO_DEVICE);
> > >  }
> > 
> > Have you tested this with CONFIG_DMA_API_DEBUG=y? I _think_ it should be
> > ok as long as we don't attempt to sync across a page boundary, but it would
> > be good to give it a spin just to check.
> 
> syncing over a page boundary is perfectly fine.  It just needs to say in
> the bounds of the original mapping.

Yes, you're right. I got the CPU page size mixed up with the IOMMU page
size, so I think we're good as the allocations here are made at IOMMU
page size granularity.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v4 10/15] iommu/io-pgtable-arm: Implement arm_lpae_unmap_pages()

[Will Deacon]

On Wed, Apr 07, 2021 at 09:52:36PM -0700, Isaac J. Manjarres wrote:
> Implement the unmap_pages() callback for the ARM LPAE io-pgtable
> format.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Will Deacon <will@kernel.org>
> ---
>  drivers/iommu/io-pgtable-arm.c | 70 ++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index ea66b10c04c4..6700685f81d4 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -253,8 +253,8 @@ static void __arm_lpae_set_pte(arm_lpae_iopte *ptep, arm_lpae_iopte pte,
>  
>  static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>  			       struct iommu_iotlb_gather *gather,
> -			       unsigned long iova, size_t size, int lvl,
> -			       arm_lpae_iopte *ptep);
> +			       unsigned long iova, size_t size, size_t pgcount,
> +			       int lvl, arm_lpae_iopte *ptep);
>  
>  static void __arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>  				phys_addr_t paddr, arm_lpae_iopte prot,
> @@ -298,7 +298,7 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>  			size_t sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
>  
>  			tblp = ptep - ARM_LPAE_LVL_IDX(iova, lvl, data);
> -			if (__arm_lpae_unmap(data, NULL, iova + i * sz, sz,
> +			if (__arm_lpae_unmap(data, NULL, iova + i * sz, sz, 1,
>  					     lvl, tblp) != sz) {
>  				WARN_ON(1);
>  				return -EINVAL;
> @@ -526,14 +526,14 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>  				       struct iommu_iotlb_gather *gather,
>  				       unsigned long iova, size_t size,
>  				       arm_lpae_iopte blk_pte, int lvl,
> -				       arm_lpae_iopte *ptep)
> +				       arm_lpae_iopte *ptep, size_t pgcount)
>  {
>  	struct io_pgtable_cfg *cfg = &data->iop.cfg;
>  	arm_lpae_iopte pte, *tablep;
>  	phys_addr_t blk_paddr;
>  	size_t tablesz = ARM_LPAE_GRANULE(data);
>  	size_t split_sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
> -	int i, unmap_idx = -1;
> +	int i, unmap_idx_start = -1, num_entries = 0, max_entries;
>  
>  	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
>  		return 0;
> @@ -542,15 +542,18 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>  	if (!tablep)
>  		return 0; /* Bytes unmapped */
>  
> -	if (size == split_sz)
> -		unmap_idx = ARM_LPAE_LVL_IDX(iova, lvl, data);
> +	if (size == split_sz) {
> +		unmap_idx_start = ARM_LPAE_LVL_IDX(iova, lvl, data);
> +		max_entries = (tablesz >> ilog2(sizeof(pte))) - unmap_idx_start;
> +		num_entries = min_t(int, pgcount, max_entries);
> +	}
>  
>  	blk_paddr = iopte_to_paddr(blk_pte, data);
>  	pte = iopte_prot(blk_pte);
>  
>  	for (i = 0; i < tablesz / sizeof(pte); i++, blk_paddr += split_sz) {

Given that we already have a 'tablesz / sizeof(pte)' expression here, I'd be
inclined to have either a local variable or a macro helper to get at the
ptes_per_table value that you also need to compute max_entries.

>  		/* Unmap! */
> -		if (i == unmap_idx)
> +		if (i >= unmap_idx_start && i < (unmap_idx_start + num_entries))
>  			continue;
>  
>  		__arm_lpae_init_pte(data, blk_paddr, pte, lvl, 1, &tablep[i]);
> @@ -568,38 +571,45 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>  			return 0;
>  
>  		tablep = iopte_deref(pte, data);
> -	} else if (unmap_idx >= 0) {
> -		io_pgtable_tlb_add_page(&data->iop, gather, iova, size);
> -		return size;
> +	} else if (unmap_idx_start >= 0) {
> +		for (i = 0; i < num_entries; i++)
> +			io_pgtable_tlb_add_page(&data->iop, gather, iova + i * size, size);

I suppose we could add a count paramater to the iotlb gather stuff in
future too, but for now this is fine as this series is already pretty big.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v4 12/15] iommu/io-pgtable-arm-v7s: Implement arm_v7s_unmap_pages()

[Will Deacon]

On Thu, Apr 08, 2021 at 07:19:29AM -0700, isaacm@codeaurora.org wrote:
> On 2021-04-08 06:58, Will Deacon wrote:
> > On Wed, Apr 07, 2021 at 09:52:38PM -0700, Isaac J. Manjarres wrote:
> > > Implement the unmap_pages() callback for the ARM v7s io-pgtable
> > > format.
> > > 
> > > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> > > ---
> > >  drivers/iommu/io-pgtable-arm-v7s.c | 23 ++++++++++++++++++++---
> > >  1 file changed, 20 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
> > > b/drivers/iommu/io-pgtable-arm-v7s.c
> > > index d4004bcf333a..5e203e03c352 100644
> > > --- a/drivers/iommu/io-pgtable-arm-v7s.c
> > > +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> > > @@ -710,15 +710,32 @@ static size_t __arm_v7s_unmap(struct
> > > arm_v7s_io_pgtable *data,
> > >  	return __arm_v7s_unmap(data, gather, iova, size, lvl + 1, ptep);
> > >  }
> > > 
> > > -static size_t arm_v7s_unmap(struct io_pgtable_ops *ops, unsigned
> > > long iova,
> > > -			    size_t size, struct iommu_iotlb_gather *gather)
> > > +static size_t arm_v7s_unmap_pages(struct io_pgtable_ops *ops,
> > > unsigned long iova,
> > > +				  size_t pgsize, size_t pgcount,
> > > +				  struct iommu_iotlb_gather *gather)
> > >  {
> > >  	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > > +	size_t unmapped = 0, ret;
> > > 
> > >  	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
> > >  		return 0;
> > > 
> > > -	return __arm_v7s_unmap(data, gather, iova, size, 1, data->pgd);
> > > +	while (pgcount--) {
> > > +		ret = __arm_v7s_unmap(data, gather, iova, pgsize, 1, data->pgd);
> > > +		if (!ret)
> > > +			break;
> > > +
> > > +		unmapped += pgsize;
> > > +		iova += pgsize;
> > > +	}
> > > +
> > > +	return unmapped;
> > > +}
> > 
> > Wait -- don't you need to hook this up somewhere (likewise for
> > ->map_pages)?
> Done. Likewise for map_pages(). I'm not sure how the compiler didn't catch
> this; I'm compile testing this, as I don't have hardware that uses the short
> descriptor format.

Damn, neither do I :/ My seattle has all the memory high up iirc.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel