[RFC PATCH v6 0/5] treewide: improve R-Car SDHI performance

[RFC PATCH v6 0/5] treewide: improve R-Car SDHI performance

[Yoshihiro Shimoda]

This patch series is based on iommu.git / next branch.

Since SDHI host internal DMAC of the R-Car Gen3 cannot handle two or
more segments, the performance rate (especially, eMMC HS400 reading)
is not good. However, if IOMMU is enabled on the DMAC, since IOMMU will
map multiple scatter gather buffers as one contignous iova, the DMAC can
handle the iova as well and then the performance rate is possible to
improve. In fact, I have measured the performance by using bonnie++,
"Sequential Input - block" rate was improved on r8a7795.

To achieve this, this patch series modifies IOMMU and Block subsystem
at first. Since I'd like to get any feedback from each subsystem whether
this way is acceptable for upstream, I submit it to treewide with RFC.

Changes from v5:
 - Almost all patches are new code.
 - [4/5 for MMC] This is a refactor patch so that I don't add any
   {Tested,Reviewed}-by tags.
 - [5/5 for MMC] Modify MMC subsystem to use bigger segments instead of
   the renesas_sdhi driver.
 - [5/5 for MMC] Use BLK_MAX_SEGMENTS (128) instead of local value
   SDHI_MAX_SEGS_IN_IOMMU (512). Even if we use BLK_MAX_SEGMENTS,
   the performance is still good.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=127511

Changes from v4:
 - [DMA MAPPING] Add a new device_dma_parameters for iova contiguous.
 - [IOMMU] Add a new capable for "merging" segments.
 - [IOMMU] Add a capable ops into the ipmmu-vmsa driver.
 - [MMC] Sort headers in renesas_sdhi_core.c.
 - [MMC] Remove the following codes that made on v3 that can be achieved by
	 DMA MAPPING and IOMMU subsystem:
 -- Check if R-Car Gen3 IPMMU is used or not on patch 3.
 -- Check if all multiple segment buffers are aligned to PAGE_SIZE on patch 3.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=125593

Changes from v3:
 - Use a helper function device_iommu_mapped on patch 1 and 3.
 - Check if R-Car Gen3 IPMMU is used or not on patch 3.
 - Check if all multiple segment buffers are aligned to PAGE_SIZE on patch 3.
 - Add Reviewed-by Wolfram-san on patch 1 and 2. Note that I also got his
   Reviewed-by on patch 3, but I changed it from v2. So, I didn't add
   his Reviewed-by at this time.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=120985

Changes from v2:
 - Add some conditions in the init_card().
 - Add a comment in the init_card().
 - Add definitions for some "MAX_SEGS".
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=116729

Changes from v1:
 - Remove adding init_card ops into struct tmio_mmc_dma_ops and
   tmio_mmc_host and just set init_card on renesas_sdhi_core.c.
 - Revise typos on "mmc: tmio: No memory size limitation if runs on IOMMU".
 - Add Simon-san's Reviewed-by on a tmio patch.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=110485

Yoshihiro Shimoda (5):
  iommu: add an exported function to get minimum page size for a domain
  block: sort headers on blk-setting.c
  block: add a helper function to merge the segments by an IOMMU
  mmc: tmio: Use dma_max_mapping_size() instead of a workaround
  mmc: queue: Use bigger segments if IOMMU can merge the segments

 block/blk-settings.c             | 40 ++++++++++++++++++++++++++++++++++------
 drivers/iommu/iommu.c            | 18 +++++++++++++++---
 drivers/mmc/core/queue.c         | 33 +++++++++++++++++++++++++++++----
 drivers/mmc/host/tmio_mmc_core.c | 17 ++++-------------
 include/linux/blkdev.h           |  2 ++
 include/linux/iommu.h            |  1 +
 include/linux/mmc/host.h         |  1 +
 7 files changed, 86 insertions(+), 26 deletions(-)

-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH v6 1/5] iommu: add an exported function to get minimum page size for a domain

[Yoshihiro Shimoda]

This patch adds an exported function to get minimum page size for
a domain. This patch also modifies similar codes on the iommu.c.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/iommu.c | 18 +++++++++++++++---
 include/linux/iommu.h |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2a90638..7ed16af 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -280,6 +280,18 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
 	return ret;
 }
 
+/**
+ * iommu_get_minimum_page_size - get minimum page size for a domain
+ * @domain: the domain
+ *
+ * Allow iommu driver to get a minimum page size for a domain.
+ */
+unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain)
+{
+	return 1UL << __ffs(domain->pgsize_bitmap);
+}
+EXPORT_SYMBOL_GPL(iommu_get_minimum_page_size);
+
 int iommu_get_group_resv_regions(struct iommu_group *group,
 				 struct list_head *head)
 {
@@ -558,7 +570,7 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
 
 	BUG_ON(!domain->pgsize_bitmap);
 
-	pg_size = 1UL << __ffs(domain->pgsize_bitmap);
+	pg_size = iommu_get_minimum_page_size(domain);
 	INIT_LIST_HEAD(&mappings);
 
 	iommu_get_resv_regions(dev, &mappings);
@@ -1595,7 +1607,7 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		return -EINVAL;
 
 	/* find out the minimum page size supported */
-	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
+	min_pagesz = iommu_get_minimum_page_size(domain);
 
 	/*
 	 * both the virtual address and the physical one, as well as
@@ -1655,7 +1667,7 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
 		return 0;
 
 	/* find out the minimum page size supported */
-	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
+	min_pagesz = iommu_get_minimum_page_size(domain);
 
 	/*
 	 * The virtual address, as well as the size of the mapping, must be
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 91af22a..7e53b43 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -366,6 +366,7 @@ extern int iommu_request_dma_domain_for_dev(struct device *dev);
 extern struct iommu_resv_region *
 iommu_alloc_resv_region(phys_addr_t start, size_t length, int prot,
 			enum iommu_resv_type type);
+extern unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain);
 extern int iommu_get_group_resv_regions(struct iommu_group *group,
 					struct list_head *head);
 
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH v6 2/5] block: sort headers on blk-setting.c

[Yoshihiro Shimoda]

This patch sorts the headers in alphabetic order to ease
the maintenance for this part.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 block/blk-settings.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 2ae348c..45f2c52 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -2,16 +2,16 @@
 /*
  * Functions related to setting various queue properties from drivers
  */
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
 #include <linux/bio.h>
 #include <linux/blkdev.h>
-#include <linux/memblock.h>	/* for max_pfn/max_low_pfn */
 #include <linux/gcd.h>
-#include <linux/lcm.h>
-#include <linux/jiffies.h>
 #include <linux/gfp.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/lcm.h>
+#include <linux/memblock.h>     /* for max_pfn/max_low_pfn */
+#include <linux/module.h>
 
 #include "blk.h"
 #include "blk-wbt.h"
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH v6 3/5] block: add a helper function to merge the segments by an IOMMU

[Yoshihiro Shimoda]

This patch adds a helper function whether a queue can merge
the segments by an IOMMU.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 block/blk-settings.c   | 28 ++++++++++++++++++++++++++++
 include/linux/blkdev.h |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 45f2c52..4e4e13e 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -4,9 +4,11 @@
  */
 #include <linux/bio.h>
 #include <linux/blkdev.h>
+#include <linux/device.h>
 #include <linux/gcd.h>
 #include <linux/gfp.h>
 #include <linux/init.h>
+#include <linux/iommu.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/lcm.h>
@@ -831,6 +833,32 @@ void blk_queue_write_cache(struct request_queue *q, bool wc, bool fua)
 }
 EXPORT_SYMBOL_GPL(blk_queue_write_cache);
 
+/**
+ * blk_queue_can_use_iommu_merging - configure queue for merging segments.
+ * @q:		the request queue for the device
+ * @dev:	the device pointer for dma
+ *
+ * Tell the block layer about the iommu merging of @q.
+ */
+bool blk_queue_can_use_iommu_merging(struct request_queue *q,
+				     struct device *dev)
+{
+	struct iommu_domain *domain;
+
+	/*
+	 * If the device DMA is translated by an IOMMU, we can assume
+	 * the device can merge the segments.
+	 */
+	if (!device_iommu_mapped(dev))
+		return false;
+
+	domain = iommu_get_domain_for_dev(dev);
+	/* No need to update max_segment_size. see blk_queue_virt_boundary() */
+	blk_queue_virt_boundary(q, iommu_get_minimum_page_size(domain) - 1);
+
+	return true;
+}
+
 static int __init blk_settings_init(void)
 {
 	blk_max_low_pfn = max_low_pfn - 1;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 592669b..4d1f7dc 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1091,6 +1091,8 @@ extern void blk_queue_dma_alignment(struct request_queue *, int);
 extern void blk_queue_update_dma_alignment(struct request_queue *, int);
 extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
 extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool fua);
+extern bool blk_queue_can_use_iommu_merging(struct request_queue *q,
+					    struct device *dev);
 
 /*
  * Number of physical segments as sent to the device.
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Yoshihiro Shimoda]

Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()")
provides a helper function to get the max mapping size, we can use
the function instead of the workaround code for swiotlb.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/tmio_mmc_core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 130b91c..85bd6aa6 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -26,6 +26,7 @@
 
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -1189,19 +1190,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	mmc->max_blk_size = TMIO_MAX_BLK_SIZE;
 	mmc->max_blk_count = pdata->max_blk_count ? :
 		(PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs;
-	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
-	/*
-	 * Since swiotlb has memory size limitation, this will calculate
-	 * the maximum size locally (because we don't have any APIs for it now)
-	 * and check the current max_req_size. And then, this will update
-	 * the max_req_size if needed as a workaround.
-	 */
-	if (swiotlb_max_segment()) {
-		unsigned int max_size = (1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE;
-
-		if (mmc->max_req_size > max_size)
-			mmc->max_req_size = max_size;
-	}
+	mmc->max_req_size = min_t(unsigned int,
+				  mmc->max_blk_size * mmc->max_blk_count,
+				  dma_max_mapping_size(&pdev->dev));
 	mmc->max_seg_size = mmc->max_req_size;
 
 	if (mmc_can_gpio_ro(mmc))
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Yoshihiro Shimoda]

If max_segs of a mmc host is smaller than BLK_MAX_SEGMENTS,
the mmc subsystem tries to use such bigger segments by using
IOMMU subsystem, and then the mmc subsystem exposes such information
to the block layer by using blk_queue_can_use_iommu_merging().

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/core/queue.c | 33 +++++++++++++++++++++++++++++----
 include/linux/mmc/host.h |  1 +
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index b5b9c61..59d7606 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -196,6 +196,11 @@ static void mmc_queue_setup_discard(struct request_queue *q,
 		blk_queue_flag_set(QUEUE_FLAG_SECERASE, q);
 }
 
+static unsigned int mmc_get_max_segments(struct mmc_host *host)
+{
+	return host->can_merge ? BLK_MAX_SEGMENTS : host->max_segs;
+}
+
 /**
  * mmc_init_request() - initialize the MMC-specific per-request data
  * @q: the request queue
@@ -209,7 +214,7 @@ static int __mmc_init_request(struct mmc_queue *mq, struct request *req,
 	struct mmc_card *card = mq->card;
 	struct mmc_host *host = card->host;
 
-	mq_rq->sg = mmc_alloc_sg(host->max_segs, gfp);
+	mq_rq->sg = mmc_alloc_sg(mmc_get_max_segments(host), gfp);
 	if (!mq_rq->sg)
 		return -ENOMEM;
 
@@ -368,15 +373,24 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
 	blk_queue_bounce_limit(mq->queue, limit);
 	blk_queue_max_hw_sectors(mq->queue,
 		min(host->max_blk_count, host->max_req_size / 512));
-	blk_queue_max_segments(mq->queue, host->max_segs);
+	/* blk_queue_can_use_iommu_merging() should succeed if can_merge = 1 */
+	if (host->can_merge &&
+	    !blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)))
+		WARN_ON(1);
+	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
 
 	if (mmc_card_mmc(card))
 		block_size = card->ext_csd.data_sector_size;
 
 	blk_queue_logical_block_size(mq->queue, block_size);
-	blk_queue_max_segment_size(mq->queue,
+	/*
+	 * After blk_queue_can_use_iommu_merging() was called with succeed,
+	 * since it calls blk_queue_virt_boundary for IOMMU, the mmc should
+	 * not call blk_queue_max_segment_size().
+	 */
+	if (!host->can_merge)
+		blk_queue_max_segment_size(mq->queue,
 			round_down(host->max_seg_size, block_size));
-
 	INIT_WORK(&mq->recovery_work, mmc_mq_recovery_handler);
 	INIT_WORK(&mq->complete_work, mmc_blk_mq_complete_work);
 
@@ -422,6 +436,17 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card)
 	mq->tag_set.cmd_size = sizeof(struct mmc_queue_req);
 	mq->tag_set.driver_data = mq;
 
+	/*
+	 * Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops,
+	 * the host->can_merge should be set before to get max_segs from
+	 * mmc_get_max_segments().
+	 */
+	if (host->max_segs < BLK_MAX_SEGMENTS &&
+	    device_iommu_mapped(mmc_dev(host)))
+		host->can_merge = 1;
+	else
+		host->can_merge = 0;
+
 	ret = blk_mq_alloc_tag_set(&mq->tag_set);
 	if (ret)
 		return ret;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 43d0f0c..84b9bef 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -398,6 +398,7 @@ struct mmc_host {
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
 	unsigned int		retune_paused:1; /* re-tuning is temporarily disabled */
 	unsigned int		use_blk_mq:1;	/* use blk-mq */
+	unsigned int		can_merge:1;	/* merging can be used */
 
 	int			rescan_disable;	/* disable card detection */
 	int			rescan_entered;	/* used with nonremovable devices */
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 0/5] treewide: improve R-Car SDHI performance

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1596 bytes --]

On Thu, Jun 13, 2019 at 07:20:10PM +0900, Yoshihiro Shimoda wrote:
> This patch series is based on iommu.git / next branch.
> 
> Since SDHI host internal DMAC of the R-Car Gen3 cannot handle two or
> more segments, the performance rate (especially, eMMC HS400 reading)
> is not good. However, if IOMMU is enabled on the DMAC, since IOMMU will
> map multiple scatter gather buffers as one contignous iova, the DMAC can
> handle the iova as well and then the performance rate is possible to
> improve. In fact, I have measured the performance by using bonnie++,
> "Sequential Input - block" rate was improved on r8a7795.
> 
> To achieve this, this patch series modifies IOMMU and Block subsystem
> at first. Since I'd like to get any feedback from each subsystem whether
> this way is acceptable for upstream, I submit it to treewide with RFC.
> 
> Changes from v5:
>  - Almost all patches are new code.
>  - [4/5 for MMC] This is a refactor patch so that I don't add any
>    {Tested,Reviewed}-by tags.
>  - [5/5 for MMC] Modify MMC subsystem to use bigger segments instead of
>    the renesas_sdhi driver.
>  - [5/5 for MMC] Use BLK_MAX_SEGMENTS (128) instead of local value
>    SDHI_MAX_SEGS_IN_IOMMU (512). Even if we use BLK_MAX_SEGMENTS,
>    the performance is still good.

Thanks for your hard work, Shimoda-san!

I may not be the biggest DMA, IOMMU, and block layer expert, but I
really like how this simplifies the SDHI driver and enhances the MMC
core. So, I'll add my two cents to the patches although I can't really
comment on the main functionality.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 1/5] iommu: add an exported function to get minimum page size for a domain

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1223 bytes --]

On Thu, Jun 13, 2019 at 07:20:11PM +0900, Yoshihiro Shimoda wrote:
> This patch adds an exported function to get minimum page size for
> a domain. This patch also modifies similar codes on the iommu.c.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/iommu/iommu.c | 18 +++++++++++++++---
>  include/linux/iommu.h |  1 +
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 2a90638..7ed16af 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -280,6 +280,18 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
>  	return ret;
>  }
>  
> +/**
> + * iommu_get_minimum_page_size - get minimum page size for a domain
> + * @domain: the domain
> + *
> + * Allow iommu driver to get a minimum page size for a domain.
> + */
> +unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain)
> +{
> +	return 1UL << __ffs(domain->pgsize_bitmap);
> +}
> +EXPORT_SYMBOL_GPL(iommu_get_minimum_page_size);

What about making this a 'static inline' in the iommu header file? I'd
think it is simple enough and would save us the EXPORT symbol.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 2/5] block: sort headers on blk-setting.c

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 336 bytes --]

On Thu, Jun 13, 2019 at 07:20:12PM +0900, Yoshihiro Shimoda wrote:
> This patch sorts the headers in alphabetic order to ease
> the maintenance for this part.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Yup, I got the same result :)

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 561 bytes --]

On Thu, Jun 13, 2019 at 07:20:14PM +0900, Yoshihiro Shimoda wrote:
> Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()")
> provides a helper function to get the max mapping size, we can use
> the function instead of the workaround code for swiotlb.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I love it! I'd really like to see this code go away. Do I get this right
that this patch is kinda independent of the reset of the series? Anyway:

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 661 bytes --]


> -	blk_queue_max_segments(mq->queue, host->max_segs);
> +	/* blk_queue_can_use_iommu_merging() should succeed if can_merge = 1 */
> +	if (host->can_merge &&
> +	    !blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)))
> +		WARN_ON(1);
> +	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));

Maybe we could use WARN here to save the comment and move the info to
the printout?

-	blk_queue_max_segments(mq->queue, host->max_segs);
+	if (host->can_merge)
+		WARN(!blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)),
+		     "merging was advertised but not possible\n");
+	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Jun 13, 2019 at 5:37 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()")
> provides a helper function to get the max mapping size, we can use
> the function instead of the workaround code for swiotlb.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c

> @@ -1189,19 +1190,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
>         mmc->max_blk_size = TMIO_MAX_BLK_SIZE;
>         mmc->max_blk_count = pdata->max_blk_count ? :
>                 (PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs;
> -       mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
> -       /*
> -        * Since swiotlb has memory size limitation, this will calculate
> -        * the maximum size locally (because we don't have any APIs for it now)
> -        * and check the current max_req_size. And then, this will update
> -        * the max_req_size if needed as a workaround.
> -        */
> -       if (swiotlb_max_segment()) {
> -               unsigned int max_size = (1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE;
> -
> -               if (mmc->max_req_size > max_size)
> -                       mmc->max_req_size = max_size;
> -       }
> +       mmc->max_req_size = min_t(unsigned int,
> +                                 mmc->max_blk_size * mmc->max_blk_count,
> +                                 dma_max_mapping_size(&pdev->dev));
>         mmc->max_seg_size = mmc->max_req_size;

I'm always triggered by the use of min_t() and other casts:
mmc->max_blk_size and mmc->max_blk_count are both unsigned int.
dma_max_mapping_size() returns size_t, which can be 64-bit.

 1) Can the multiplication overflow?
    Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi:
    prevent overflow for max_req_size"), but I thought I'd better ask.
 2) In theory, dma_max_mapping_size() can return a number that doesn't
    fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size
    is zero?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 1/5] iommu: add an exported function to get minimum page size for a domain

[Christoph Hellwig]

On Thu, Jun 13, 2019 at 09:37:59PM +0200, Wolfram Sang wrote:
> What about making this a 'static inline' in the iommu header file? I'd
> think it is simple enough and would save us the EXPORT symbol.

Agreed, this seems simple enought for an inline.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Christoph Hellwig]

On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote:
> I'm always triggered by the use of min_t() and other casts:
> mmc->max_blk_size and mmc->max_blk_count are both unsigned int.
> dma_max_mapping_size() returns size_t, which can be 64-bit.
> 
>  1) Can the multiplication overflow?
>     Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi:
>     prevent overflow for max_req_size"), but I thought I'd better ask.
>  2) In theory, dma_max_mapping_size() can return a number that doesn't
>     fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size
>     is zero?

This really should use a min_t on size_t.  Otherwise the patch looks
fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 3/5] block: add a helper function to merge the segments by an IOMMU

[Christoph Hellwig]

I'm a little worried about this directly calling into the iommu
API instead of going through the DMA mapping code.  We still have
plenty of iommus not using the iommu layer for DMA mapping.  But at
least this code is in the block layer and not the driver, so maybe
we can live with it.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Christoph Hellwig]

On Thu, Jun 13, 2019 at 07:20:15PM +0900, Yoshihiro Shimoda wrote:
> +static unsigned int mmc_get_max_segments(struct mmc_host *host)
> +{
> +	return host->can_merge ? BLK_MAX_SEGMENTS : host->max_segs;
> +}

Note that BLK_MAX_SEGMENTS is really a little misnamed, it just
is a BLK_DEFAULT_SEGMENTS.  I think you are better of picking your
own value here (even if 128 ends up ok) than reusing this somewhat
confusing constant.

> +	/*
> +	 * Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops,
> +	 * the host->can_merge should be set before to get max_segs from
> +	 * mmc_get_max_segments().
> +	 */
> +	if (host->max_segs < BLK_MAX_SEGMENTS &&
> +	    device_iommu_mapped(mmc_dev(host)))
> +		host->can_merge = 1;
> +	else
> +		host->can_merge = 0;
> +

can_merge seems a little too generic a name to me.  Maybe can_iommu_merge?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Geert Uytterhoeven]

Hi Christoph,

On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote:
> > I'm always triggered by the use of min_t() and other casts:
> > mmc->max_blk_size and mmc->max_blk_count are both unsigned int.
> > dma_max_mapping_size() returns size_t, which can be 64-bit.
> >
> >  1) Can the multiplication overflow?
> >     Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi:
> >     prevent overflow for max_req_size"), but I thought I'd better ask.
> >  2) In theory, dma_max_mapping_size() can return a number that doesn't
> >     fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size
> >     is zero?
>
> This really should use a min_t on size_t.  Otherwise the patch looks
> fine:

Followed by another min() to make it fit in mmc->max_req_size, which is
unsigned int.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 1/5] iommu: add an exported function to get minimum page size for a domain

[Robin Murphy]

On 13/06/2019 11:20, Yoshihiro Shimoda wrote:
> This patch adds an exported function to get minimum page size for
> a domain. This patch also modifies similar codes on the iommu.c.

Heh, seeing this gave me a genuine déjà vu moment...

...but it turns out I actually *have* reviewed this patch before :)

https://lore.kernel.org/lkml/05eca601-0264-8141-ceeb-7ef7ad5d5650@arm.com/

Robin.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   drivers/iommu/iommu.c | 18 +++++++++++++++---
>   include/linux/iommu.h |  1 +
>   2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 2a90638..7ed16af 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -280,6 +280,18 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
>   	return ret;
>   }
>   
> +/**
> + * iommu_get_minimum_page_size - get minimum page size for a domain
> + * @domain: the domain
> + *
> + * Allow iommu driver to get a minimum page size for a domain.
> + */
> +unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain)
> +{
> +	return 1UL << __ffs(domain->pgsize_bitmap);
> +}
> +EXPORT_SYMBOL_GPL(iommu_get_minimum_page_size);
> +
>   int iommu_get_group_resv_regions(struct iommu_group *group,
>   				 struct list_head *head)
>   {
> @@ -558,7 +570,7 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
>   
>   	BUG_ON(!domain->pgsize_bitmap);
>   
> -	pg_size = 1UL << __ffs(domain->pgsize_bitmap);
> +	pg_size = iommu_get_minimum_page_size(domain);
>   	INIT_LIST_HEAD(&mappings);
>   
>   	iommu_get_resv_regions(dev, &mappings);
> @@ -1595,7 +1607,7 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
>   		return -EINVAL;
>   
>   	/* find out the minimum page size supported */
> -	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> +	min_pagesz = iommu_get_minimum_page_size(domain);
>   
>   	/*
>   	 * both the virtual address and the physical one, as well as
> @@ -1655,7 +1667,7 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
>   		return 0;
>   
>   	/* find out the minimum page size supported */
> -	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> +	min_pagesz = iommu_get_minimum_page_size(domain);
>   
>   	/*
>   	 * The virtual address, as well as the size of the mapping, must be
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 91af22a..7e53b43 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -366,6 +366,7 @@ extern int iommu_request_dma_domain_for_dev(struct device *dev);
>   extern struct iommu_resv_region *
>   iommu_alloc_resv_region(phys_addr_t start, size_t length, int prot,
>   			enum iommu_resv_type type);
> +extern unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain);
>   extern int iommu_get_group_resv_regions(struct iommu_group *group,
>   					struct list_head *head);
>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 3/5] block: add a helper function to merge the segments by an IOMMU

[Robin Murphy]

On 13/06/2019 11:20, Yoshihiro Shimoda wrote:
> This patch adds a helper function whether a queue can merge
> the segments by an IOMMU.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   block/blk-settings.c   | 28 ++++++++++++++++++++++++++++
>   include/linux/blkdev.h |  2 ++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 45f2c52..4e4e13e 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -4,9 +4,11 @@
>    */
>   #include <linux/bio.h>
>   #include <linux/blkdev.h>
> +#include <linux/device.h>
>   #include <linux/gcd.h>
>   #include <linux/gfp.h>
>   #include <linux/init.h>
> +#include <linux/iommu.h>
>   #include <linux/jiffies.h>
>   #include <linux/kernel.h>
>   #include <linux/lcm.h>
> @@ -831,6 +833,32 @@ void blk_queue_write_cache(struct request_queue *q, bool wc, bool fua)
>   }
>   EXPORT_SYMBOL_GPL(blk_queue_write_cache);
>   
> +/**
> + * blk_queue_can_use_iommu_merging - configure queue for merging segments.
> + * @q:		the request queue for the device
> + * @dev:	the device pointer for dma
> + *
> + * Tell the block layer about the iommu merging of @q.
> + */
> +bool blk_queue_can_use_iommu_merging(struct request_queue *q,
> +				     struct device *dev)
> +{
> +	struct iommu_domain *domain;
> +
> +	/*
> +	 * If the device DMA is translated by an IOMMU, we can assume
> +	 * the device can merge the segments.
> +	 */
> +	if (!device_iommu_mapped(dev))

Careful here - I think this validates the comment I made when this 
function was introduced, in that that name doesn't necesarily mean what 
it sounds like it might mean - "iommu_mapped" was as close as we managed 
to get to a convenient shorthand for "performs DMA through an 
IOMMU-API-enabled IOMMU". Specifically, it does not imply that 
translation is *currently* active; if you boot with "iommu=pt" or 
equivalent this will still return true even though the device will be 
using direct/SWIOTLB DMA ops without any IOMMU translation.

Robin.

> +		return false;
> +
> +	domain = iommu_get_domain_for_dev(dev);
> +	/* No need to update max_segment_size. see blk_queue_virt_boundary() */
> +	blk_queue_virt_boundary(q, iommu_get_minimum_page_size(domain) - 1);
> +
> +	return true;
> +}
> +
>   static int __init blk_settings_init(void)
>   {
>   	blk_max_low_pfn = max_low_pfn - 1;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 592669b..4d1f7dc 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1091,6 +1091,8 @@ extern void blk_queue_dma_alignment(struct request_queue *, int);
>   extern void blk_queue_update_dma_alignment(struct request_queue *, int);
>   extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
>   extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool fua);
> +extern bool blk_queue_can_use_iommu_merging(struct request_queue *q,
> +					    struct device *dev);
>   
>   /*
>    * Number of physical segments as sent to the device.
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 171 bytes --]


> > +		host->can_merge = 1;
> > +	else
> > +		host->can_merge = 0;
> > +
> 
> can_merge seems a little too generic a name to me.  Maybe can_iommu_merge?

Ack.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, June 14, 2019 4:46 AM
> 
> On Thu, Jun 13, 2019 at 07:20:14PM +0900, Yoshihiro Shimoda wrote:
> > Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()")
> > provides a helper function to get the max mapping size, we can use
> > the function instead of the workaround code for swiotlb.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> I love it! I'd really like to see this code go away. Do I get this right
> that this patch is kinda independent of the reset of the series? Anyway:

Thank you for your suggestion! I think so (because IOMMU and block patches seem
to need update). I'll submit 3/5 and 4/5 patches as independent later.

> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thank you for your Acked-by!

Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Yoshihiro Shimoda]

Hi Geert, Christoph,

Thank you for your comments!

> From: Geert Uytterhoeven, Sent: Friday, June 14, 2019 4:27 PM
> 
> Hi Christoph,
> 
> On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote:
> > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote:
> > > I'm always triggered by the use of min_t() and other casts:
> > > mmc->max_blk_size and mmc->max_blk_count are both unsigned int.
> > > dma_max_mapping_size() returns size_t, which can be 64-bit.
> > >
> > >  1) Can the multiplication overflow?
> > >     Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi:
> > >     prevent overflow for max_req_size"), but I thought I'd better ask.

Geert-san:

I agree.

> > >  2) In theory, dma_max_mapping_size() can return a number that doesn't
> > >     fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size
> > >     is zero?

Geert-san:

I agree. If dma_max_mapping_size() return 0x1_0000_0000, it will be truncated to 0
and then max_req_size is set to zero. It is a problem. Also, the second argument
"mmc->max_blk_size * mmc->max_blk_count" will not be overflow and then the value is
0xffff_ffff or less. So, I also think this should use size_t instead of unsigned int.

> > This really should use a min_t on size_t.  Otherwise the patch looks
> > fine:
> 
> Followed by another min() to make it fit in mmc->max_req_size, which is
> unsigned int.

Geert-san:

I'm afraid, but I cannot understand this means.
Is this patch is possible to be upstream? Or, do you have any concern?


Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 1/5] iommu: add an exported function to get minimum page size for a domain

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, June 14, 2019 4:38 AM
> 
> On Thu, Jun 13, 2019 at 07:20:11PM +0900, Yoshihiro Shimoda wrote:
> > This patch adds an exported function to get minimum page size for
> > a domain. This patch also modifies similar codes on the iommu.c.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/iommu/iommu.c | 18 +++++++++++++++---
> >  include/linux/iommu.h |  1 +
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 2a90638..7ed16af 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -280,6 +280,18 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
> >  	return ret;
> >  }
> >
> > +/**
> > + * iommu_get_minimum_page_size - get minimum page size for a domain
> > + * @domain: the domain
> > + *
> > + * Allow iommu driver to get a minimum page size for a domain.
> > + */
> > +unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain)
> > +{
> > +	return 1UL << __ffs(domain->pgsize_bitmap);
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_get_minimum_page_size);
> 
> What about making this a 'static inline' in the iommu header file? I'd
> think it is simple enough and would save us the EXPORT symbol.

Thank you for the review. I think so. I'll use 'static inline' instead of
EXPORT symbol.

Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 1/5] iommu: add an exported function to get minimum page size for a domain

[Yoshihiro Shimoda]

Hi Robin,

> From: Robin Murphy, Sent: Friday, June 14, 2019 6:41 PM
> 
> On 13/06/2019 11:20, Yoshihiro Shimoda wrote:
> > This patch adds an exported function to get minimum page size for
> > a domain. This patch also modifies similar codes on the iommu.c.
> 
> Heh, seeing this gave me a genuine déjà vu moment...
> 
> ...but it turns out I actually *have* reviewed this patch before :)
> 
> https://lore.kernel.org/lkml/05eca601-0264-8141-ceeb-7ef7ad5d5650@arm.com/

Thank you for the information :)
I realized my patch should have taken care of the CONFIG_IPMMU_API=n case.

However, the latest patch series doesn't have such a patch. So, I'll keep this
my patch on next patch version.
https://lore.kernel.org/lkml/20190603011620.31999-1-baolu.lu@linux.intel.com/

Best regards,
Yoshihiro Shimoda

> Robin.
> 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >   drivers/iommu/iommu.c | 18 +++++++++++++++---
> >   include/linux/iommu.h |  1 +
> >   2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 2a90638..7ed16af 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -280,6 +280,18 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
> >   	return ret;
> >   }
> >
> > +/**
> > + * iommu_get_minimum_page_size - get minimum page size for a domain
> > + * @domain: the domain
> > + *
> > + * Allow iommu driver to get a minimum page size for a domain.
> > + */
> > +unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain)
> > +{
> > +	return 1UL << __ffs(domain->pgsize_bitmap);
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_get_minimum_page_size);
> > +
> >   int iommu_get_group_resv_regions(struct iommu_group *group,
> >   				 struct list_head *head)
> >   {
> > @@ -558,7 +570,7 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
> >
> >   	BUG_ON(!domain->pgsize_bitmap);
> >
> > -	pg_size = 1UL << __ffs(domain->pgsize_bitmap);
> > +	pg_size = iommu_get_minimum_page_size(domain);
> >   	INIT_LIST_HEAD(&mappings);
> >
> >   	iommu_get_resv_regions(dev, &mappings);
> > @@ -1595,7 +1607,7 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
> >   		return -EINVAL;
> >
> >   	/* find out the minimum page size supported */
> > -	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> > +	min_pagesz = iommu_get_minimum_page_size(domain);
> >
> >   	/*
> >   	 * both the virtual address and the physical one, as well as
> > @@ -1655,7 +1667,7 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
> >   		return 0;
> >
> >   	/* find out the minimum page size supported */
> > -	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> > +	min_pagesz = iommu_get_minimum_page_size(domain);
> >
> >   	/*
> >   	 * The virtual address, as well as the size of the mapping, must be
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 91af22a..7e53b43 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -366,6 +366,7 @@ extern int iommu_request_dma_domain_for_dev(struct device *dev);
> >   extern struct iommu_resv_region *
> >   iommu_alloc_resv_region(phys_addr_t start, size_t length, int prot,
> >   			enum iommu_resv_type type);
> > +extern unsigned long iommu_get_minimum_page_size(struct iommu_domain *domain);
> >   extern int iommu_get_group_resv_regions(struct iommu_group *group,
> >   					struct list_head *head);
> >
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jun 17, 2019 at 6:54 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Friday, June 14, 2019 4:27 PM
> > On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote:
> > > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote:
> > > > I'm always triggered by the use of min_t() and other casts:
> > > > mmc->max_blk_size and mmc->max_blk_count are both unsigned int.
> > > > dma_max_mapping_size() returns size_t, which can be 64-bit.
> > > >
> > > >  1) Can the multiplication overflow?
> > > >     Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi:
> > > >     prevent overflow for max_req_size"), but I thought I'd better ask.
>
> Geert-san:
>
> I agree.
>
> > > >  2) In theory, dma_max_mapping_size() can return a number that doesn't
> > > >     fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size
> > > >     is zero?
>
> Geert-san:
>
> I agree. If dma_max_mapping_size() return 0x1_0000_0000, it will be truncated to 0
> and then max_req_size is set to zero. It is a problem. Also, the second argument
> "mmc->max_blk_size * mmc->max_blk_count" will not be overflow and then the value is
> 0xffff_ffff or less. So, I also think this should use size_t instead of unsigned int.
>
> > > This really should use a min_t on size_t.  Otherwise the patch looks
> > > fine:
> >
> > Followed by another min() to make it fit in mmc->max_req_size, which is
> > unsigned int.
>
> Geert-san:
>
> I'm afraid, but I cannot understand this means.
> Is this patch is possible to be upstream? Or, do you have any concern?

Please disregard my last comment: as the value of "mmc->max_blk_size *
mmc->max_blk_count" is always 0xffff_ffff or less, "min_t(size_t,
mmc->max_blk_size * mmc->max_blk_count, dma_max_mapping_size(&pdev->dev))"
will always be 0xffff_ffff or less, too, so there is no extra step needed
to make it fit in mmc->max_req_size.


Sorry for the confusion.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 3/5] block: add a helper function to merge the segments by an IOMMU

[Yoshihiro Shimoda]

Hi Robin,

> From: Robin Murphy, Sent: Friday, June 14, 2019 6:55 PM
> 
> On 13/06/2019 11:20, Yoshihiro Shimoda wrote:
<snip>
> > +bool blk_queue_can_use_iommu_merging(struct request_queue *q,
> > +				     struct device *dev)
> > +{
> > +	struct iommu_domain *domain;
> > +
> > +	/*
> > +	 * If the device DMA is translated by an IOMMU, we can assume
> > +	 * the device can merge the segments.
> > +	 */
> > +	if (!device_iommu_mapped(dev))
> 
> Careful here - I think this validates the comment I made when this
> function was introduced, in that that name doesn't necesarily mean what
> it sounds like it might mean - "iommu_mapped" was as close as we managed
> to get to a convenient shorthand for "performs DMA through an
> IOMMU-API-enabled IOMMU". Specifically, it does not imply that
> translation is *currently* active; if you boot with "iommu=pt" or
> equivalent this will still return true even though the device will be
> using direct/SWIOTLB DMA ops without any IOMMU translation.

Thank you for your comments. I understood the mean of "iommu_mapped" and
this patch's condition causes a problem on iommu=pt.
So, I'll add and additional condition like
"domain->type == IOMMU_DOMAIN_DMA" to check whether the translation is
currently active on the domain or not.

Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, June 14, 2019 4:59 AM
> 
> > -	blk_queue_max_segments(mq->queue, host->max_segs);
> > +	/* blk_queue_can_use_iommu_merging() should succeed if can_merge = 1 */
> > +	if (host->can_merge &&
> > +	    !blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)))
> > +		WARN_ON(1);
> > +	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> 
> Maybe we could use WARN here to save the comment and move the info to
> the printout?
> 
> -	blk_queue_max_segments(mq->queue, host->max_segs);
> +	if (host->can_merge)
> +		WARN(!blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)),
> +		     "merging was advertised but not possible\n");
> +	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));

Thank you for the suggestion. It's a good idea! I'll fix the patch.

Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Yoshihiro Shimoda]

Hi Christoph,

> From: Christoph Hellwig, Sent: Friday, June 14, 2019 4:25 PM
> 
> On Thu, Jun 13, 2019 at 07:20:15PM +0900, Yoshihiro Shimoda wrote:
> > +static unsigned int mmc_get_max_segments(struct mmc_host *host)
> > +{
> > +	return host->can_merge ? BLK_MAX_SEGMENTS : host->max_segs;
> > +}
> 
> Note that BLK_MAX_SEGMENTS is really a little misnamed, it just
> is a BLK_DEFAULT_SEGMENTS.  I think you are better of picking your
> own value here (even if 128 ends up ok) than reusing this somewhat
> confusing constant.

Thank you for your comments. I got it. I'll fix this.

> > +	/*
> > +	 * Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops,
> > +	 * the host->can_merge should be set before to get max_segs from
> > +	 * mmc_get_max_segments().
> > +	 */
> > +	if (host->max_segs < BLK_MAX_SEGMENTS &&
> > +	    device_iommu_mapped(mmc_dev(host)))
> > +		host->can_merge = 1;
> > +	else
> > +		host->can_merge = 0;
> > +
> 
> can_merge seems a little too generic a name to me.  Maybe can_iommu_merge?

I'll fix the name. Also, only the device_iommu_mapped() condition wiil cause
a problem on iommu=pt [1]. So, I'll add another condition here.

[1]
https://marc.info/?l=linux-mmc&m=156050608709643&w=2

Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Christoph Hellwig]

On Mon, Jun 17, 2019 at 06:46:33AM +0000, Yoshihiro Shimoda wrote:
> > can_merge seems a little too generic a name to me.  Maybe can_iommu_merge?
> 
> I'll fix the name. Also, only the device_iommu_mapped() condition wiil cause
> a problem on iommu=pt [1]. So, I'll add another condition here.

Instead of adding another condition here I think we need to properly
abstract it out in the DMA layer.  E.g. have a

unsigned long dma_get_merge_boundary(struct device *dev)
{
	const struct dma_map_ops *ops = get_dma_ops(dev);

	if (!ops || !ops->get_merge_boundary)
		return 0; /* can't merge */
	return ops->get_merge_boundary(dev);
}

and then implement the method in dma-iommu.c.

blk_queue_can_use_iommu_merging then comes:

bool blk_queue_enable_iommu_merging(struct request_queue *q,
		struct device *dev)
{
	unsigned long boundary = dma_get_merge_boundary(dev);

	if (!boundary)
		return false;
	blk_queue_virt_boundary(q, boundary);
	return true;
}
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Monday, June 17, 2019 3:23 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Jun 17, 2019 at 6:54 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Geert Uytterhoeven, Sent: Friday, June 14, 2019 4:27 PM
> > > On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote:
> > > > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote:
<snip>
> > > > This really should use a min_t on size_t.  Otherwise the patch looks
> > > > fine:
> > >
> > > Followed by another min() to make it fit in mmc->max_req_size, which is
> > > unsigned int.
> >
> > Geert-san:
> >
> > I'm afraid, but I cannot understand this means.
> > Is this patch is possible to be upstream? Or, do you have any concern?
> 
> Please disregard my last comment: as the value of "mmc->max_blk_size *
> mmc->max_blk_count" is always 0xffff_ffff or less, "min_t(size_t,
> mmc->max_blk_size * mmc->max_blk_count, dma_max_mapping_size(&pdev->dev))"
> will always be 0xffff_ffff or less, too, so there is no extra step needed
> to make it fit in mmc->max_req_size.

Thank you for your prompt reply! I understood it.

> Sorry for the confusion.

No worries.

Best regards,
Yoshihiro Shimoda

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments

[Yoshihiro Shimoda]

Hi Christoph,

> From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:54 PM
> 
> On Mon, Jun 17, 2019 at 06:46:33AM +0000, Yoshihiro Shimoda wrote:
> > > can_merge seems a little too generic a name to me.  Maybe can_iommu_merge?
> >
> > I'll fix the name. Also, only the device_iommu_mapped() condition wiil cause
> > a problem on iommu=pt [1]. So, I'll add another condition here.
> 
> Instead of adding another condition here I think we need to properly
> abstract it out in the DMA layer.  E.g. have a

Thank you for your comment and sample code! I'll add such functions
on next patch series.

Best regards,
Yoshihiro Shimoda

> unsigned long dma_get_merge_boundary(struct device *dev)
> {
> 	const struct dma_map_ops *ops = get_dma_ops(dev);
> 
> 	if (!ops || !ops->get_merge_boundary)
> 		return 0; /* can't merge */
> 	return ops->get_merge_boundary(dev);
> }
> 
> and then implement the method in dma-iommu.c.
> 
> blk_queue_can_use_iommu_merging then comes:
> 
> bool blk_queue_enable_iommu_merging(struct request_queue *q,
> 		struct device *dev)
> {
> 	unsigned long boundary = dma_get_merge_boundary(dev);
> 
> 	if (!boundary)
> 		return false;
> 	blk_queue_virt_boundary(q, boundary);
> 	return true;
> }
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu