Fix NVMe driver support on Power with 32-bit DMA
Subsystems (4)
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.
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.
---
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
@@ -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))
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.
drivers/block/nvme-core.c +6 -0
@@ -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,
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.
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
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.
kbuild test robot
build botRe: [PATCH 2/2] drivers/nvme: default to the IOMMU page size on Power
In reply to Nishanth Aravamudanconfig: 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
Re: [PATCH 1/2] powerpc/iommu: expose IOMMU page shift
In reply to Nishanth AravamudanQuoted 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 existingIOMMU_PAGE_SIZE()value.arch/powerpc/include/asm/iommu.h +0
-0
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>
Re: [PATCH 1/2] powerpc/iommu: expose IOMMU page shift
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 existingIOMMU_PAGE_SIZE()value.arch/powerpc/include/asm/iommu.h +0
-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)
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_SIZEso it uses the newIOMMU_PAGE_SHIFTmacro.
Yes absolutely! Sorry, I initially didn't add the first macro, so didn't think that through. Will update.
-Nish
Re: [PATCH 1/2] powerpc/iommu: expose IOMMU page shift
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 existingIOMMU_PAGE_SIZE()value.arch/powerpc/include/asm/iommu.h +0
-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)
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_SIZEso it uses the newIOMMU_PAGE_SHIFTmacro.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
v2 cover Fix NVMe driver support on Power with 32-bit DMA
In reply to Nishanth AravamudanWe 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.
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.
include/asm-generic/dma-mapping-common.h +7 -0
@@ -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
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.
arch/powerpc/include/asm/dma-mapping.h +3 -0
@@ -125,6 +125,9 @@ static inline void set_dma_offset(struct device *dev, dma_addr_t off)#define HAVE_ARCH_DMA_SET_MASK 1extern 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
@@ -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);
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.
arch/powerpc/include/asm/machdep.h +2 -1
@@ -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
@@ -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 ;
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.
arch/powerpc/platforms/pseries/iommu.c +36 -0
@@ -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;
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
@@ -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 ;
kbuild test robot
build botRe: [PATCH 5/5 v2] drivers/nvme: default to the IOMMU page size
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>
kbuild test robot
build botRe: [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift
In reply to Nishanth Aravamudanconfig: 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>
Michael Ellerman
reviewerRe: [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift
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: 'structmachdep_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
Fengguang Wu
build botRe: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift
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
Michael Ellerman
reviewerRe: [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=powerpcAll 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: 'structmachdep_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
Fengguang Wu
build botRe: [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
Fengguang Wu
build botRe: [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
Michael Ellerman
reviewerRe: [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=powerpcQuoted 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: 'structmachdep_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-rdmaQuoted 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
Fengguang Wu
build botRe: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift
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
Michael Ellerman
reviewerRe: [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
Re: [kbuild-all] [PATCH 4/5 v2] pseries/iommu: implement DDW-aware dma_get_page_shift
In reply to Fengguang WuHi 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---
Fengguang Wu
build bottesting 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
Michael Ellerman
reviewerRe: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API
In reply to Nishanth AravamudanQuoted 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_SHIFTon all architectures.Signed-off-by Nishanth Aravamudan <nacc at linux.vnet.ibm.com>include/asm-generic/dma-mapping-common.h +0
-0@@ -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
Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API
In reply to Nishanth AravamudanDo 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.
Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API
Quoted 2 lines · Christoph Hellwig
Do we need a function here or can we just have a
IOMMU_PAGE_SHIFTdefine 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.hyet, so you either need to update all of those as well, or just add the #ifndef directly tolinux/dma-mapping.h.
Ok, I will take a look at that and spin up a v3.
Thanks for the feedback!
-Nish
Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API
In reply to Christoph HellwigQuoted 2 lines · Christoph Hellwig
Do we need a function here or can we just have a
IOMMU_PAGE_SHIFTdefine 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
Re: [PATCH 1/5 v2] dma-mapping: add generic dma_get_page_shift API
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_SHIFTdefine 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_SHIFTalready, 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
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_SHIFTalready, 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.
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_SHIFTalready, 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
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_SHIFTalready, 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.hinstead ofdma-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 definesIOMMU_PAGE_SHIFTandIO_PAGE_SHIFTinarch/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 aasm/dma-mapping.h? Should we move some of the constants fromarch/sparc/kernel/iommu_common.htoarch/sparc/include/asm/iommu_common.hand then #include that inasm/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_SHIFTis fine there too?
Anyone with sparc knowledge have an opinion/input?
-Nish
Quoted 2 lines · Nishanth Aravamudan
1) add a generic
dma_get_page_shiftimplementation that just returnsPAGE_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.
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 2 lines · Nishanth Aravamudan
1) add a generic
dma_get_page_shiftimplementation that just returnsPAGE_SHIFTSo 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
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.
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
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 2 lines · Nishanth Aravamudan
1) add a generic
dma_get_page_shiftimplementation that just returnsPAGE_SHIFTSo 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.