Skip to contentSkip to conversation

Fix NVMe driver support on Power with 32-bit DMA

PATCH Superseded Nishanth Aravamudan 7 participants 49 messages Oct 23, 2015 – Nov 18, 2015

Subsystems (5)
PowerPC Block layer NVMe SPARC + UltraSPARC (sparc/sparc64) +1 more subsystems
lore
Share & tools
lore

Nishanth Aravamudan

author
#0 lore raw

v3 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 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) move some sparc code around to make IOMMU_PAGE_SHIFT available in include/asm
6) override the generic implementation on sparce to use IOMMU_PAGE_SHIFT
7) 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 ++++++++++++++++++++++++++++++++++++
 arch/sparc/include/asm/dma-mapping.h   |  8 ++++++++
 arch/sparc/include/asm/iommu_common.h  | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/sparc/kernel/iommu.c              |  2 +-
 arch/sparc/kernel/iommu_common.h       | 51 ---------------------------------------------------
 arch/sparc/kernel/pci_psycho.c         |  2 +-
 arch/sparc/kernel/pci_sabre.c          |  2 +-
 arch/sparc/kernel/pci_schizo.c         |  2 +-
 arch/sparc/kernel/pci_sun4v.c          |  2 +-
 arch/sparc/kernel/psycho_common.c      |  2 +-
 arch/sparc/kernel/sbus.c               |  3 +--
 drivers/block/nvme-core.c              |  3 ++-
 include/linux/dma-mapping.h            |  7 +++++++
 16 files changed, 127 insertions(+), 61 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.
v2 -> v3:
Based upon feedback from Christoph Hellwig, put the generic implementation in include/linux/dma-mapping.h, since not all archs use include/asm-generic/dma-mapping-common.h.
Add sparc implementation, as that arch seems to have a different IOMMU page size.

author
#1 lore raw

PATCH v3 1/7 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>

---
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.
v2 -> v3:
Based upon feedback from Christoph Hellwig that not all architectures have moved to dma-mapping-common.h, so move the #ifdef and default to linux/dma-mapping.h.

---
 include/linux/dma-mapping.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/dma-mapping.h +7 -0
index ac07ff0..7eaba8d 100644
@@ -95,6 +95,13 @@ static inline u64 dma_get_mask(struct device *dev)
 	return DMA_BIT_MASK(32);
 }
 
+#ifndef HAVE_ARCH_DMA_GET_PAGE_SHIFT
+static inline unsigned long dma_get_page_shift(struct device *dev)
+{
+	return PAGE_SHIFT;
+}
+#endif
+
 #ifdef CONFIG_ARCH_HAS_DMA_SET_COHERENT_MASK
 int dma_set_coherent_mask(struct device *dev, u64 mask);
 #else
Signature · 1 line
1.9.1
author
#2 lore raw

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

In reply to Nishanth Aravamudan

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 +++
 arch/powerpc/kernel/dma.c              | 9 +++++++++
 2 files changed, 12 insertions(+)
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);
Signature · 1 line
1.9.1
author
#11 lore raw
In reply to Nishanth Aravamudan

[Sorry, subject should have been 0/7!]

Quoted 63 lines · 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) move some sparc code around to make IOMMU_PAGE_SHIFT available in include/asm
6) override the generic implementation on sparce to use IOMMU_PAGE_SHIFT
7) 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 ++++++++++++++++++++++++++++++++++++
 arch/sparc/include/asm/dma-mapping.h   |  8 ++++++++
 arch/sparc/include/asm/iommu_common.h  | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/sparc/kernel/iommu.c              |  2 +-
 arch/sparc/kernel/iommu_common.h       | 51 ---------------------------------------------------
 arch/sparc/kernel/pci_psycho.c         |  2 +-
 arch/sparc/kernel/pci_sabre.c          |  2 +-
 arch/sparc/kernel/pci_schizo.c         |  2 +-
 arch/sparc/kernel/pci_sun4v.c          |  2 +-
 arch/sparc/kernel/psycho_common.c      |  2 +-
 arch/sparc/kernel/sbus.c               |  3 +--
 drivers/block/nvme-core.c              |  3 ++-
 include/linux/dma-mapping.h            |  7 +++++++
 16 files changed, 127 insertions(+), 61 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.
v2 -> v3:
Based upon feedback from Christoph Hellwig, put the generic implementation in include/linux/dma-mapping.h, since not all archs use include/asm-generic/dma-mapping-common.h.
Add sparc implementation, as that arch seems to have a different IOMMU page size.

author
#12 lore raw

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

In reply to Nishanth Aravamudan

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 | 3 ++-
 arch/powerpc/kernel/dma.c          | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)
arch/powerpc/include/asm/machdep.h +2 -1
index 3f191f5..9dd03cf 100644
@@ -83,9 +83,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;
Signature · 1 line
1.9.1
author
#13 lore raw

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

In reply to Nishanth Aravamudan

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 ++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
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;
Signature · 1 line
1.9.1
author
#16 lore raw

RFC PATCH 5/7 sparc: rename kernel/iommu_common.h -> include/asm/iommu_common.h

In reply to Nishanth Aravamudan

In order to cleanly expose the desired IOMMU page shift via the new dma_get_page_shift API, we need to have the sparc constants available in a more typical location. There should be no functional impact to this move, but it is untested.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
---
 arch/sparc/include/asm/iommu_common.h | 51 +++++++++++++++++++++++++++++++++++
 arch/sparc/kernel/iommu.c             |  2 +-
 arch/sparc/kernel/iommu_common.h      | 51 -----------------------------------
 arch/sparc/kernel/pci_psycho.c        |  2 +-
 arch/sparc/kernel/pci_sabre.c         |  2 +-
 arch/sparc/kernel/pci_schizo.c        |  2 +-
 arch/sparc/kernel/pci_sun4v.c         |  2 +-
 arch/sparc/kernel/psycho_common.c     |  2 +-
 arch/sparc/kernel/sbus.c              |  3 +--
 9 files changed, 58 insertions(+), 59 deletions(-)
 create mode 100644 arch/sparc/include/asm/iommu_common.h
 delete mode 100644 arch/sparc/kernel/iommu_common.h
arch/sparc/include/asm/iommu_common.h new file +51 -0
new file mode 100644
index 0000000..b40cec2
@@ -0,0 +1,51 @@
+/* iommu_common.h: UltraSparc SBUS/PCI common iommu declarations.
+ *
+ * Copyright (C) 1999, 2008 David S. Miller (davem at davemloft.net)
+ */
+
+#ifndef _IOMMU_COMMON_H
+#define _IOMMU_COMMON_H
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+#include <linux/mm.h>
+#include <linux/scatterlist.h>
+#include <linux/device.h>
+#include <linux/iommu-helper.h>
+#include <linux/scatterlist.h>
+
+#include <asm/iommu.h>
+
+/*
+ * These give mapping size of each iommu pte/tlb.
+ */
+#define IO_PAGE_SHIFT			13
+#define IO_PAGE_SIZE			(1UL << IO_PAGE_SHIFT)
+#define IO_PAGE_MASK			(~(IO_PAGE_SIZE-1))
+#define IO_PAGE_ALIGN(addr)		ALIGN(addr, IO_PAGE_SIZE)
+
+#define IO_TSB_ENTRIES			(128*1024)
+#define IO_TSB_SIZE			(IO_TSB_ENTRIES * 8)
+
+/*
+ * This is the hardwired shift in the iotlb tag/data parts.
+ */
+#define IOMMU_PAGE_SHIFT		13
+
+#define SG_ENT_PHYS_ADDRESS(SG)	(__pa(sg_virt((SG))))
+
+static inline int is_span_boundary(unsigned long entry,
+				   unsigned long shift,
+				   unsigned long boundary_size,
+				   struct scatterlist *outs,
+				   struct scatterlist *sg)
+{
+	unsigned long paddr = SG_ENT_PHYS_ADDRESS(outs);
+	int nr = iommu_num_pages(paddr, outs->dma_length + sg->length,
+				 IO_PAGE_SIZE);
+
+	return iommu_is_span_boundary(entry, nr, shift, boundary_size);
+}
+
+#endif /* _IOMMU_COMMON_H */
arch/sparc/kernel/iommu.c +1 -1
index 5320689..f9b6077 100644
@@ -20,8 +20,8 @@
 #endif
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 
-#include "iommu_common.h"
 #include "kernel.h"
 
 #define STC_CTXMATCH_ADDR(STC, CTX)	\
arch/sparc/kernel/iommu_common.h deleted +0 -51

51 lines not on this page · raw message

arch/sparc/kernel/pci_psycho.c +1 -1
index 7dce27b..332fd6f 100644
@@ -15,13 +15,13 @@
 #include <linux/of_device.h>
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/starfire.h>
 #include <asm/prom.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 #include "psycho_common.h"
 
 #define DRIVER_NAME	"psycho"
arch/sparc/kernel/pci_sabre.c +1 -1
index 00a616f..6c2c616 100644
@@ -16,12 +16,12 @@
 
 #include <asm/apb.h>
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/prom.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 #include "psycho_common.h"
 
 #define DRIVER_NAME	"sabre"
arch/sparc/kernel/pci_schizo.c +1 -1
index c664d3e..ef23ab2 100644
@@ -13,13 +13,13 @@
 #include <linux/of_device.h>
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/pstate.h>
 #include <asm/prom.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 
 #define DRIVER_NAME	"schizo"
 #define PFX		DRIVER_NAME ": "
arch/sparc/kernel/pci_sun4v.c +1 -1
index d2fe57d..1d72295 100644
@@ -18,12 +18,12 @@
 #include <linux/iommu-common.h>
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/hypervisor.h>
 #include <asm/prom.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 
 #include "pci_sun4v.h"
arch/sparc/kernel/psycho_common.c +1 -1
index 8db48e8..8eec09d 100644
@@ -5,10 +5,10 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 
+#include <asm/iommu_common.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 #include "psycho_common.h"
 
 #define  PSYCHO_STRBUF_CTRL_DENAB	0x0000000000000002ULL
arch/sparc/kernel/sbus.c +1 -2
index be5bdf9..a92cdf6 100644
@@ -24,8 +24,7 @@
 #include <asm/prom.h>
 #include <asm/oplib.h>
 #include <asm/starfire.h>
-
-#include "iommu_common.h"
+#include <asm/iommu_common.h>
 
 #define MAP_BASE	((u32)0xc0000000)
Signature · 1 line
1.9.1
author
#18 lore raw

RFC PATCH 6/7 sparc/dma-mapping: override dma_get_page_shift

In reply to Nishanth Aravamudan

On sparc, the kernel's page size differs from the IOMMU's page size, so override the generic implementation, which always returns the kernel's page size, and return IOMMU_PAGE_SHIFT instead.

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

---
I know very little about sparc, so please correct me if this patch is invalid.

 arch/sparc/include/asm/dma-mapping.h | 8 ++++++++
 1 file changed, 8 insertions(+)
arch/sparc/include/asm/dma-mapping.h +8 -0
index a21da59..76ba470 100644
@@ -52,6 +52,14 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
 	return -EINVAL;
 }
 
+/* for IOMMU_PAGE_SHIFT */
+#include <asm/iommu_common.h>
+#define HAVE_ARCH_DMA_GET_PAGE_SHIFT 1
+static inline unsigned long dma_get_page_shift(struct device *dev)
+{
+	return IOMMU_PAGE_SHIFT;
+}
+
 #include <asm-generic/dma-mapping-common.h>
 
 #endif
Signature · 1 line
1.9.1
author
#19 lore raw

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

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 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.

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

---
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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/block/nvme-core.c +2 -1
index 6f04771..5a79106 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>

@@ -1711,7 +1712,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;
Signature · 1 line
1.9.1
author
#17 lore raw

Re: [PATCH 5/7] [RFC PATCH 5/7] sparc: rename kernel/iommu_common.h -> include/asm/iommu_common.h

In reply to Nishanth Aravamudan

[Apologies for the subject line, should just have the [RFC PATCH 5/7]]

Quoted 255 lines · Nishanth Aravamudan

In order to cleanly expose the desired IOMMU page shift via the new dma_get_page_shift API, we need to have the sparc constants available in a more typical location. There should be no functional impact to this move, but it is untested.

Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
---
 arch/sparc/include/asm/iommu_common.h | 51 +++++++++++++++++++++++++++++++++++
 arch/sparc/kernel/iommu.c             |  2 +-
 arch/sparc/kernel/iommu_common.h      | 51 -----------------------------------
 arch/sparc/kernel/pci_psycho.c        |  2 +-
 arch/sparc/kernel/pci_sabre.c         |  2 +-
 arch/sparc/kernel/pci_schizo.c        |  2 +-
 arch/sparc/kernel/pci_sun4v.c         |  2 +-
 arch/sparc/kernel/psycho_common.c     |  2 +-
 arch/sparc/kernel/sbus.c              |  3 +--
 9 files changed, 58 insertions(+), 59 deletions(-)
 create mode 100644 arch/sparc/include/asm/iommu_common.h
 delete mode 100644 arch/sparc/kernel/iommu_common.h
arch/sparc/include/asm/iommu_common.h new file +51 -0
new file mode 100644
index 0000000..b40cec2
@@ -0,0 +1,51 @@
+/* iommu_common.h: UltraSparc SBUS/PCI common iommu declarations.
+ *
+ * Copyright (C) 1999, 2008 David S. Miller (davem at davemloft.net)
+ */
+
+#ifndef _IOMMU_COMMON_H
+#define _IOMMU_COMMON_H
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+#include <linux/mm.h>
+#include <linux/scatterlist.h>
+#include <linux/device.h>
+#include <linux/iommu-helper.h>
+#include <linux/scatterlist.h>
+
+#include <asm/iommu.h>
+
+/*
+ * These give mapping size of each iommu pte/tlb.
+ */
+#define IO_PAGE_SHIFT			13
+#define IO_PAGE_SIZE			(1UL << IO_PAGE_SHIFT)
+#define IO_PAGE_MASK			(~(IO_PAGE_SIZE-1))
+#define IO_PAGE_ALIGN(addr)		ALIGN(addr, IO_PAGE_SIZE)
+
+#define IO_TSB_ENTRIES			(128*1024)
+#define IO_TSB_SIZE			(IO_TSB_ENTRIES * 8)
+
+/*
+ * This is the hardwired shift in the iotlb tag/data parts.
+ */
+#define IOMMU_PAGE_SHIFT		13
+
+#define SG_ENT_PHYS_ADDRESS(SG)	(__pa(sg_virt((SG))))
+
+static inline int is_span_boundary(unsigned long entry,
+				   unsigned long shift,
+				   unsigned long boundary_size,
+				   struct scatterlist *outs,
+				   struct scatterlist *sg)
+{
+	unsigned long paddr = SG_ENT_PHYS_ADDRESS(outs);
+	int nr = iommu_num_pages(paddr, outs->dma_length + sg->length,
+				 IO_PAGE_SIZE);
+
+	return iommu_is_span_boundary(entry, nr, shift, boundary_size);
+}
+
+#endif /* _IOMMU_COMMON_H */
arch/sparc/kernel/iommu.c +1 -1
index 5320689..f9b6077 100644
@@ -20,8 +20,8 @@
 #endif
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 
-#include "iommu_common.h"
 #include "kernel.h"
 
 #define STC_CTXMATCH_ADDR(STC, CTX)	\
arch/sparc/kernel/iommu_common.h deleted +0 -51
deleted file mode 100644
index b40cec2..0000000
@@ -1,51 +0,0 @@
-/* iommu_common.h: UltraSparc SBUS/PCI common iommu declarations.
- *
- * Copyright (C) 1999, 2008 David S. Miller (davem at davemloft.net)
- */
-
-#ifndef _IOMMU_COMMON_H
-#define _IOMMU_COMMON_H
-
-#include <linux/kernel.h>
-#include <linux/types.h>
-#include <linux/sched.h>
-#include <linux/mm.h>
-#include <linux/scatterlist.h>
-#include <linux/device.h>
-#include <linux/iommu-helper.h>
-#include <linux/scatterlist.h>
-
-#include <asm/iommu.h>
-
-/*
- * These give mapping size of each iommu pte/tlb.
- */
-#define IO_PAGE_SHIFT			13
-#define IO_PAGE_SIZE			(1UL << IO_PAGE_SHIFT)
-#define IO_PAGE_MASK			(~(IO_PAGE_SIZE-1))
-#define IO_PAGE_ALIGN(addr)		ALIGN(addr, IO_PAGE_SIZE)
-
-#define IO_TSB_ENTRIES			(128*1024)
-#define IO_TSB_SIZE			(IO_TSB_ENTRIES * 8)
-
-/*
- * This is the hardwired shift in the iotlb tag/data parts.
- */
-#define IOMMU_PAGE_SHIFT		13
-
-#define SG_ENT_PHYS_ADDRESS(SG)	(__pa(sg_virt((SG))))
-
-static inline int is_span_boundary(unsigned long entry,
-				   unsigned long shift,
-				   unsigned long boundary_size,
-				   struct scatterlist *outs,
-				   struct scatterlist *sg)
-{
-	unsigned long paddr = SG_ENT_PHYS_ADDRESS(outs);
-	int nr = iommu_num_pages(paddr, outs->dma_length + sg->length,
-				 IO_PAGE_SIZE);
-
-	return iommu_is_span_boundary(entry, nr, shift, boundary_size);
-}
-
-#endif /* _IOMMU_COMMON_H */
arch/sparc/kernel/pci_psycho.c +1 -1
index 7dce27b..332fd6f 100644
@@ -15,13 +15,13 @@
 #include <linux/of_device.h>
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/starfire.h>
 #include <asm/prom.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 #include "psycho_common.h"
 
 #define DRIVER_NAME	"psycho"
arch/sparc/kernel/pci_sabre.c +1 -1
index 00a616f..6c2c616 100644
@@ -16,12 +16,12 @@
 
 #include <asm/apb.h>
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/prom.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 #include "psycho_common.h"
 
 #define DRIVER_NAME	"sabre"
arch/sparc/kernel/pci_schizo.c +1 -1
index c664d3e..ef23ab2 100644
@@ -13,13 +13,13 @@
 #include <linux/of_device.h>
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/pstate.h>
 #include <asm/prom.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 
 #define DRIVER_NAME	"schizo"
 #define PFX		DRIVER_NAME ": "
arch/sparc/kernel/pci_sun4v.c +1 -1
index d2fe57d..1d72295 100644
@@ -18,12 +18,12 @@
 #include <linux/iommu-common.h>
 
 #include <asm/iommu.h>
+#include <asm/iommu_common.h>
 #include <asm/irq.h>
 #include <asm/hypervisor.h>
 #include <asm/prom.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 
 #include "pci_sun4v.h"
arch/sparc/kernel/psycho_common.c +1 -1
index 8db48e8..8eec09d 100644
@@ -5,10 +5,10 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 
+#include <asm/iommu_common.h>
 #include <asm/upa.h>
 
 #include "pci_impl.h"
-#include "iommu_common.h"
 #include "psycho_common.h"
 
 #define  PSYCHO_STRBUF_CTRL_DENAB	0x0000000000000002ULL
arch/sparc/kernel/sbus.c +1 -2
index be5bdf9..a92cdf6 100644
@@ -24,8 +24,7 @@
 #include <asm/prom.h>
 #include <asm/oplib.h>
 #include <asm/starfire.h>
-
-#include "iommu_common.h"
+#include <asm/iommu_common.h>
 
 #define MAP_BASE	((u32)0xc0000000)
Signature · 1 line
1.9.1
maintainer
#20 lore raw
In reply to Nishanth Aravamudan

From: Nishanth Aravamudan <[email protected]>
Date: Fri, 23 Oct 2015 13:54:20 -0700

Quoted 2 lines

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

I won't object to this patch series, but if I had implemented this I would have required the architectures to implement this explicitly, one-by-one. I think it is less error prone and more likely to end up with all the architectures setting this correctly.

#14 lore raw

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

In reply to Nishanth Aravamudan
Quoted 32 lines · Nishanth Aravamudan

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 ++++++++++++++++++++++++++++++++++
  1 file changed, 36 insertions(+)
arch/powerpc/platforms/pseries/iommu.c +12 -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;

arch/powerpc/platforms/pseries/iommu.c static u64 dma_get_required_mask_pSeriesLP(struct device *dev) · line 1316

Quoted 10 lines
arch/powerpc/platforms/pseries/iommu.c +10 -0
+				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);

This DIRECT64_PROPNAME property is only present under pHyp, QEMU/KVM does not set it as 64bit windows are dynamic there so something like find_existing_ddw() needs to be used here.

Quoted 32 lines
arch/powerpc/platforms/pseries/iommu.c +14 -0
+			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;
Signature · 1 line
Alexey
#3 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

In reply to Nishanth Aravamudan
Quoted 31 lines · Nishanth Aravamudan

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 +++
  arch/powerpc/kernel/dma.c              | 9 +++++++++
  2 files changed, 12 insertions(+)
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 +0 -0
index 59503ed..e805af2 100644

arch/powerpc/kernel/dma.c int dma_set_mask(struct device *dev, u64 dma_mask) · line 342

Quoted 8 lines
arch/powerpc/kernel/dma.c +5 -0
@@ -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;

All PCI devices have this initialized on POWER (at least, our, IBM's POWER) so 4K will always be returned here while in the case of (get_dma_ops(dev)==&dma_direct_ops) it could actually return PAGE_SHIFT. Is 4K still preferred value to return here?

Quoted 8 lines
arch/powerpc/kernel/dma.c +4 -0
+	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);
Signature · 1 line
Alexey
#4 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

Quoted 11 lines · Alexey Kardashevskiy

arch/powerpc/kernel/dma.c int dma_set_mask(struct device *dev, u64 dma_mask) · line 342

Quoted 5 lines
arch/powerpc/kernel/dma.c +5 -0
+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;

All PCI devices have this initialized on POWER (at least, our, IBM's POWER) so 4K will always be returned here while in the case of (get_dma_ops(dev)==&dma_direct_ops) it could actually return PAGE_SHIFT. Is 4K still preferred value to return here?

4k is always a safe option to return, but ideally you want to return the highest guaranteed DMA address alignment. The driver just needs to know which bits to mask from virtual addresses such that the offset is the same as the DMA address.

author
#21 lore raw
In reply to David Miller
Quoted 10 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Fri, 23 Oct 2015 13:54:20 -0700

Quoted 2 lines

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

I won't object to this patch series, but if I had implemented this I would have required the architectures to implement this explicitly, one-by-one. I think it is less error prone and more likely to end up with all the architectures setting this correctly.

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

Thanks,
Nish

author
#15 lore raw

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

In reply to Alexey Kardashevskiy
Quoted 40 lines · Alexey Kardashevskiy
Quoted 39 lines · Nishanth Aravamudan

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 ++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
arch/powerpc/platforms/pseries/iommu.c +19 -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
+		 */
Quoted 8 lines

arch/powerpc/platforms/pseries/iommu.c static u64 dma_get_required_mask_pSeriesLP(struct device *dev) · line 1316

Quoted 3 lines
arch/powerpc/platforms/pseries/iommu.c +3 -0
+		if (dn && PCI_DN(dn)) {
+			const struct dynamic_dma_window_prop *direct64 =
+				of_get_property(dn, DIRECT64_PROPNAME, NULL);

This DIRECT64_PROPNAME property is only present under pHyp, QEMU/KVM does not set it as 64bit windows are dynamic there so something like find_existing_ddw() needs to be used here.

DIRECT64_PROPNAME is a Linux thing, not a pHyp or QEMU/KVM thing -- it's created by the Linux DDW logic and left in the device-tree when we successfully configure DDW.

You're right, though, that logically find_existing_ddw() would be better to use here. I'll spin up a new version.

-Nish

author
#5 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

In reply to Alexey Kardashevskiy
Quoted 35 lines · Alexey Kardashevskiy
Quoted 34 lines · Nishanth Aravamudan

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 +++
 arch/powerpc/kernel/dma.c              | 9 +++++++++
 2 files changed, 12 insertions(+)
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 +0 -0
index 59503ed..e805af2 100644
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)
 }
 EXPORT_SYMBOL(dma_set_mask);
Quoted 11 lines

arch/powerpc/kernel/dma.c int dma_set_mask(struct device *dev, u64 dma_mask) · line 342

Quoted 5 lines
arch/powerpc/kernel/dma.c +5 -0
+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;

All PCI devices have this initialized on POWER (at least, our, IBM's POWER) so 4K will always be returned here while in the case of (get_dma_ops(dev)==&dma_direct_ops) it could actually return PAGE_SHIFT. Is 4K still preferred value to return here?

Right, so the logic of my series, goes like this:

a) We currently are assuming DMA_PAGE_SHIFT (conceptual constant) is PAGE_SHIFT everywhere, including Power.

b) After 2/7, the Power code will return either the IOMMU table's shift value, if set, or PAGE_SHIFT (I guess this would be the case if get_dma_ops(dev) == &dma_direct_ops, as you said). That is no different than we have now, except we can return the accurate IOMMU value if available.

3) After 3/7, the platform can override the generic Power get_dma_page_shift().

4) After 4/7, pseries will return the DDW value, if available, then fallback to the IOMMU table's value. I think in the case of get_dma_ops(dev)==&dma_direct_ops, the only way that can happen is if we are using DDW, right?

-Nish

#22 lore raw
In reply to Nishanth Aravamudan
Quoted 12 lines · Nishanth Aravamudan
Quoted 10 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Fri, 23 Oct 2015 13:54:20 -0700

Quoted 2 lines

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

I won't object to this patch series, but if I had implemented this I would have required the architectures to implement this explicitly, one-by-one. I think it is less error prone and more likely to end up with all the architectures setting this correctly.

Quoted 6 lines

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

If you're suggesting to compile-time break architectures that currently work just fine with NVMe, let me stop you right there.

#42 lore raw
In reply to Nishanth Aravamudan

Hi Nishanth,

Quoted 12 lines · Nishanth Aravamudan
Quoted 10 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Fri, 23 Oct 2015 13:54:20 -0700

Quoted 2 lines

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

I won't object to this patch series, but if I had implemented this I would have required the architectures to implement this explicitly, one-by-one. I think it is less error prone and more likely to end up with all the architectures setting this correctly.

Quoted 6 lines

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

He's suggesting that you _don't_ put a generic implementation in /include/linux/dma-mapping.h and instead add it to _every_ architecture.

Thanks,

Signature · 4 lines
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
author
#43 lore raw
Quoted 23 lines · Julian Calaby

Hi Nishanth,

Quoted 12 lines · Nishanth Aravamudan
Quoted 10 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Fri, 23 Oct 2015 13:54:20 -0700

Quoted 2 lines

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

I won't object to this patch series, but if I had implemented this I would have required the architectures to implement this explicitly, one-by-one. I think it is less error prone and more likely to end up with all the architectures setting this correctly.

Quoted 6 lines

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

Quoted 3 lines

He's suggesting that you _don't_ put a generic implementation in /include/linux/dma-mapping.h and instead add it to _every_ architecture.

Ah, I see! Well, I don't know much about the DMA internals of most architectures -- and my approach kept things functionally the same everywhere (using PAGE_SHIFT) except:

a) Power, where I know it doesn't work as-is
and
b) sparc, where the code implied that a different value than PAGE_SHIFT should be used.

Thanks,
Nish

#44 lore raw

Hi Nishanth,

Quoted 28 lines · Nishanth Aravamudan
Quoted 23 lines · Julian Calaby

Hi Nishanth,

Quoted 12 lines · Nishanth Aravamudan
Quoted 10 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Fri, 23 Oct 2015 13:54:20 -0700

Quoted 2 lines

1) add a generic dma_get_page_shift implementation that just returns PAGE_SHIFT

I won't object to this patch series, but if I had implemented this I would have required the architectures to implement this explicitly, one-by-one. I think it is less error prone and more likely to end up with all the architectures setting this correctly.

Quoted 6 lines

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

Quoted 3 lines

He's suggesting that you _don't_ put a generic implementation in /include/linux/dma-mapping.h and instead add it to _every_ architecture.

Quoted 8 lines

Ah, I see! Well, I don't know much about the DMA internals of most architectures -- and my approach kept things functionally the same everywhere (using PAGE_SHIFT) except:

a) Power, where I know it doesn't work as-is
and
b) sparc, where the code implied that a different value than PAGE_SHIFT should be used.

You'll be CCing the maintainers of each architecture on the patches to add the functions, so if they do have specific requirements, I'm sure they'll let you know or provide patches.

Thanks,

Signature · 4 lines
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
#45 lore raw
Quoted 5 lines · Julian Calaby

Hi Nishanth,
You'll be CCing the maintainers of each architecture on the patches to
add the functions, so if they do have specific requirements, I'm sure they'll let you know or provide patches.

That sort of accross-all-arch change tend to take forever. I'd rather get the existing series in to fix the problem, we can look into improving things later but I tend to think that the default of using PAGE_SHIFT in asm-generic will be fine for most archs.

Ben.

maintainer
#47 lore raw
In reply to Nishanth Aravamudan

From: Nishanth Aravamudan <[email protected]>
Date: Tue, 27 Oct 2015 15:20:10 -0700

Quoted 6 lines

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

Yes, I am basically suggesting to simply not provide a default at all.

maintainer
#23 lore raw
In reply to Busch, Keith

From: "Busch, Keith" <[email protected]>
Date: Tue, 27 Oct 2015 22:36:43 +0000

Quoted 2 lines

If you're suggesting to compile-time break architectures that currently work just fine with NVMe, let me stop you right there.

Silently "working" without the architecture maintainer having to explicity look at the new interface and make sure his platform is implementing it properly is an extremely bad practice.

maintainer
#46 lore raw
In reply to Julian Calaby

From: Julian Calaby <[email protected]>
Date: Wed, 28 Oct 2015 10:43:35 +1100

Quoted 3 lines

You'll be CCing the maintainers of each architecture on the patches to add the functions, so if they do have specific requirements, I'm sure they'll let you know or provide patches.

People miss things, maintainers get busy, so while a CC: is important and appreciated, it doesn't ensure a correct implementation is likely to result.

Personally, I'd much rather my arch stop building so I have to go in there to explicitly look at the reason why and make sure I fill in the missing pieces properly and accurately.

#6 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

In reply to Nishanth Aravamudan
Quoted 50 lines · Nishanth Aravamudan
Quoted 35 lines · Alexey Kardashevskiy
Quoted 34 lines · Nishanth Aravamudan

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 +++
  arch/powerpc/kernel/dma.c              | 9 +++++++++
  2 files changed, 12 insertions(+)
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 +0 -0
index 59503ed..e805af2 100644
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)
  }
  EXPORT_SYMBOL(dma_set_mask);
Quoted 11 lines

arch/powerpc/kernel/dma.c int dma_set_mask(struct device *dev, u64 dma_mask) · line 342

Quoted 5 lines
arch/powerpc/kernel/dma.c +5 -0
+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;

All PCI devices have this initialized on POWER (at least, our, IBM's POWER) so 4K will always be returned here while in the case of (get_dma_ops(dev)==&dma_direct_ops) it could actually return PAGE_SHIFT. Is 4K still preferred value to return here?

Right, so the logic of my series, goes like this:

Quoted 8 lines

a) We currently are assuming DMA_PAGE_SHIFT (conceptual constant) is PAGE_SHIFT everywhere, including Power.

b) After 2/7, the Power code will return either the IOMMU table's shift value, if set, or PAGE_SHIFT (I guess this would be the case if get_dma_ops(dev) == &dma_direct_ops, as you said). That is no different than we have now, except we can return the accurate IOMMU value if available.

If it is not available, then something went wrong and BUG_ON(!tbl || !tbl->it_page_shift) make more sense here than pretending that this function can ever return PAGE_SHIFT. imho.

Quoted 8 lines

3) After 3/7, the platform can override the generic Power get_dma_page_shift().

4) After 4/7, pseries will return the DDW value, if available, then fallback to the IOMMU table's value. I think in the case of get_dma_ops(dev)==&dma_direct_ops, the only way that can happen is if we are using DDW, right?

This is for pseries guests; for the powernv host it is a "bypass" mode which does 64bit direct DMA mapping and there is no additional window for that (i.e. DIRECT64_PROPNAME, etc).

Signature · 1 line
Alexey
author
#48 lore raw
In reply to David Miller
Quoted 11 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Tue, 27 Oct 2015 15:20:10 -0700

Quoted 6 lines

Well, looks like I should spin up a v4 anyways for the powerpc changes. So, to make sure I understand your point, should I make the generic dma_get_page_shift a compile-error kind of thing? It will only fail on architectures that actually build the NVME driver (as the only caller). But I'm not sure how exactly to achieve that, if you could give a bit more detail I'd appreciate it!

Yes, I am basically suggesting to simply not provide a default at all.

For my own edification -- what is the way that gets resolved? I guess I mean it seems like linux-next would cease to compile because of my new series. Would my patches just get kicked out of -next for introducing that (or even via the 0-day notifications), or should I put something into the commit message indicating it is an API introduction?

Sorry for the tentativeness, I have not introduce a cross-architecture API like this before.

Thanks,
Nish

Quoted 1 line
author
#7 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

In reply to Alexey Kardashevskiy
Quoted 60 lines · Alexey Kardashevskiy
Quoted 50 lines · Nishanth Aravamudan
Quoted 35 lines · Alexey Kardashevskiy
Quoted 34 lines · Nishanth Aravamudan

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 +++
 arch/powerpc/kernel/dma.c              | 9 +++++++++
 2 files changed, 12 insertions(+)
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 +0 -0
index 59503ed..e805af2 100644
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)
 }
 EXPORT_SYMBOL(dma_set_mask);
Quoted 11 lines

arch/powerpc/kernel/dma.c int dma_set_mask(struct device *dev, u64 dma_mask) · line 342

Quoted 5 lines
arch/powerpc/kernel/dma.c +5 -0
+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;

All PCI devices have this initialized on POWER (at least, our, IBM's POWER) so 4K will always be returned here while in the case of (get_dma_ops(dev)==&dma_direct_ops) it could actually return PAGE_SHIFT. Is 4K still preferred value to return here?

Right, so the logic of my series, goes like this:

Quoted 8 lines

a) We currently are assuming DMA_PAGE_SHIFT (conceptual constant) is PAGE_SHIFT everywhere, including Power.

b) After 2/7, the Power code will return either the IOMMU table's shift value, if set, or PAGE_SHIFT (I guess this would be the case if get_dma_ops(dev) == &dma_direct_ops, as you said). That is no different than we have now, except we can return the accurate IOMMU value if available.

Quoted 3 lines

If it is not available, then something went wrong and BUG_ON(!tbl || !tbl->it_page_shift) make more sense here than pretending that this function can ever return PAGE_SHIFT. imho.

That's a good point, thanks!

Quoted 11 lines
Quoted 7 lines

3) After 3/7, the platform can override the generic Power get_dma_page_shift().

4) After 4/7, pseries will return the DDW value, if available, then fallback to the IOMMU table's value. I think in the case of get_dma_ops(dev)==&dma_direct_ops, the only way that can happen is if we are using DDW, right?

This is for pseries guests; for the powernv host it is a "bypass" mode which does 64bit direct DMA mapping and there is no additional window for that (i.e. DIRECT64_PROPNAME, etc).

You're right! I should update the code to handle both cases.

In "bypass" mode, what TCE size is used? Is it guaranteed to be 4K?

Seems like this would be a different platform implentation I'd put in for 'powernv', is that right?

My apologies for missing that, and thank you for the review!

-Nish

#8 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

Quoted 2 lines · Nishanth Aravamudan

In "bypass" mode, what TCE size is used? Is it guaranteed to be 4K?

None :-) The TCEs are completely bypassed. You get a N:M linear mapping of all memory starting at 1<<59 PCI side.

Quoted 4 lines

Seems like this would be a different platform implentation I'd put in for 'powernv', is that right?

My apologies for missing that, and thank you for the review!

Cheers,
Ben.

author
#9 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

Quoted 6 lines · Benjamin Herrenschmidt
Quoted 2 lines · Nishanth Aravamudan

In "bypass" mode, what TCE size is used? Is it guaranteed to be 4K?

None :-) The TCEs are completely bypassed. You get a N:M linear mapping of all memory starting at 1<<59 PCI side.

Err, duh, sorry! Ok, so in that case, DMA page shift is PAGE_SHIFT, then?

-Nish

#10 lore raw

Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift

Quoted 6 lines · Nishanth Aravamudan
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 3 lines · Nishanth Aravamudan

In "bypass" mode, what TCE size is used? Is it guaranteed to be 4K?

Quoted 6 lines
Quoted 3 lines

None :-) The TCEs are completely bypassed. You get a N:M linear mapping
of all memory starting at 1<<59 PCI side.

Err, duh, sorry! Ok, so in that case, DMA page shift is PAGE_SHIFT, then?

I think so.

Cheers,
Ben.

#24 lore raw
In reply to David Miller
Quoted 9 lines · David Miller

From: "Busch, Keith" <keith.busch at intel.com>
Date: Tue, 27 Oct 2015 22:36:43 +0000

Quoted 2 lines

If you're suggesting to compile-time break architectures that currently work just fine with NVMe, let me stop you right there.

Silently "working" without the architecture maintainer having to explicity look at the new interface and make sure his platform is implementing it properly is an extremely bad practice.

It won't work if it's wrong. It'll BUG_ON, and I'll be assigned to help fix it, like what's happened here (on a private bugzilla).

The "new" interface for all the other architectures is the same as the old one we've been using for the last 5 years.

I welcome x86 maintainer feedback to confirm virtual and DMA addresses have the same offset at 4k alignment, but I have to insist we don't break my currently working hardware to force their attention.

#25 lore raw
Quoted 6 lines · Keith

The "new" interface for all the other architectures is the same as the old one we've been using for the last 5 years.

I welcome x86 maintainer feedback to confirm virtual and DMA addresses have the same offset at 4k alignment, but I have to insist we don't break my currently working hardware to force their attention.

We had a quick cht about this issue and I think we simply should default to a NVMe controler page size of 4k everywhere as that's the safe default. This is also what we do for RDMA Memory reigstrations and it works fine there for SRP and iSER.

author
#26 lore raw
Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Keith

The "new" interface for all the other architectures is the same as the old one we've been using for the last 5 years.

Quoted 8 lines
Quoted 3 lines

I welcome x86 maintainer feedback to confirm virtual and DMA addresses have the same offset at 4k alignment, but I have to insist we don't break my currently working hardware to force their attention.

We had a quick cht about this issue and I think we simply should default to a NVMe controler page size of 4k everywhere as that's the safe default. This is also what we do for RDMA Memory reigstrations and it works fine there for SRP and iSER.

So, would that imply changing just the NVMe driver code rather than adding the dma_page_shift API at all? What about architectures that can support the larger page sizes? There is an implied performance impact, at least, of shifting the IO size down.

Sorry for the continuing questions -- I got lots of conflicting feedback on the last series and want to make sure v4 is more acceptable.

Thanks,
Nish

#27 lore raw
Quoted 10 lines · Nishanth Aravamudan
Quoted 4 lines · Christoph Hellwig

We had a quick cht about this issue and I think we simply should default to a NVMe controler page size of 4k everywhere as that's the safe default. This is also what we do for RDMA Memory reigstrations and it works fine there for SRP and iSER.

So, would that imply changing just the NVMe driver code rather than adding the dma_page_shift API at all? What about architectures that can support the larger page sizes? There is an implied performance impact, at least, of shifting the IO size down.

It is the safe option, but you're right that it might have a measurable performance impact (can you run an experiment?). Maybe we should just change the driver to always use MPSMIN for the moment in the interest of time, and you can flush out the new API before the next merge window.

maintainer
#40 lore raw
In reply to Nishanth Aravamudan

From: Nishanth Aravamudan <[email protected]>
Date: Thu, 29 Oct 2015 08:57:01 -0700

Quoted 7 lines

So, would that imply changing just the NVMe driver code rather than adding the dma_page_shift API at all? What about architectures that can support the larger page sizes? There is an implied performance impact, at least, of shifting the IO size down.

Sorry for the continuing questions -- I got lots of conflicting feedback on the last series and want to make sure v4 is more acceptable.

In the long term I would be very happy to see us having a real interface for this stuff, just my opinion...

author
#28 lore raw

PATCH v3 1/1 drivers/nvme: default to 4k device page size

In reply to Busch, Keith
Quoted 12 lines · Keith
Quoted 10 lines · Nishanth Aravamudan
Quoted 4 lines · Christoph Hellwig

We had a quick cht about this issue and I think we simply should default to a NVMe controler page size of 4k everywhere as that's the safe default. This is also what we do for RDMA Memory reigstrations and it works fine there for SRP and iSER.

So, would that imply changing just the NVMe driver code rather than adding the dma_page_shift API at all? What about architectures that can support the larger page sizes? There is an implied performance impact, at least, of shifting the IO size down.

Quoted 5 lines

It is the safe option, but you're right that it might have a measurable performance impact (can you run an experiment?). Maybe we should just change the driver to always use MPSMIN for the moment in the interest of time, and you can flush out the new API before the next merge window.

Given that it's 4K just about everywhere by default (and sort of implicitly expected to be, I guess), I think I'd prefer we default to 4K. That should mitigate the performance impact (I'll ask our IO team to do some runs, but since this impacts functionality on some hardware, I don't think it's too relevant for now). Unless there are NVMe devcies with a MPSMAX < 4K?

Something like the following?

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. There is not currently an API to obtain the IOMMU's page size across all architectures and in the interest of a stop-gap fix to this functional issue, default the NVMe device page size to 4K, with the intent of adding such an API and implementation across all architectures in the next merge window.

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

---
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.

v2 -> v3:
In the interest of fixing the functional problem in the short-term, just force the device page size to 4K and work on adding the new API in the next merge window.

drivers/block/nvme-core.c +6 -1
index ccc0c1f93daa..a9a5285bdb39 100644
@@ -1717,7 +1717,12 @@ 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;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;

-Nish

author
#41 lore raw
In reply to David Miller
Quoted 8 lines · David Miller

From: Nishanth Aravamudan <nacc at linux.vnet.ibm.com>
Date: Thu, 29 Oct 2015 08:57:01 -0700

Quoted 5 lines

So, would that imply changing just the NVMe driver code rather than adding the dma_page_shift API at all? What about architectures that can support the larger page sizes? There is an implied performance impact, at least, of shifting the IO size down.

Quoted 5 lines
Quoted 2 lines

Sorry for the continuing questions -- I got lots of conflicting feedback on the last series and want to make sure v4 is more acceptable.

In the long term I would be very happy to see us having a real interface for this stuff, just my opinion...

Yep, I think I'll try and balance the two -- fix NVMe for now with a 4K page size as suggested by Christoph, and then work on the more complete API for the next merge.

Thanks,
Nish

#29 lore raw

Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

In reply to Nishanth Aravamudan
Quoted 6 lines · Nishanth Aravamudan

Given that it's 4K just about everywhere by default (and sort of implicitly expected to be, I guess), I think I'd prefer we default to 4K. That should mitigate the performance impact (I'll ask our IO team to do some runs, but since this impacts functionality on some hardware, I don't think it's too relevant for now). Unless there are NVMe devcies with a MPSMAX < 4K?

Right, I assumed MPSMIN was always 4k for the same reason you mentioned, but you can hard code it like you've done in your patch.

The spec defines MPSMAX such that it's impossible to find a device with MPSMAX < 4k.

author
#30 lore raw

Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

Quoted 8 lines · Keith Busch
Quoted 6 lines · Nishanth Aravamudan

Given that it's 4K just about everywhere by default (and sort of implicitly expected to be, I guess), I think I'd prefer we default to 4K. That should mitigate the performance impact (I'll ask our IO team to do some runs, but since this impacts functionality on some hardware, I don't think it's too relevant for now). Unless there are NVMe devcies with a MPSMAX < 4K?

Quoted 5 lines

Right, I assumed MPSMIN was always 4k for the same reason you mentioned, but you can hard code it like you've done in your patch.

The spec defines MPSMAX such that it's impossible to find a device with MPSMAX < 4k.

Great, thanks!

-Nish

#31 lore raw

Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

In reply to Nishanth Aravamudan
Quoted 7 lines · Nishanth Aravamudan
drivers/block/nvme-core.c +0 -0
index ccc0c1f93daa..a9a5285bdb39 100644
@@ -1717,7 +1717,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	u32 aqa;
 	u64 cap = readq(&dev->bar->cap);

drivers/block/nvme-core.c static int nvme_configure_admin_queue(struct nvme_dev *dev) · line 1727

Quoted 10 lines
drivers/block/nvme-core.c +6 -1
 	struct nvme_queue *nvmeq;
-	unsigned page_shift = PAGE_SHIFT;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;

Looks good as a start. Note that all the MPSMIN/MAX checking could be removed as NVMe devices must support 4k pages.

#32 lore raw

Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

Quoted 13 lines · Christoph Hellwig
Quoted 12 lines · Nishanth Aravamudan
drivers/block/nvme-core.c +3 -1
index ccc0c1f93daa..a9a5285bdb39 100644
@@ -1717,7 +1717,12 @@ 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;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
Quoted 8 lines

drivers/block/nvme-core.c static int nvme_configure_admin_queue(struct nvme_dev *dev) · line 1727

Quoted 5 lines
drivers/block/nvme-core.c +3 -0
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;

Looks good as a start. Note that all the MPSMIN/MAX checking could be removed as NVMe devices must support 4k pages.

MAX can go, and while it's probably the case that all devices support 4k, it's not a spec requirement, so we should keep the dev_page_min check.

author
#33 lore raw

PATCH v4 1/1 drivers/nvme: default to 4k device page size

Quoted 23 lines · Keith Busch
Quoted 13 lines · Christoph Hellwig
Quoted 12 lines · Nishanth Aravamudan
drivers/block/nvme-core.c +3 -1
index ccc0c1f93daa..a9a5285bdb39 100644
@@ -1717,7 +1717,12 @@ 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;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
Quoted 8 lines

drivers/block/nvme-core.c static int nvme_configure_admin_queue(struct nvme_dev *dev) · line 1727

Quoted 5 lines
drivers/block/nvme-core.c +3 -0
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;

Looks good as a start. Note that all the MPSMIN/MAX checking could be removed as NVMe devices must support 4k pages.

Quoted 2 lines

MAX can go, and while it's probably the case that all devices support 4k, it's not a spec requirement, so we should keep the dev_page_min check.

Ok, here's an updated patch.

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. There is not currently an API to obtain the IOMMU's page size across all architectures and in the interest of a stop-gap fix to this functional issue, default the NVMe device page size to 4K, with the intent of adding such an API and implementation across all architectures in the next merge window.

With the functionally equivalent v3 of this patch, our hardware test exerciser survives when using 32-bit DMA; without the patch, the kernel will BUG within a few minutes.

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

---
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.

v2 -> v3:
In the interest of fixing the functional problem in the short-term, just force the device page size to 4K and work on adding the new API in the next merge window.

v3 -> v4:
Rebase to the 4.3, including the new code locations.
Based upon feedback from Keith Busch and Christoph Hellwig, remove the device max check, as the spec requires MPSMAX >= 4K.

drivers/nvme/host/pci.c +6 -9
index e878590e71b6..00ca45bb0bc0 100644
@@ -1701,9 +1701,13 @@ 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;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
-	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
 
 	if (page_shift < dev_page_min) {
 		dev_err(dev->dev,

@@ -1712,13 +1716,6 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 				1 << page_shift);
 		return -ENODEV;
 	}
-	if (page_shift > dev_page_max) {
-		dev_info(dev->dev,
-				"Device maximum page size (%u) smaller than "
-				"host (%u); enabling work-around\n",
-				1 << dev_page_max, 1 << page_shift);
-		page_shift = dev_page_max;
-	}
 
 	dev->subsystem = readl(&dev->bar->vs) >= NVME_VS(1, 1) ?
 						NVME_CAP_NSSRC(cap) : 0;
Reviewed-by on v4 1/1
#34 lore raw

Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>

... but I doubt we'll ever bother updating it. Most architectures with arger page sizes also have iommus and would need different settings for different iommus vs direct mapping for very little gain. There's a reason why we never bothered for RDMA either.

author
#35 lore raw

Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

Quoted 8 lines · Christoph Hellwig

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>

... but I doubt we'll ever bother updating it. Most architectures with arger page sizes also have iommus and would need different settings for different iommus vs direct mapping for very little gain. There's a reason why we never bothered for RDMA either.

Fair enough :) Thanks for all your reviews and comments.

-Nish

author
#36 lore raw

Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

In reply to Christoph Hellwig
Quoted 8 lines · Christoph Hellwig

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>

... but I doubt we'll ever bother updating it. Most architectures with arger page sizes also have iommus and would need different settings for different iommus vs direct mapping for very little gain. There's a reason why we never bothered for RDMA either.

FWIW, whose tree should this go through? The bug only appears on Power, afaik, but the patch is now just an NVMe change.

Thanks,
Nish

Acked-by
#38 lore raw

Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

Quoted 1 line · Christoph Hellwig

Jens, Keith: any chance to get this to Linux for 4.4 (and -stable)?

I agreed, looks good to me.

Acked-by Keith Busch <keith.busch at intel.com>
#39 lore raw

Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

Quoted 6 lines · Keith Busch
Quoted 1 line · Christoph Hellwig

Jens, Keith: any chance to get this to Linux for 4.4 (and -stable)?

I agreed, looks good to me.

Acked-by Keith Busch <keith.busch at intel.com>

Jens, can you pick this one for -rc2?