Skip to contentSkip to conversation

Fix NVMe driver support on Power with 32-bit DMA

PATCH Superseded Nishanth Aravamudan 6 participants 41 messages Oct 2, 2015 – Oct 19, 2015

Subsystems (4)
PowerPC Block layer Generic include/asm header files NVMe

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Nishanth Aravamudan

author
#0 lore raw

cover Fix NVMe driver support on Power with 32-bit DMA

We received a bug report recently when DDW (64-bit direct DMA on Power) is not enabled for NVMe devices. In that case, we fall back to 32-bit DMA via the IOMMU, which is always done via 4K TCEs (Translation Control Entries).

The NVMe device driver, though, assumes that the DMA alignment for the PRP entries will match the device's page size, and that the DMA aligment matches the kernel's page aligment. On Power, the the IOMMU page size, as mentioned above, can be 4K, while the device can have a page size of 8K, while the kernel has a page size of 64K. This eventually trips the BUG_ON in nvme_setup_prps(), as we have a 'dma_len' that is a multiple of 4K but not 8K (e.g., 0xF000).

In this particular case, and generally, we want to use the IOMMU's page size for the default device page size, rather than the kernel's page size.

This series consists of two patches, one of which exposes the IOMMU's page shift on Power (currently only the page size is exposed, and it seems unnecessary to ilog2 that value in the driver). The second patch leverages this value on Power in the NVMe driver.

With these patches, a NVMe device survives our internal hardware exerciser; the kernel BUGs within a few seconds without the patch.

author
#1 lore raw

PATCH 1/2 powerpc/iommu: expose IOMMU page shift

We will leverage this macro in the NVMe driver, which needs to know the configured IOMMU page shift to properly configure its device's page size.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>

---
Given this is available, it seems reasonable to expose -- and it doesn't really make sense to make the driver do a log2 call on the existing IOMMU_PAGE_SIZE() value.

arch/powerpc/include/asm/iommu.h +1 -0
index ca18cff..6fdf857 100644
@@ -36,6 +36,7 @@
 #define IOMMU_PAGE_MASK_4K       (~((1 << IOMMU_PAGE_SHIFT_4K) - 1))
 #define IOMMU_PAGE_ALIGN_4K(addr) _ALIGN_UP(addr, IOMMU_PAGE_SIZE_4K)
 
+#define IOMMU_PAGE_SHIFT(tblptr) (tblptr)->it_page_shift
 #define IOMMU_PAGE_SIZE(tblptr) (ASM_CONST(1) << (tblptr)->it_page_shift)
 #define IOMMU_PAGE_MASK(tblptr) (~((1 << (tblptr)->it_page_shift) - 1))
 #define IOMMU_PAGE_ALIGN(addr, tblptr) _ALIGN_UP(addr, IOMMU_PAGE_SIZE(tblptr))
author
#2 lore raw

PATCH 2/2 drivers/nvme: default to the IOMMU page size on Power

We received a bug report recently when DDW (64-bit direct DMA on Power) is not enabled for NVMe devices. In that case, we fall back to 32-bit DMA via the IOMMU, which is always done via 4K TCEs (Translation Control Entries).

The NVMe device driver, though, assumes that the DMA alignment for the PRP entries will match the device's page size, and that the DMA aligment matches the kernel's page aligment. On Power, the the IOMMU page size, as mentioned above, can be 4K, while the device can have a page size of 8K, while the kernel has a page size of 64K. This eventually trips the BUG_ON in nvme_setup_prps(), as we have a 'dma_len' that is a multiple of 4K but not 8K (e.g., 0xF000).

In this particular case, and generally, we want to use the IOMMU's page size for the default device page size, rather than the kernel's page size.

With this patch, a NVMe device survives our internal hardware exerciser; the kernel BUGs within a few seconds without the patch.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
drivers/block/nvme-core.c +6 -0
index 7920c27..969a95e 100644
@@ -42,6 +42,7 @@
 #include <linux/types.h>
 #include <scsi/sg.h>
 #include <asm-generic/io-64-nonatomic-lo-hi.h>
+#include <asm/iommu.h>
 
 #define NVME_MINORS		(1U << MINORBITS)
 #define NVME_Q_DEPTH		1024

@@ -1680,6 +1681,11 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	unsigned page_shift = PAGE_SHIFT;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
+#ifdef CONFIG_PPC64
+	struct iommu_table *tbl = get_iommu_table_base(dev->dev);
+	if (tbl)
+		page_shift = IOMMU_PAGE_SHIFT(tbl);
+#endif
 
 	if (page_shift < dev_page_min) {
 		dev_err(dev->dev,
#3 lore raw

Re: [PATCH 2/2] drivers/nvme: default to the IOMMU page size on Power

Hi Nishanth,

please expose this value through the generic DMA API instead of adding architecture specific hacks to drivers.

author
#4 lore raw

Re: [PATCH 2/2] drivers/nvme: default to the IOMMU page size on Power

Quoted 4 lines · Christoph Hellwig

Hi Nishanth,

please expose this value through the generic DMA API instead of adding architecture specific hacks to drivers.

Ok, I'm happy to do that instead -- what I struggled with is that I don't have enough knowledge of the various architectures to provide the right default implementation. It should be sufficient for the default to return PAGE_SHIFT, and on Power just override that to return the IOMMU table's page size? Since the only user will be the NVMe driver currently, that should be fine?

Sorry for the less-than-ideal patch!

-Nish

#5 lore raw

Re: [PATCH 2/2] drivers/nvme: default to the IOMMU page size on Power

Quoted 6 lines · Nishanth Aravamudan

Ok, I'm happy to do that instead -- what I struggled with is that I don't have enough knowledge of the various architectures to provide the right default implementation. It should be sufficient for the default to return PAGE_SHIFT, and on Power just override that to return the IOMMU table's page size? Since the only user will be the NVMe driver currently, that should be fine?

I think that's fine.

Quoted 1 line

Sorry for the less-than-ideal patch!

Np, it's a reasonable first attempt that we just need to refine.

build bot
#6 lore raw

Re: [PATCH 2/2] drivers/nvme: default to the IOMMU page size on Power

In reply to Nishanth Aravamudan
config: arm64-allmodconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross #…

Hi Nishanth,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: arm64-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64

All error/warnings (new ones prefixed by >>):

>> drivers/block/nvme-core.c:45:23: fatal error: asm/iommu.h: No such file or directory
    #include <asm/iommu.h>
                          ^

compilation terminated.

vim +45 drivers/block/nvme-core.c

    39	#include <linux/sched.h>
    40	#include <linux/slab.h>
    41	#include <linux/t10-pi.h>
    42	#include <linux/types.h>
    43	#include <scsi/sg.h>
    44	#include <asm-generic/io-64-nonatomic-lo-hi.h>
  > 45	#include <asm/iommu.h>
    46	
    47	#define NVME_MINORS		(1U << MINORBITS)
    48	#define NVME_Q_DEPTH		1024

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

author
#10 lore raw

v2 cover Fix NVMe driver support on Power with 32-bit DMA

In reply to Nishanth Aravamudan

We received a bug report recently when DDW (64-bit direct DMA on Power) is not enabled for NVMe devices. In that case, we fall back to 32-bit DMA via the IOMMU, which is always done via 4K TCEs (Translation Control Entries).

The NVMe device driver, though, assumes that the DMA alignment for the PRP entries will match the device's page size, and that the DMA aligment matches the kernel's page aligment. On Power, the the IOMMU page size, as mentioned above, can be 4K, while the device can have a page size of 8K, while the kernel has a page size of 64K. This eventually trips the BUG_ON in nvme_setup_prps(), as we have a 'dma_len' that is a multiple of 4K but not 8K (e.g., 0xF000).

In this particular case, and generally, we want to use the IOMMU's page size for the default device page size, rather than the kernel's page size.

This series consists of five patches:

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT
2) override the generic implementation on Power to use the IOMMU table's page shift if available
3) allow further specific overriding on power with machdep platform overrides
4) use the machdep override on pseries, as the DDW code puts the TCE shift in a special property and there is no IOMMU table available
5) leverage the new API in the NVMe driver

With these patches, a NVMe device survives our internal hardware exerciser; the kernel BUGs within a few seconds without the patch.

 arch/powerpc/include/asm/dma-mapping.h   |  3 +++
 arch/powerpc/include/asm/machdep.h       |  3 ++-
 arch/powerpc/kernel/dma.c                | 11 +++++++++++
 arch/powerpc/platforms/pseries/iommu.c   | 36 ++++++++++++++++++++++++++++++++++++
 drivers/block/nvme-core.c                |  3 ++-
 include/asm-generic/dma-mapping-common.h |  7 +++++++
 6 files changed, 61 insertions(+), 2 deletions(-)

v1 -> v2:
Based upon feedback from Christoph Hellwig, rather than using an arch-specific hack, expose the DMA page shift via a generic DMA API and override it on Power as needed.

author
#11 lore raw

PATCH v2 1/5 dma-mapping: add generic dma_get_page_shift API

Drivers like NVMe need to be able to determine the page size used for DMA transfers. Add a new API that defaults to return PAGE_SHIFT on all architectures.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
include/asm-generic/dma-mapping-common.h +7 -0
index b1bc954..86e4e97 100644
@@ -355,4 +355,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
 }
 #endif
 
+#ifndef HAVE_ARCH_DMA_GET_PAGE_SHIFT
+static inline unsigned long dma_get_page_shift(struct device *dev)
+{
+	return PAGE_SHIFT;
+}
+#endif
+
 #endif
author
#12 lore raw

PATCH v2 2/5 powerpc/dma-mapping: override dma_get_page_shift

On Power, the kernel's page size can differ from the IOMMU's page size, so we need to override the generic implementation, which always returns the kernel's page size. Lookup the IOMMU's page size from struct iommu_table, if available. Fallback to the kernel's page size, otherwise.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
arch/powerpc/include/asm/dma-mapping.h +3 -0
index 7f522c0..c5638f4 100644
@@ -125,6 +125,9 @@ static inline void set_dma_offset(struct device *dev, dma_addr_t off)
 #define HAVE_ARCH_DMA_SET_MASK 1
 extern int dma_set_mask(struct device *dev, u64 dma_mask);
 
+#define HAVE_ARCH_DMA_GET_PAGE_SHIFT 1
+extern unsigned long dma_get_page_shift(struct device *dev);
+
 #include <asm-generic/dma-mapping-common.h>
 
 extern int __dma_set_mask(struct device *dev, u64 dma_mask);
arch/powerpc/kernel/dma.c +9 -0
index 59503ed..e805af2 100644
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)
 }
 EXPORT_SYMBOL(dma_set_mask);
 
+unsigned long dma_get_page_shift(struct device *dev)
+{
+	struct iommu_table *tbl = get_iommu_table_base(dev);
+	if (tbl)
+		return tbl->it_page_shift;
+	return PAGE_SHIFT;
+}
+EXPORT_SYMBOL(dma_get_page_shift);
+
 u64 __dma_get_required_mask(struct device *dev)
 {
 	struct dma_map_ops *dma_ops = get_dma_ops(dev);
author
#13 lore raw

PATCH v2 3/5 powerpc/dma: implement per-platform dma_get_page_shift

The IOMMU page size is not always stored in struct iommu on Power. Specifically if a device is configured for DDW (Dynamic DMA Windows aka. 64-bit direct DMA), the used TCE (Translation Control Entry) size is stored in a special device property created at run-time by the DDW configuration code. DDW is a pseries-specific feature, so allow platforms to override the implementation of dma_get_page_shift if desired.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
arch/powerpc/include/asm/machdep.h +2 -1
index cab6753..5c372e3 100644
@@ -78,9 +78,10 @@ struct machdep_calls {
 #endif
 #endif /* CONFIG_PPC64 */
 
-	/* Platform set_dma_mask and dma_get_required_mask overrides */
+	/* Platform overrides */
 	int		(*dma_set_mask)(struct device *dev, u64 dma_mask);
 	u64		(*dma_get_required_mask)(struct device *dev);
+	unsigned long	(*dma_get_page_shift)(struct device *dev);
 
 	int		(*probe)(void);
 	void		(*setup_arch)(void); /* Optional, may be NULL */
arch/powerpc/kernel/dma.c +2 -0
index e805af2..c363896 100644
@@ -338,6 +338,8 @@ EXPORT_SYMBOL(dma_set_mask);
 unsigned long dma_get_page_shift(struct device *dev)
 {
 	struct iommu_table *tbl = get_iommu_table_base(dev);
+	if (ppc_md.dma_get_page_shift)
+		return ppc_md.dma_get_page_shift(dev);
 	if (tbl)
 		return tbl->it_page_shift;
 	return PAGE_SHIFT;
author
#14 lore raw

PATCH v2 4/5 pseries/iommu: implement DDW-aware dma_get_page_shift

When DDW (Dynamic DMA Windows) are present for a device, we have stored the TCE (Translation Control Entry) size in a special device tree property. Check if we have enabled DDW for the device and return the TCE size from that property if present. If the property isn't present, fallback to looking the value up in struct iommu_table. If we don't find a iommu_table, fallback to the kernel's page size.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
arch/powerpc/platforms/pseries/iommu.c +36 -0
index 0946b98..1bf6471 100644
@@ -1292,6 +1292,40 @@ static u64 dma_get_required_mask_pSeriesLP(struct device *dev)
 	return dma_iommu_ops.get_required_mask(dev);
 }
 
+static unsigned long dma_get_page_shift_pSeriesLP(struct device *dev)
+{
+	struct iommu_table *tbl;
+
+	if (!disable_ddw && dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+		struct device_node *dn;
+
+		dn = pci_device_to_OF_node(pdev);
+
+		/* search upwards for ibm,dma-window */
+		for (; dn && PCI_DN(dn) && !PCI_DN(dn)->table_group;
+				dn = dn->parent)
+			if (of_get_property(dn, "ibm,dma-window", NULL))
+				break;
+		/*
+		 * if there is a DDW configuration, the TCE shift is stored in
+		 * the property
+		 */
+		if (dn && PCI_DN(dn)) {
+			const struct dynamic_dma_window_prop *direct64 =
+				of_get_property(dn, DIRECT64_PROPNAME, NULL);
+			if (direct64)
+				return be32_to_cpu(direct64->tce_shift);
+		}
+	}
+
+	tbl = get_iommu_table_base(dev);
+	if (tbl)
+		return tbl->it_page_shift;
+
+	return PAGE_SHIFT;
+}
+
 #else  /* CONFIG_PCI */
 #define pci_dma_bus_setup_pSeries	NULL
 #define pci_dma_dev_setup_pSeries	NULL

@@ -1299,6 +1333,7 @@ static u64 dma_get_required_mask_pSeriesLP(struct device *dev)
 #define pci_dma_dev_setup_pSeriesLP	NULL
 #define dma_set_mask_pSeriesLP		NULL
 #define dma_get_required_mask_pSeriesLP	NULL
+#define dma_get_page_shift_pSeriesLP	NULL
 #endif /* !CONFIG_PCI */
 
 static int iommu_mem_notifier(struct notifier_block *nb, unsigned long action,

@@ -1395,6 +1430,7 @@ void iommu_init_early_pSeries(void)
 		pseries_pci_controller_ops.dma_dev_setup = pci_dma_dev_setup_pSeriesLP;
 		ppc_md.dma_set_mask = dma_set_mask_pSeriesLP;
 		ppc_md.dma_get_required_mask = dma_get_required_mask_pSeriesLP;
+		ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;
 	} else {
 		pseries_pci_controller_ops.dma_bus_setup = pci_dma_bus_setup_pSeries;
 		pseries_pci_controller_ops.dma_dev_setup = pci_dma_dev_setup_pSeries;
author
#15 lore raw

PATCH v2 5/5 drivers/nvme: default to the IOMMU page size

We received a bug report recently when DDW (64-bit direct DMA on Power) is not enabled for NVMe devices. In that case, we fall back to 32-bit DMA via the IOMMU, which is always done via 4K TCEs (Translation Control Entries).

The NVMe device driver, though, assumes that the DMA alignment for the PRP entries will match the device's page size, and that the DMA aligment matches the kernel's page aligment. On Power, the the IOMMU page size, as mentioned above, can be 4K, while the device can have a page size of 8K, while the kernel has a page size of 64K. This eventually trips the BUG_ON in nvme_setup_prps(), as we have a 'dma_len' that is a multiple of 4K but not 8K (e.g., 0xF000).

In this particular case of page sizes, we clearly want to use the IOMMU's page size in the driver. And generally, the NVMe driver in this function should be using the IOMMU's page size for the default device page size, rather than the kernel's page size.

With this patch, a NVMe device survives our internal hardware exerciser; the kernel BUGs within a few seconds without the patch.

---
v1 -> v2:
Based upon feedback from Christoph Hellwig, implement the IOMMU page size lookup as a generic DMA API, rather than an architecture-specific hack.

drivers/block/nvme-core.c +2 -1
index b97fc3f..c561137 100644
@@ -18,6 +18,7 @@
 #include <linux/blk-mq.h>
 #include <linux/cpu.h>
 #include <linux/delay.h>
+#include <linux/dma-mapping.h>
 #include <linux/errno.h>
 #include <linux/fs.h>
 #include <linux/genhd.h>

@@ -1713,7 +1714,7 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	u32 aqa;
 	u64 cap = readq(&dev->bar->cap);
 	struct nvme_queue *nvmeq;
-	unsigned page_shift = PAGE_SHIFT;
+	unsigned page_shift = dma_get_page_shift(dev->dev);
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
build bot
#17 lore raw

Re: [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

In reply to Nishanth Aravamudan
config: powerpc-defconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross #…

Hi Nishanth,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: powerpc-defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc

All error/warnings (new ones prefixed by >>):

arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':

>> arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'
      ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;
            ^

vim +1433 arch/powerpc/platforms/pseries/iommu.c

1427
1428 if (firmware_has_feature(FW_FEATURE_LPAR)) {
1429 pseries_pci_controller_ops.dma_bus_setup = pci_dma_bus_setup_pSeriesLP;
1430 pseries_pci_controller_ops.dma_dev_setup = pci_dma_dev_setup_pSeriesLP;
1431 ppc_md.dma_set_mask = dma_set_mask_pSeriesLP;
1432 ppc_md.dma_get_required_mask = dma_get_required_mask_pSeriesLP;

> 1433			ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;

1434 } else {
1435 pseries_pci_controller_ops.dma_bus_setup = pci_dma_bus_setup_pSeries;
1436 pseries_pci_controller_ops.dma_dev_setup = pci_dma_dev_setup_pSeries;

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 21511 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20151003/f5223439/attachment-0001.obj>

build bot
#16 lore raw

Re: [PATCH 5/5 v2] drivers/nvme: default to the IOMMU page size

In reply to Nishanth Aravamudan
config: sparc64-allyesconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross…

Hi Nishanth,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: sparc64-allyesconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64

All error/warnings (new ones prefixed by >>):

drivers/block/nvme-core.c: In function 'nvme_configure_admin_queue':

>> drivers/block/nvme-core.c:1717:2: error: implicit declaration of function 'dma_get_page_shift' [-Werror=implicit-function-declaration]
     unsigned page_shift = dma_get_page_shift(dev->dev);
     ^

cc1: some warnings being treated as errors

vim +/dma_get_page_shift +1717 drivers/block/nvme-core.c

1711 static int nvme_configure_admin_queue(struct nvme_dev *dev)
1712 {
1713 int result;
1714 u32 aqa;
1715 u64 cap = readq(&dev->bar->cap);
1716 struct nvme_queue *nvmeq;

> 1717		unsigned page_shift = dma_get_page_shift(dev->dev);

1718 unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
1719 unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
1720

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 43733 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20151003/9547770b/attachment-0001.obj>

#36 lore raw
In reply to Nishanth Aravamudan
Quoted 2 lines · Nishanth Aravamudan

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

So you chose to return the granularity of the iommu to the driver rather than providing a way for the driver to request a specific alignment for DMA mappings. Any specific reason ?

Cheers,
Ben.

author
#37 lore raw
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 2 lines · Nishanth Aravamudan

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

So you chose to return the granularity of the iommu to the driver rather than providing a way for the driver to request a specific alignment for DMA mappings. Any specific reason ?

Right, I did start with your advice and tried that approach, but it turned out I was wrong about the actual issue at the time. The problem for NVMe isn't actually the starting address alignment (which it can handle not being aligned to the device's page size). It doesn't handle (addr + len % dev_page_size != 0). That is, it's really a length alignment issue.

It seems incredibly device specific to have a an API into the DMA code to request an end alignment -- no other device seems to have this issue/design. If you think that's better, I can fiddle with that instead.

Sorry, I should have called this out better as an alternative consideration.

-Nish

#38 lore raw
Quoted 7 lines · Nishanth Aravamudan

Right, I did start with your advice and tried that approach, but it turned out I was wrong about the actual issue at the time. The problem for NVMe isn't actually the starting address alignment (which it can handle not being aligned to the device's page size). It doesn't handle (addr + len % dev_page_size != 0). That is, it's really a length alignment issue.

Quoted 7 lines

It seems incredibly device specific to have a an API into the DMA code to request an end alignment -- no other device seems to have this issue/design. If you think that's better, I can fiddle with that instead.

Sorry, I should have called this out better as an alternative consideration.

Nah it's fine. Ok. Also adding the alignment requirement to the API would have been a much more complex patch since it would have had to be implemented for all archs.

I think your current solution is fine.

Cheers,
Ben.

author
#39 lore raw
Quoted 13 lines · Benjamin Herrenschmidt
Quoted 12 lines · Nishanth Aravamudan

Right, I did start with your advice and tried that approach, but it turned out I was wrong about the actual issue at the time. The problem for NVMe isn't actually the starting address alignment (which it can handle not being aligned to the device's page size). It doesn't handle (addr + len % dev_page_size != 0). That is, it's really a length alignment issue.

It seems incredibly device specific to have a an API into the DMA code to request an end alignment -- no other device seems to have this issue/design. If you think that's better, I can fiddle with that instead.

Quoted 8 lines
Quoted 2 lines

Sorry, I should have called this out better as an alternative consideration.

Nah it's fine. Ok. Also adding the alignment requirement to the API would have been a much more complex patch since it would have had to be implemented for all archs.

I think your current solution is fine.

Great, thanks. Also, while it's possible an alignment API would be more performant...we're already not using DDW on Power in this case, performance is not a primary concern. We want to simply be functional/correct in this configuration.

-Nish

#40 lore raw
In reply to Benjamin Herrenschmidt
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 2 lines · Nishanth Aravamudan

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

So you chose to return the granularity of the iommu to the driver rather than providing a way for the driver to request a specific alignment for DMA mappings. Any specific reason ?

At least for NVMe that's the way to go - it allows to set a page set in the device which should fit the IOMMU page size.

#7 lore raw

Re: [PATCH 1/2] powerpc/iommu: expose IOMMU page shift

In reply to Nishanth Aravamudan
Quoted 16 lines · Nishanth Aravamudan

We will leverage this macro in the NVMe driver, which needs to know the configured IOMMU page shift to properly configure its device's page size.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>

---
Given this is available, it seems reasonable to expose -- and it doesn't really make sense to make the driver do a log2 call on the existing IOMMU_PAGE_SIZE() value.

arch/powerpc/include/asm/iommu.h +0 -0
index ca18cff..6fdf857 100644

arch/powerpc/include/asm/iommu.h line 40

Quoted 5 lines
arch/powerpc/include/asm/iommu.h +1 -0
@@ -36,6 +36,7 @@
 #define IOMMU_PAGE_MASK_4K       (~((1 << IOMMU_PAGE_SHIFT_4K) - 1))
 #define IOMMU_PAGE_ALIGN_4K(addr) _ALIGN_UP(addr, IOMMU_PAGE_SIZE_4K)
 
+#define IOMMU_PAGE_SHIFT(tblptr) (tblptr)->it_page_shift
 #define IOMMU_PAGE_SIZE(tblptr) (ASM_CONST(1) << (tblptr)->it_page_shift)

Seems like it would be a touch safer to alter IOMMU_PAGE_SIZE so it uses the new IOMMU_PAGE_SHIFT macro.

Quoted 3 lines
arch/powerpc/include/asm/iommu.h +0 -0
 #define IOMMU_PAGE_MASK(tblptr) (~((1 << (tblptr)->it_page_shift) - 1))
 #define IOMMU_PAGE_ALIGN(addr, tblptr) _ALIGN_UP(addr, IOMMU_PAGE_SIZE(tblptr))
Signature · 11 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20151006/655bdd8a/attachment.sig>
reviewer
#18 lore raw

Re: [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

In reply to kbuild test robot
Quoted 11 lines · kbuild test robot

Hi Nishanth,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: powerpc-defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc

Quoted 5 lines

All error/warnings (new ones prefixed by >>):

arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':

Quoted 1 line
Quoted 1 line

arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'

      ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;

It was added in patch 3/5, so I think this error is bogus. Unless there's a typo I'm missing?

cheers

reviewer
#28 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

In reply to Nishanth Aravamudan
Quoted 13 lines · Nishanth Aravamudan

Drivers like NVMe need to be able to determine the page size used for DMA transfers. Add a new API that defaults to return PAGE_SHIFT on all architectures.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
include/asm-generic/dma-mapping-common.h +0 -0
index b1bc954..86e4e97 100644
@@ -355,4 +355,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
 }

include/asm-generic/dma-mapping-common.h static inline int dma_set_mask(struct device *dev, u64 mask) · line 365

Quoted 10 lines
include/asm-generic/dma-mapping-common.h +7 -0
 #endif
 
+#ifndef HAVE_ARCH_DMA_GET_PAGE_SHIFT
+static inline unsigned long dma_get_page_shift(struct device *dev)
+{
+	return PAGE_SHIFT;
+}
+#endif
+
 #endif

Adding Arnd who is asm-generic maintainer.

Arnd/Christoph can one of you give us an ack for this patch?

cheers

#29 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

In reply to Nishanth Aravamudan

Do we need a function here or can we just have a IOMMU_PAGE_SHIFT define with an #ifndef in common code?

Also not all architectures use dma-mapping-common.h yet, so you either need to update all of those as well, or just add the #ifndef directly to linux/dma-mapping.h.

build bot
#19 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

In reply to Michael Ellerman
Yes sorry, the patchset was not detected correctly in your case, ending up the patches being tested as individual ones.

On Tue, Oct 06, 2015@02:39:06PM +1100, Michael Ellerman wrote:

> On Sat, 2015-10-03@04:33 +0800, kbuild test robot wrote:
> > Hi Nishanth,
> > 
> > [auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
> > 
> > config: powerpc-defconfig (attached as .config)
> > reproduce:
> >         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         make.cross ARCH=powerpc 
> > 
> > All error/warnings (new ones prefixed by >>):
> > 
> >    arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':
> > >> arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'
> >       ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;
> 
> It was added in patch 3/5, so I think this error is bogus. Unless there's a
> typo I'm missing?

Yes sorry, the patchset was not detected correctly in your case, ending up the patches being tested as individual ones.

I'll fixup the robot.

Thanks,
Fengguang

reviewer
#20 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

Quoted 19 lines · Fengguang Wu
Quoted 18 lines · Michael Ellerman
Quoted 16 lines · kbuild test robot

Hi Nishanth,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: powerpc-defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc

All error/warnings (new ones prefixed by >>):

arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':

Quoted 1 line
Quoted 1 line

arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'

      ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;
Quoted 7 lines
Quoted 2 lines

It was added in patch 3/5, so I think this error is bogus. Unless there's a typo I'm missing?

Yes sorry, the patchset was not detected correctly in your case, ending up the patches being tested as individual ones.

I'll fixup the robot.

Thanks.

How did the robot decide to build this series in the first place? Does it build everything sent to one of the lists on CC?

cheers

build bot
#21 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

Yes, currently the following mailing lists are subscribed. Patches sent to them will be tested if they can be git-am to RC or linux-next kernels:

On Thu, Oct 08, 2015@11:11:59AM +1100, Michael Ellerman wrote:

> On Wed, 2015-10-07@21:56 +0800, Fengguang Wu wrote:
> > On Tue, Oct 06, 2015@02:39:06PM +1100, Michael Ellerman wrote:
> > > On Sat, 2015-10-03@04:33 +0800, kbuild test robot wrote:
> > > > Hi Nishanth,
> > > > 
> > > > [auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
> > > > 
> > > > config: powerpc-defconfig (attached as .config)
> > > > reproduce:
> > > >         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         # save the attached .config to linux build tree
> > > >         make.cross ARCH=powerpc 
> > > > 
> > > > All error/warnings (new ones prefixed by >>):
> > > > 
> > > >    arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':
> > > > >> arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'
> > > >       ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;
> > > 
> > > It was added in patch 3/5, so I think this error is bogus. Unless there's a
> > > typo I'm missing?
> > 
> > Yes sorry, the patchset was not detected correctly in your case,
> > ending up the patches being tested as individual ones.
> > 
> > I'll fixup the robot.
> 
> Thanks.
> 
> How did the robot decide to build this series in the first place? Does it build
> everything sent to one of the lists on CC?

Yes, currently the following mailing lists are subscribed. Patches sent to them will be tested if they can be git-am to RC or linux-next kernels:

kvm
linux-acpi
linux-bluetooth
linux-btrfs
linux-embedded
linux-ext4
linux-fsdevel
linux-media
linux-mmc
linux-nfs
linux-omap
linux-pci
linux-pm
linux-raid
linux-rdma
linux-scsi
linux-usb
linux-wireless
netdev
netfilter-devel
linux-mm
driverdev-devel
intel-gfx

Thanks,
Fengguang

build bot
#22 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

And of course linux-kernel. More lists could be added in future.

On Thu, Oct 08, 2015@09:06:03AM +0800, Fengguang Wu wrote:

> On Thu, Oct 08, 2015@11:11:59AM +1100, Michael Ellerman wrote:
> > On Wed, 2015-10-07@21:56 +0800, Fengguang Wu wrote:
> > > On Tue, Oct 06, 2015@02:39:06PM +1100, Michael Ellerman wrote:
> > > > On Sat, 2015-10-03@04:33 +0800, kbuild test robot wrote:
> > > > > Hi Nishanth,
> > > > > 
> > > > > [auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
> > > > > 
> > > > > config: powerpc-defconfig (attached as .config)
> > > > > reproduce:
> > > > >         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> > > > >         chmod +x ~/bin/make.cross
> > > > >         # save the attached .config to linux build tree
> > > > >         make.cross ARCH=powerpc 
> > > > > 
> > > > > All error/warnings (new ones prefixed by >>):
> > > > > 
> > > > >    arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':
> > > > > >> arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'
> > > > >       ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;
> > > > 
> > > > It was added in patch 3/5, so I think this error is bogus. Unless there's a
> > > > typo I'm missing?
> > > 
> > > Yes sorry, the patchset was not detected correctly in your case,
> > > ending up the patches being tested as individual ones.
> > > 
> > > I'll fixup the robot.
> > 
> > Thanks.
> > 
> > How did the robot decide to build this series in the first place? Does it build
> > everything sent to one of the lists on CC?
> 
> Yes, currently the following mailing lists are subscribed.  Patches
> sent to them will be tested if they can be git-am to RC or linux-next
> kernels:
> 
> kvm
> linux-acpi
> linux-bluetooth
> linux-btrfs
> linux-embedded
> linux-ext4
> linux-fsdevel
> linux-media
> linux-mmc
> linux-nfs
> linux-omap
> linux-pci
> linux-pm
> linux-raid
> linux-rdma
> linux-scsi
> linux-usb
> linux-wireless
> netdev
> netfilter-devel
> linux-mm
> driverdev-devel
> intel-gfx

And of course linux-kernel. More lists could be added in future.

Thanks,
Fengguang

reviewer
#23 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

Quoted 63 lines · Fengguang Wu
Quoted 53 lines · Fengguang Wu
Quoted 26 lines · Michael Ellerman
Quoted 25 lines · Fengguang Wu
Quoted 12 lines · Michael Ellerman
Quoted 11 lines · kbuild test robot

Hi Nishanth,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: powerpc-defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc

Quoted 8 lines
Quoted 5 lines

All error/warnings (new ones prefixed by >>):

arch/powerpc/platforms/pseries/iommu.c: In function 'iommu_init_early_pSeries':

Quoted 1 line
Quoted 1 line

arch/powerpc/platforms/pseries/iommu.c:1433:9: error: 'struct machdep_calls' has no member named 'dma_get_page_shift'

      ppc_md.dma_get_page_shift = dma_get_page_shift_pSeriesLP;

It was added in patch 3/5, so I think this error is bogus. Unless there's a typo I'm missing?

Yes sorry, the patchset was not detected correctly in your case, ending up the patches being tested as individual ones.

Quoted 6 lines
Quoted 1 line

I'll fixup the robot.

Thanks.

How did the robot decide to build this series in the first place? Does it build everything sent to one of the lists on CC?

Yes, currently the following mailing lists are subscribed. Patches sent to them will be tested if they can be git-am to RC or linux-next kernels:

kvm
linux-acpi
linux-bluetooth
linux-btrfs
linux-embedded
linux-ext4
linux-fsdevel
linux-media
linux-mmc
linux-nfs
linux-omap
linux-pci
linux-pm
linux-raid
linux-rdma

Quoted 8 lines

linux-scsi
linux-usb
linux-wireless
netdev
netfilter-devel
linux-mm
driverdev-devel
intel-gfx

Quoted 1 line

And of course linux-kernel. More lists could be added in future.

So do you mind adding linuxppc-dev at lists.ozlabs.org ? :)

It's pretty low traffic compared to lkml.

cheers

#26 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

In reply to Fengguang Wu

Hi Fengguang,

I think this proactive testing does a little more harm than good in it's current form. While offering testing for patches that aren't in git trees and or by people that don't even have a git tree that the build bots known about does seem useful, blindly doing it for every patch against something that most likely isn't the right base seems counter intertuitive. We'll probaby need some annotation in the O/n mail that asks for a test and sets a base tree to actually make it useful. With those few tweaks it should be really useful!

Maybe we should have a discussion about this at kernel summit?

Quoted 4 lines · Fengguang Wu

And of course linux-kernel. More lists could be added in future.

Thanks,
Fengguang

---end quoted text---

build bot
#27 lore raw

testing email patches

Yes that may be a good topic for gathering feedbacks and ideas for improving this useful but messy testing feature.

Hi Christoph,

On Thu, Oct 08, 2015@12:46:16AM -0700, Christoph Hellwig wrote:

> Hi Fengguang,
> 
> I think this proactive testing does a little more harm than good in
> it's current form.  While offering testing for patches that aren't in
> git trees and or by people that don't even have a git tree that the
> build bots known about does seem useful, blindly doing it for every
> patch against something that most likely isn't the right base seems
> counter intertuitive.  We'll probaby need some annotation in the O/n
> mail that asks for a test and sets a base tree to actually make it
> useful.  With those few tweaks it should be really useful!
> 
> Maybe we should have a discussion about this at kernel summit?

Yes that may be a good topic for gathering feedbacks and ideas for improving this useful but messy testing feature.

The best option could be to define a way to annotate the base tree's branch/commit of a patchset and possibly automate the annotation in the git/quilt email clients. Currently git does generate

        index c933675..a2aa0cd

for each diff files, however I find it far from enough -- there are ~50000 files in the kernel tree, knowing hash numbers for the typical 1-100 files a patchset may touch is pretty helpless in narrowing down the search space of possible base trees.

It may also be useful to specify whether or not a patchset needs such kind of testing. That'd be an easier task -- the robot can detect some magic words in the email and take action accordingly. And it can auto skip testing in some cases -- for example, Peter Zijlstra offered a good point to skip testing patches without "Signed-off-by:" in a "Re:" email. The patches already tested as git tree commits in the 500+ git trees the 0day robot monitors will be auto skipped, too.

Also we could test against both mainline and linux-next -- an excellent suggestion from Dan Carpenter, which is just implemented, thanks!

Not knowing the exact base tree leads to inherent messiness, and the possible solutions/workarounds are

1) annotations to tell the base tree (best option)
2) annotations & heuristics to skip tests
3) guess the suitable tree the maintainer would apply patches to
4) test on mainline & linux-next to filter out unstable build errors

(3) is kind of dirty work and enlightenments from subsystem maintainers are highly welcome -- typically the suggestions can be offered when you find a bug report to be unsuitable.

As for now I'm trying to teach 0day robot to apply patches to the matching subsystem's "for-next" branch based on information in the MAINTAINERS file.

Thanks,
Fengguang

> On Thu, Oct 08, 2015@09:16:09AM +0800, Fengguang Wu wrote:
> > And of course linux-kernel. More lists could be added in future.
> > 
> > Thanks,
> > Fengguang
> ---end quoted text---
> _______________________________________________
> kbuild-all mailing list
> kbuild-all at lists.01.org
> https://lists.01.org/mailman/listinfo/kbuild-all
build bot
#24 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

In reply to Michael Ellerman
Sure, I'm subscribing to it. :)

Hi Michael,

> > > > How did the robot decide to build this series in the first place? Does it build
> > > > everything sent to one of the lists on CC?
> > > 
> > > Yes, currently the following mailing lists are subscribed.  Patches
> > > sent to them will be tested if they can be git-am to RC or linux-next
> > > kernels:
> > > 
> > > kvm
> > > linux-acpi
> > > linux-bluetooth
> > > linux-btrfs
> > > linux-embedded
> > > linux-ext4
> > > linux-fsdevel
> > > linux-media
> > > linux-mmc
> > > linux-nfs
> > > linux-omap
> > > linux-pci
> > > linux-pm
> > > linux-raid
> > > linux-rdma
> > > linux-scsi
> > > linux-usb
> > > linux-wireless
> > > netdev
> > > netfilter-devel
> > > linux-mm
> > > driverdev-devel
> > > intel-gfx
> > 
> > And of course linux-kernel. More lists could be added in future.
> 
> So do you mind adding linuxppc-dev at lists.ozlabs.org ? :)
> 
> It's pretty low traffic compared to lkml.

Sure, I'm subscribing to it. :)

Thanks,
Fengguang

reviewer
#25 lore raw

Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift

Quoted 6 lines · Fengguang Wu
Quoted 4 lines

So do you mind adding linuxppc-dev at lists.ozlabs.org ? :)

It's pretty low traffic compared to lkml.

Sure, I'm subscribing to it. :)

Thanks.

cheers

author
#8 lore raw

Re: [PATCH 1/2] powerpc/iommu: expose IOMMU page shift

In reply to David Gibson
Quoted 20 lines · David Gibson
Quoted 19 lines · Nishanth Aravamudan

We will leverage this macro in the NVMe driver, which needs to know the configured IOMMU page shift to properly configure its device's page size.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>

---
Given this is available, it seems reasonable to expose -- and it doesn't really make sense to make the driver do a log2 call on the existing IOMMU_PAGE_SIZE() value.

arch/powerpc/include/asm/iommu.h +0 -0
index ca18cff..6fdf857 100644
@@ -36,6 +36,7 @@
 #define IOMMU_PAGE_MASK_4K       (~((1 << IOMMU_PAGE_SHIFT_4K) - 1))
 #define IOMMU_PAGE_ALIGN_4K(addr) _ALIGN_UP(addr, IOMMU_PAGE_SIZE_4K)
Quoted 5 lines

arch/powerpc/include/asm/iommu.h line 40

Quoted 2 lines
arch/powerpc/include/asm/iommu.h +1 -0
+#define IOMMU_PAGE_SHIFT(tblptr) (tblptr)->it_page_shift
 #define IOMMU_PAGE_SIZE(tblptr) (ASM_CONST(1) << (tblptr)->it_page_shift)

Seems like it would be a touch safer to alter IOMMU_PAGE_SIZE so it uses the new IOMMU_PAGE_SHIFT macro.

Yes absolutely! Sorry, I initially didn't add the first macro, so didn't think that through. Will update.

-Nish

author
#30 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

In reply to Christoph Hellwig
Quoted 2 lines · Christoph Hellwig

Do we need a function here or can we just have a IOMMU_PAGE_SHIFT define with an #ifndef in common code?

I suppose we could do that -- I wasn't sure if the macro would be palatable.

Quoted 3 lines

Also not all architectures use dma-mapping-common.h yet, so you either need to update all of those as well, or just add the #ifndef directly to linux/dma-mapping.h.

Ok, I will take a look at that and spin up a v3.

Thanks for the feedback!

-Nish

author
#31 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

In reply to Christoph Hellwig
Quoted 2 lines · Christoph Hellwig

Do we need a function here or can we just have a IOMMU_PAGE_SHIFT define with an #ifndef in common code?

On Power, since it's technically variable, we'd need a function. So are you suggesting define'ing it to a function just on Power and leaving it a constant elsewhere?

I noticed that sparc has a IOMMU_PAGE_SHIFT already, fwiw.

-Nish

author
#9 lore raw

Re: [PATCH 1/2] powerpc/iommu: expose IOMMU page shift

In reply to Nishanth Aravamudan
Quoted 21 lines · Nishanth Aravamudan
Quoted 20 lines · David Gibson
Quoted 19 lines · Nishanth Aravamudan

We will leverage this macro in the NVMe driver, which needs to know the configured IOMMU page shift to properly configure its device's page size.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>

---
Given this is available, it seems reasonable to expose -- and it doesn't really make sense to make the driver do a log2 call on the existing IOMMU_PAGE_SIZE() value.

arch/powerpc/include/asm/iommu.h +0 -0
index ca18cff..6fdf857 100644
@@ -36,6 +36,7 @@
 #define IOMMU_PAGE_MASK_4K       (~((1 << IOMMU_PAGE_SHIFT_4K) - 1))
 #define IOMMU_PAGE_ALIGN_4K(addr) _ALIGN_UP(addr, IOMMU_PAGE_SIZE_4K)
Quoted 8 lines
Quoted 5 lines

arch/powerpc/include/asm/iommu.h line 40

Quoted 2 lines
arch/powerpc/include/asm/iommu.h +1 -0
+#define IOMMU_PAGE_SHIFT(tblptr) (tblptr)->it_page_shift
 #define IOMMU_PAGE_SIZE(tblptr) (ASM_CONST(1) << (tblptr)->it_page_shift)

Seems like it would be a touch safer to alter IOMMU_PAGE_SIZE so it uses the new IOMMU_PAGE_SHIFT macro.

Yes absolutely! Sorry, I initially didn't add the first macro, so didn't think that through. Will update.

Err, replied too quickly -- just got back from vacation -- this is from an old version of the patchset, we no longer introduce this macro.

-Nish

author
#32 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

In reply to Nishanth Aravamudan

Hi Christoph,

Quoted 9 lines · Nishanth Aravamudan
Quoted 2 lines · Christoph Hellwig

Do we need a function here or can we just have a IOMMU_PAGE_SHIFT define with an #ifndef in common code?

On Power, since it's technically variable, we'd need a function. So are you suggesting define'ing it to a function just on Power and leaving it a constant elsewhere?

I noticed that sparc has a IOMMU_PAGE_SHIFT already, fwiw.

Sorry, I should have been more specific -- I'm ready to spin out a v3, with a sparc-specific function.

Are you ok with leaving it a function for now (the only caller is in NVMe obviously).

-Nish

#33 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

Hi Nishanth,

sorry for the late reply.

Quoted 11 lines
Quoted 5 lines

On Power, since it's technically variable, we'd need a function. So are you suggesting define'ing it to a function just on Power and leaving it a constant elsewhere?

I noticed that sparc has a IOMMU_PAGE_SHIFT already, fwiw.

Sorry, I should have been more specific -- I'm ready to spin out a v3, with a sparc-specific function.

Are you ok with leaving it a function for now (the only caller is in NVMe obviously).

I guess we do indeed need a function then. I'll take a look at your patch, but as long you found a way to avoid adding too much boilerplate code it should be fine.

author
#34 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

Quoted 13 lines · Christoph Hellwig

Hi Nishanth,

sorry for the late reply.

Quoted 9 lines
Quoted 5 lines

On Power, since it's technically variable, we'd need a function. So are you suggesting define'ing it to a function just on Power and leaving it a constant elsewhere?

I noticed that sparc has a IOMMU_PAGE_SHIFT already, fwiw.

Sorry, I should have been more specific -- I'm ready to spin out a v3, with a sparc-specific function.

Quoted 7 lines
Quoted 2 lines

Are you ok with leaving it a function for now (the only caller is in NVMe obviously).

I guess we do indeed need a function then. I'll take a look at your patch, but as long you found a way to avoid adding too much boilerplate code it should be fine.

Ok, so I've got the moved function (include/linux/dma-mapping.h instead of dma-mapping-common.h) ready to go, which should only involve changing the first patch in the series. But I'm really mystified by what to do for sparc, which defines IOMMU_PAGE_SHIFT and IO_PAGE_SHIFT in arch/sparc/kernel/iommu_common.h.

1) Which constant reflects the value we mean for this function on sparc? I assume it should be IOMMU_PAGE_SHIFT, but they are the same value and I want to make sure I get the semantics right.

2) Where would I put sparc's definition of dma_get_page_shift()? Should it be in a asm/dma-mapping.h? Should we move some of the constants from arch/sparc/kernel/iommu_common.h to arch/sparc/include/asm/iommu_common.h and then #include that in asm/dma-mapping.h?

Dave M., any opinions/insights? Essentially, this helper function assists the NVMe driver in determining what page size it should use to satisfy both the device and IOMMU's requirements. Maybe I misunderstand the constants on sparc and PAGE_SHIFT is fine there too?

-Nish

author
#35 lore raw

Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API

Quoted 38 lines · Nishanth Aravamudan
Quoted 13 lines · Christoph Hellwig

Hi Nishanth,

sorry for the late reply.

Quoted 9 lines
Quoted 5 lines

On Power, since it's technically variable, we'd need a function. So are you suggesting define'ing it to a function just on Power and leaving it a constant elsewhere?

I noticed that sparc has a IOMMU_PAGE_SHIFT already, fwiw.

Sorry, I should have been more specific -- I'm ready to spin out a v3, with a sparc-specific function.

Quoted 7 lines
Quoted 2 lines

Are you ok with leaving it a function for now (the only caller is in NVMe obviously).

I guess we do indeed need a function then. I'll take a look at your patch, but as long you found a way to avoid adding too much boilerplate code it should be fine.

Ok, so I've got the moved function (include/linux/dma-mapping.h instead of dma-mapping-common.h) ready to go, which should only involve changing the first patch in the series. But I'm really mystified by what to do for sparc, which defines IOMMU_PAGE_SHIFT and IO_PAGE_SHIFT in arch/sparc/kernel/iommu_common.h.

1) Which constant reflects the value we mean for this function on sparc? I assume it should be IOMMU_PAGE_SHIFT, but they are the same value and I want to make sure I get the semantics right.

2) Where would I put sparc's definition of dma_get_page_shift()? Should it be in a asm/dma-mapping.h? Should we move some of the constants from arch/sparc/kernel/iommu_common.h to arch/sparc/include/asm/iommu_common.h and then #include that in asm/dma-mapping.h?

Quoted 4 lines

Dave M., any opinions/insights? Essentially, this helper function assists the NVMe driver in determining what page size it should use to satisfy both the device and IOMMU's requirements. Maybe I misunderstand the constants on sparc and PAGE_SHIFT is fine there too?

Anyone with sparc knowledge have an opinion/input?

-Nish