Re: [RFC PATCH v3 02/12] iommu: Add an unmap_pages() op for IOMMU drivers

Re: [RFC PATCH v3 02/12] iommu: Add an unmap_pages() op for IOMMU drivers

[Lu Baolu]

On 4/6/21 3:11 AM, Isaac J. Manjarres wrote:
> Add a callback for IOMMU drivers to provide a path for the
> IOMMU framework to call into an IOMMU driver, which can call
> into the io-pgtable code, to unmap a virtually contiguous
> range of pages of the same size.
> 
> For IOMMU drivers that do not specify an unmap_pages() callback,
> the existing logic of unmapping memory one page block at a time
> will be used.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   include/linux/iommu.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5e7fe519430a..9cf81242581a 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -193,6 +193,7 @@ struct iommu_iotlb_gather {
>    * @detach_dev: detach device from an iommu domain
>    * @map: map a physically contiguous memory region to an iommu domain
>    * @unmap: unmap a physically contiguous memory region from an iommu domain
> + * @unmap_pages: unmap a number of pages of the same size from an iommu domain
>    * @flush_iotlb_all: Synchronously flush all hardware TLBs for this domain
>    * @iotlb_sync_map: Sync mappings created recently using @map to the hardware
>    * @iotlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
> @@ -245,6 +246,9 @@ struct iommu_ops {
>   		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
>   	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>   		     size_t size, struct iommu_iotlb_gather *iotlb_gather);
> +	size_t (*unmap_pages)(struct iommu_domain *domain, unsigned long iova,
> +			      size_t pgsize, size_t pgcount,
> +			      struct iommu_iotlb_gather *iotlb_gather);
>   	void (*flush_iotlb_all)(struct iommu_domain *domain);
>   	void (*iotlb_sync_map)(struct iommu_domain *domain, unsigned long iova,
>   			       size_t size);
> 

This looks good to me.

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

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

Re: [RFC PATCH v3 05/12] iommu: Use bitmap to calculate page size in iommu_pgsize()

[Will Deacon]

On Mon, Apr 05, 2021 at 12:11:05PM -0700, Isaac J. Manjarres wrote:
> From: Will Deacon <will@kernel.org>
> 
> Avoid the potential for shifting values by amounts greater than the
> width of their type by using a bitmap to compute page size in
> iommu_pgsize().
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>  drivers/iommu/iommu.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d0b0a15dba84..9006397b6604 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> +#include <linux/bits.h>
>  #include <linux/bug.h>
>  #include <linux/types.h>
>  #include <linux/init.h>
> @@ -2360,30 +2361,22 @@ static size_t iommu_pgsize(struct iommu_domain *domain,
>  			   unsigned long addr_merge, size_t size)
>  {
>  	unsigned int pgsize_idx;
> +	unsigned long pgsizes;
>  	size_t pgsize;
>  
> -	/* Max page size that still fits into 'size' */
> -	pgsize_idx = __fls(size);
> +	/* Page sizes supported by the hardware and small enough for @size */
> +	pgsizes = domain->pgsize_bitmap & GENMASK_ULL(__fls(size), 0);

See my comments on the other thread, but I don't think it's necessary to use
the _ULL versions everywhere.

Will

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

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

[Will Deacon]

On Mon, Apr 05, 2021 at 12:11:06PM -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 9006397b6604..a3bbf7e310b0 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;

Huh, so this was 'unsigned long' before and, given that the pgsize_bitmap
on the domain is also unsigned long, then I think that's fine. So using
that would mean you don't need GENMASK_ULL for this guy either.

Will

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

Re: [RFC PATCH v3 03/12] iommu/io-pgtable: Introduce map_pages() as a page table op

[Will Deacon]

On Mon, Apr 05, 2021 at 12:11:03PM -0700, Isaac J. Manjarres wrote:
> Mapping memory into io-pgtables follows the same semantics
> that unmapping memory used to follow (i.e. a buffer will be
> mapped one page block per call to the io-pgtable code). This
> means that it can be optimized in the same way that unmapping
> memory was, so add a map_pages() callback to the io-pgtable
> ops structure, so that a range of pages of the same size
> can be mapped within the same call.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Will Deacon <will@kernel.org>
> ---
>  include/linux/io-pgtable.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index 2ed0c057d9e7..019149b204b8 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -143,6 +143,7 @@ struct io_pgtable_cfg {
>   * struct io_pgtable_ops - Page table manipulation API for IOMMU drivers.
>   *
>   * @map:          Map a physically contiguous memory region.
> + * @map_pages:    Map a physically contiguous range of pages of the same size.
>   * @unmap:        Unmap a physically contiguous memory region.
>   * @unmap_pages:  Unmap a range of virtually contiguous pages of the same size.
>   * @iova_to_phys: Translate iova to physical address.
> @@ -153,6 +154,9 @@ struct io_pgtable_cfg {
>  struct io_pgtable_ops {
>  	int (*map)(struct io_pgtable_ops *ops, unsigned long iova,
>  		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
> +	int (*map_pages)(struct io_pgtable_ops *ops, unsigned long iova,
> +			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			 int prot, gfp_t gfp, size_t *mapped);

How about returning 'size_t' and using IS_ERR_VALUE() instead of adding
the extra 'mapped' argument (i.e. return the size of the region mapped
or an error code)? I don't think we realistically need to care about map
sizes that overlap with the error region.

Will

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

Re: [RFC PATCH v3 03/12] iommu/io-pgtable: Introduce map_pages() as a page table op

[Will Deacon]

On Tue, Apr 06, 2021 at 02:07:41PM -0700, isaacm@codeaurora.org wrote:
> On 2021-04-06 04:57, Will Deacon wrote:
> > On Mon, Apr 05, 2021 at 12:11:03PM -0700, Isaac J. Manjarres wrote:
> > > Mapping memory into io-pgtables follows the same semantics
> > > that unmapping memory used to follow (i.e. a buffer will be
> > > mapped one page block per call to the io-pgtable code). This
> > > means that it can be optimized in the same way that unmapping
> > > memory was, so add a map_pages() callback to the io-pgtable
> > > ops structure, so that a range of pages of the same size
> > > can be mapped within the same call.
> > > 
> > > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> > > Suggested-by: Will Deacon <will@kernel.org>
> > > ---
> > >  include/linux/io-pgtable.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> > > index 2ed0c057d9e7..019149b204b8 100644
> > > --- a/include/linux/io-pgtable.h
> > > +++ b/include/linux/io-pgtable.h
> > > @@ -143,6 +143,7 @@ struct io_pgtable_cfg {
> > >   * struct io_pgtable_ops - Page table manipulation API for IOMMU
> > > drivers.
> > >   *
> > >   * @map:          Map a physically contiguous memory region.
> > > + * @map_pages:    Map a physically contiguous range of pages of the
> > > same size.
> > >   * @unmap:        Unmap a physically contiguous memory region.
> > >   * @unmap_pages:  Unmap a range of virtually contiguous pages of
> > > the same size.
> > >   * @iova_to_phys: Translate iova to physical address.
> > > @@ -153,6 +154,9 @@ struct io_pgtable_cfg {
> > >  struct io_pgtable_ops {
> > >  	int (*map)(struct io_pgtable_ops *ops, unsigned long iova,
> > >  		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
> > > +	int (*map_pages)(struct io_pgtable_ops *ops, unsigned long iova,
> > > +			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
> > > +			 int prot, gfp_t gfp, size_t *mapped);
> > 
> > How about returning 'size_t' and using IS_ERR_VALUE() instead of adding
> > the extra 'mapped' argument (i.e. return the size of the region mapped
> > or an error code)? I don't think we realistically need to care about map
> > sizes that overlap with the error region.
> > 
> I'd given that a shot before, but the problem that I kept running into was
> that
> in case of an error, if I return an error code, I don't know how much memory
> was mapped, so that I can invoke iommu_unmap from __iommu_map with that size
> to
> undo the partial mappings from a map_pages() call.

Ah yes, sorry, I see it now. So keep this as you've got it. Pushing the
cleanup path deeper doesn't feel like the right thing to do.

Will

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

Re: [RFC PATCH v3 04/12] iommu: Add a map_pages() op for IOMMU drivers

[Lu Baolu]

On 4/6/21 3:11 AM, Isaac J. Manjarres wrote:
> Add a callback for IOMMU drivers to provide a path for the
> IOMMU framework to call into an IOMMU driver, which can
> call into the io-pgtable code, to map a physically contiguous
> rnage of pages of the same size.
> 
> For IOMMU drivers that do not specify a map_pages() callback,
> the existing logic of mapping memory one page block at a time
> will be used.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Will Deacon <will@kernel.org>
> ---
>   include/linux/iommu.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9cf81242581a..528d6a58479e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -192,6 +192,8 @@ struct iommu_iotlb_gather {
>    * @attach_dev: attach device to an iommu domain
>    * @detach_dev: detach device from an iommu domain
>    * @map: map a physically contiguous memory region to an iommu domain
> + * @map_pages: map a physically contiguous set of pages of the same size to
> + *             an iommu domain.
>    * @unmap: unmap a physically contiguous memory region from an iommu domain
>    * @unmap_pages: unmap a number of pages of the same size from an iommu domain
>    * @flush_iotlb_all: Synchronously flush all hardware TLBs for this domain
> @@ -244,6 +246,9 @@ struct iommu_ops {
>   	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>   	int (*map)(struct iommu_domain *domain, unsigned long iova,
>   		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
> +	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
> +			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			 int prot, gfp_t gfp, size_t *mapped);
>   	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>   		     size_t size, struct iommu_iotlb_gather *iotlb_gather);
>   	size_t (*unmap_pages)(struct iommu_domain *domain, unsigned long iova,
> 

This looks good to me.

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

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

Re: [RFC PATCH v3 04/12] iommu: Add a map_pages() op for IOMMU drivers

[Will Deacon]

On Mon, Apr 05, 2021 at 12:11:04PM -0700, Isaac J. Manjarres wrote:
> Add a callback for IOMMU drivers to provide a path for the
> IOMMU framework to call into an IOMMU driver, which can
> call into the io-pgtable code, to map a physically contiguous
> rnage of pages of the same size.
> 
> For IOMMU drivers that do not specify a map_pages() callback,
> the existing logic of mapping memory one page block at a time
> will be used.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Will Deacon <will@kernel.org>
> ---
>  include/linux/iommu.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9cf81242581a..528d6a58479e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -192,6 +192,8 @@ struct iommu_iotlb_gather {
>   * @attach_dev: attach device to an iommu domain
>   * @detach_dev: detach device from an iommu domain
>   * @map: map a physically contiguous memory region to an iommu domain
> + * @map_pages: map a physically contiguous set of pages of the same size to
> + *             an iommu domain.
>   * @unmap: unmap a physically contiguous memory region from an iommu domain
>   * @unmap_pages: unmap a number of pages of the same size from an iommu domain
>   * @flush_iotlb_all: Synchronously flush all hardware TLBs for this domain
> @@ -244,6 +246,9 @@ struct iommu_ops {
>  	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>  	int (*map)(struct iommu_domain *domain, unsigned long iova,
>  		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
> +	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
> +			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			 int prot, gfp_t gfp, size_t *mapped);

(same comment as for the io-pgtable callback).

Will

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

Re: [RFC PATCH v3 09/12] iommu/io-pgtable-arm: Implement arm_lpae_unmap_pages()

[Will Deacon]

On Mon, Apr 05, 2021 at 12:11:09PM -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 | 124 +++++++++++++++++++++++++++------
>  1 file changed, 104 insertions(+), 20 deletions(-)

[...]

> +static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iova,
> +				   size_t pgsize, size_t pgcount,
> +				   struct iommu_iotlb_gather *gather)
> +{
> +	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> +	arm_lpae_iopte *ptep = data->pgd;
> +	long iaext = (s64)iova >> cfg->ias;
> +	size_t unmapped = 0, unmapped_page;
> +	int last_lvl;
> +	size_t table_size, pages, tbl_offset, max_entries;
> +
> +	if (WARN_ON(!pgsize || (pgsize & cfg->pgsize_bitmap) != pgsize || !pgcount))
> +		return 0;
> +
> +	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
> +		iaext = ~iaext;
> +	if (WARN_ON(iaext))
> +		return 0;
> +
> +	/*
> +	 * Calculating the page table size here helps avoid situations where
> +	 * a page range that is being unmapped may be mapped at the same level
> +	 * but not mapped by the same tables. Allowing such a scenario to
> +	 * occur can complicate the logic in arm_lpae_split_blk_unmap().
> +	 */
> +	last_lvl = ARM_LPAE_BLOCK_SIZE_LVL(pgsize, data);
> +
> +	if (last_lvl == data->start_level)
> +		table_size = ARM_LPAE_PGD_SIZE(data);
> +	else
> +		table_size = ARM_LPAE_GRANULE(data);
> +
> +	max_entries = table_size / sizeof(*ptep);
> +
> +	while (pgcount) {
> +		tbl_offset = ARM_LPAE_LVL_IDX(iova, last_lvl, data);
> +		pages = min_t(size_t, pgcount, max_entries - tbl_offset);
> +		unmapped_page = __arm_lpae_unmap(data, gather, iova, pgsize,
> +						 pages, data->start_level,
> +						 ptep);
> +		if (!unmapped_page)
> +			break;
> +
> +		unmapped += unmapped_page;
> +		iova += unmapped_page;
> +		pgcount -= pages;
> +	}

Robin has some comments on the first version of this patch, and I don't think you
addressed them:

https://lore.kernel.org/r/b93fa0b1-e2a4-1aad-8b88-4d0dfecdfef7@arm.com

I'm inclined to agree that iterating here doesn't make a lot of sense --
we've already come back out of the page-table walk, so I think we should
just return to the caller (who is well prepared to handle a partial unmap).
Same for the map side of things.

If we get numbers showing that this is causing a performance issue, then
we should rework the page-table code to handle this at the lower level
(because I doubt the loop that you have is really worse than returning to
the caller anyway).

Will

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

Re: [RFC PATCH v3 09/12] iommu/io-pgtable-arm: Implement arm_lpae_unmap_pages()

[Will Deacon]

On Tue, Apr 06, 2021 at 02:02:26PM -0700, isaacm@codeaurora.org wrote:
> On 2021-04-06 05:15, Will Deacon wrote:
> > On Mon, Apr 05, 2021 at 12:11:09PM -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 | 124
> > > +++++++++++++++++++++++++++------
> > >  1 file changed, 104 insertions(+), 20 deletions(-)
> > 
> > [...]
> > 
> > > +static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops,
> > > unsigned long iova,
> > > +				   size_t pgsize, size_t pgcount,
> > > +				   struct iommu_iotlb_gather *gather)
> > > +{
> > > +	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > > +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> > > +	arm_lpae_iopte *ptep = data->pgd;
> > > +	long iaext = (s64)iova >> cfg->ias;
> > > +	size_t unmapped = 0, unmapped_page;
> > > +	int last_lvl;
> > > +	size_t table_size, pages, tbl_offset, max_entries;
> > > +
> > > +	if (WARN_ON(!pgsize || (pgsize & cfg->pgsize_bitmap) != pgsize ||
> > > !pgcount))
> > > +		return 0;
> > > +
> > > +	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
> > > +		iaext = ~iaext;
> > > +	if (WARN_ON(iaext))
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * Calculating the page table size here helps avoid situations where
> > > +	 * a page range that is being unmapped may be mapped at the same
> > > level
> > > +	 * but not mapped by the same tables. Allowing such a scenario to
> > > +	 * occur can complicate the logic in arm_lpae_split_blk_unmap().
> > > +	 */
> > > +	last_lvl = ARM_LPAE_BLOCK_SIZE_LVL(pgsize, data);
> > > +
> > > +	if (last_lvl == data->start_level)
> > > +		table_size = ARM_LPAE_PGD_SIZE(data);
> > > +	else
> > > +		table_size = ARM_LPAE_GRANULE(data);
> > > +
> > > +	max_entries = table_size / sizeof(*ptep);
> > > +
> > > +	while (pgcount) {
> > > +		tbl_offset = ARM_LPAE_LVL_IDX(iova, last_lvl, data);
> > > +		pages = min_t(size_t, pgcount, max_entries - tbl_offset);
> > > +		unmapped_page = __arm_lpae_unmap(data, gather, iova, pgsize,
> > > +						 pages, data->start_level,
> > > +						 ptep);
> > > +		if (!unmapped_page)
> > > +			break;
> > > +
> > > +		unmapped += unmapped_page;
> > > +		iova += unmapped_page;
> > > +		pgcount -= pages;
> > > +	}
> > 
> > Robin has some comments on the first version of this patch, and I
> > don't think you
> > addressed them:
> > 
> > https://lore.kernel.org/r/b93fa0b1-e2a4-1aad-8b88-4d0dfecdfef7@arm.com
> > 
> > I'm inclined to agree that iterating here doesn't make a lot of sense --
> > we've already come back out of the page-table walk, so I think we should
> > just return to the caller (who is well prepared to handle a partial
> > unmap).
> > Same for the map side of things.
> > 
> > If we get numbers showing that this is causing a performance issue, then
> > we should rework the page-table code to handle this at the lower level
> > (because I doubt the loop that you have is really worse than returning
> > to
> > the caller anyway).
> > 
> Sorry, I seem to have overlooked those comments.
> 
> I will go ahead and address them. I think it might be ideal to try to do
> as much work as possible in the io-pgtable level, so as to minimize the
> number
> of indirect calls incurred by jumping back and forth between iommu fwk,
> iommu
> driver, and io-pgtable code.
> 
> Perhaps we can try something like how the linear mappings are created on
> arm64 i.e.
> on the previous level, we can determine how many pages can be unmapped in
> one page table
> in one iteration, and on the subsequent iterations, we can tackle another
> page table at
> the lower level. Looking at the code, it doesn't seem too difficult to add
> this in. Thoughts?

I don't object to getting there eventually, but as an initial step I think
returning back to the caller is perfectly reasonable and will probably make
the patches easier to review. In other words, implement the simple (correct)
thing first, and then have subsequent patches to improve it.

Will

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

Re: [RFC PATCH v3 11/12] iommu/arm-smmu: Implement the unmap_pages() IOMMU driver callback

[Will Deacon]

On Mon, Apr 05, 2021 at 12:11:11PM -0700, Isaac J. Manjarres wrote:
> Implement the unmap_pages() callback for the ARM SMMU driver
> to allow calls from iommu_unmap to unmap multiple pages of
> the same size in one call.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Suggested-by: Will Deacon <will@kernel.org>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d8c6bfde6a61..f29f1fb109f8 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1225,6 +1225,24 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>  	return ret;
>  }
>  
> +static size_t arm_smmu_unmap_pages(struct iommu_domain *domain, unsigned long iova,
> +				   size_t pgsize, size_t pgcount,
> +				   struct iommu_iotlb_gather *iotlb_gather)
> +{
> +	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> +	struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
> +	size_t ret;
> +
> +	if (!ops)
> +		return 0;
> +
> +	arm_smmu_rpm_get(smmu);
> +	ret = ops->unmap_pages(ops, iova, pgsize, pgcount, iotlb_gather);
> +	arm_smmu_rpm_put(smmu);
> +
> +	return ret;
> +}

Doesn't this go wrong if we're using the short-descriptor page-table
format? (same for the next patch)

Will

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