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.
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 <redacted>
---
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(+)
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 <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 3 +++
arch/powerpc/kernel/dma.c | 9 +++++++++
2 files changed, 12 insertions(+)
[Sorry, subject should have been 0/7!]
On 23.10.2015 [13:54:20 -0700], Nishanth Aravamudan wrote:
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.
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 <redacted>
---
arch/powerpc/include/asm/machdep.h | 3 ++-
arch/powerpc/kernel/dma.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
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 <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 36 ++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
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 <redacted>
---
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
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 <redacted>
---
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(+)
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 <redacted>
---
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(-)
[Apologies for the subject line, should just have the [RFC PATCH 5/7]]
On 23.10.2015 [14:00:08 -0700], Nishanth Aravamudan wrote:
quoted hunk
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 <redacted>
---
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
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.
On 10/24/2015 07:59 AM, Nishanth Aravamudan wrote:
quoted hunk
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 <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 36 ++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
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.
On 10/24/2015 07:57 AM, Nishanth Aravamudan wrote:
quoted hunk
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 <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 3 +++
arch/powerpc/kernel/dma.c | 9 +++++++++
2 files changed, 12 insertions(+)
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)}EXPORT_SYMBOL(dma_set_mask);+unsignedlongdma_get_page_shift(structdevice*dev)+{+structiommu_table*tbl=get_iommu_table_base(dev);+if(tbl)+returntbl->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?
From: Busch, Keith <hidden> Date: 2015-10-27 14:09:09
On Tue, Oct 27, 2015 at 05:02:16PM +1100, Alexey Kardashevskiy wrote:
quoted
+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.
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
On 27.10.2015 [16:56:10 +1100], Alexey Kardashevskiy wrote:
On 10/24/2015 07:59 AM, Nishanth Aravamudan wrote:
quoted
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 <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 36 ++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
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
On 27.10.2015 [17:02:16 +1100], Alexey Kardashevskiy wrote:
On 10/24/2015 07:57 AM, Nishanth Aravamudan wrote:
quoted
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 <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 3 +++
arch/powerpc/kernel/dma.c | 9 +++++++++
2 files changed, 12 insertions(+)
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)}EXPORT_SYMBOL(dma_set_mask);+unsignedlongdma_get_page_shift(structdevice*dev)+{+structiommu_table*tbl=get_iommu_table_base(dev);+if(tbl)+returntbl->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
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!
If you're suggesting to compile-time break architectures that currently
work just fine with NVMe, let me stop you right there.
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!
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,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
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!
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
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!
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.
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,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2015-10-28 00:31:41
On Wed, 2015-10-28 at 10:43 +1100, Julian Calaby wrote:
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.
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.
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.
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.
On 10/28/2015 09:27 AM, Nishanth Aravamudan wrote:
On 27.10.2015 [17:02:16 +1100], Alexey Kardashevskiy wrote:
quoted
On 10/24/2015 07:57 AM, Nishanth Aravamudan wrote:
quoted
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 <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 3 +++
arch/powerpc/kernel/dma.c | 9 +++++++++
2 files changed, 12 insertions(+)
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)}EXPORT_SYMBOL(dma_set_mask);+unsignedlongdma_get_page_shift(structdevice*dev)+{+structiommu_table*tbl=get_iommu_table_base(dev);+if(tbl)+returntbl->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.
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.
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).
--
Alexey
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
On 28.10.2015 [12:00:20 +1100], Alexey Kardashevskiy wrote:
On 10/28/2015 09:27 AM, Nishanth Aravamudan wrote:
quoted
On 27.10.2015 [17:02:16 +1100], Alexey Kardashevskiy wrote:
quoted
On 10/24/2015 07:57 AM, Nishanth Aravamudan wrote:
quoted
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 <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 3 +++
arch/powerpc/kernel/dma.c | 9 +++++++++
2 files changed, 12 insertions(+)
@@ -335,6 +335,15 @@ int dma_set_mask(struct device *dev, u64 dma_mask)}EXPORT_SYMBOL(dma_set_mask);+unsignedlongdma_get_page_shift(structdevice*dev)+{+structiommu_table*tbl=get_iommu_table_base(dev);+if(tbl)+returntbl->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.
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
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2015-10-28 02:21:37
On Tue, 2015-10-27 at 18:54 -0700, Nishanth Aravamudan wrote:
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.
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!
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.
From: Christoph Hellwig <hch@infradead.org> Date: 2015-10-29 11:55:47
On Wed, Oct 28, 2015 at 01:59:23PM +0000, Busch, Keith wrote:
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.
On 29.10.2015 [04:55:36 -0700], Christoph Hellwig wrote:
On Wed, Oct 28, 2015 at 01:59:23PM +0000, Busch, Keith wrote:
quoted
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.
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
From: Busch, Keith <hidden> Date: 2015-10-29 17:21:11
On Thu, Oct 29, 2015 at 08:57:01AM -0700, Nishanth Aravamudan wrote:
On 29.10.2015 [04:55:36 -0700], Christoph Hellwig wrote:
quoted
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.
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...
On 29.10.2015 [17:20:43 +0000], Busch, Keith wrote:
On Thu, Oct 29, 2015 at 08:57:01AM -0700, Nishanth Aravamudan wrote:
quoted
On 29.10.2015 [04:55:36 -0700], Christoph Hellwig wrote:
quoted
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.
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 <redacted>
---
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.
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...
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
From: Keith Busch <hidden> Date: 2015-10-30 21:48:51
On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
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.
On 30.10.2015 [21:48:48 +0000], Keith Busch wrote:
On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
quoted
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.
@@ -1717,7 +1717,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)u32aqa;u64cap=readq(&dev->bar->cap);structnvme_queue*nvmeq;-unsignedpage_shift=PAGE_SHIFT;+/*+*defaulttoa4Kpagesize,withtheintentiontoupdatethis+*pathinthefuturetoaccomodatearchitectureswithdiffering+*kernelandIOpagesizes.+*/+unsignedpage_shift=12;unsigneddev_page_min=NVME_CAP_MPSMIN(cap)+12;unsigneddev_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.
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 <redacted>
---
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.
From: Christoph Hellwig <hch@infradead.org> Date: 2015-11-05 19:58:45
Looks fine,
Reviewed-by: Christoph Hellwig <hch@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.
On 05.11.2015 [11:58:39 -0800], Christoph Hellwig wrote:
Looks fine,
Reviewed-by: Christoph Hellwig <hch@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
On 05.11.2015 [11:58:39 -0800], Christoph Hellwig wrote:
Looks fine,
Reviewed-by: Christoph Hellwig <hch@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