* [PATCH kernel v4 1/4] powerpc/powernv/ioda: Fix race in TCE level allocation
2019-07-12 9:45 [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Alexey Kardashevskiy
@ 2019-07-12 9:45 ` Alexey Kardashevskiy
2019-07-12 9:45 ` [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA Alexey Kardashevskiy
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, stable, Alexey Kardashevskiy,
Oliver O'Halloran, David Gibson
pnv_tce() returns a pointer to a TCE entry and originally a TCE table
would be pre-allocated. For the default case of 2GB window the table
needs only a single level and that is fine. However if more levels are
requested, it is possible to get a race when 2 threads want a pointer
to a TCE entry from the same page of TCEs.
This adds cmpxchg to handle the race. Note that once TCE is non-zero,
it cannot become zero again.
CC: stable@vger.kernel.org # v4.19+
Fixes: a68bd1267b72 ("powerpc/powernv/ioda: Allocate indirect TCE levels on demand")
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
The race occurs about 30 times in the first 3 minutes of copying files
via rsync and that's about it.
This fixes EEH's from
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=110810
---
Changes:
v2:
* replaced spin_lock with cmpxchg+readonce
---
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
index e28f03e1eb5e..8d6569590161 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
@@ -48,6 +48,9 @@ static __be64 *pnv_alloc_tce_level(int nid, unsigned int shift)
return addr;
}
+static void pnv_pci_ioda2_table_do_free_pages(__be64 *addr,
+ unsigned long size, unsigned int levels);
+
static __be64 *pnv_tce(struct iommu_table *tbl, bool user, long idx, bool alloc)
{
__be64 *tmp = user ? tbl->it_userspace : (__be64 *) tbl->it_base;
@@ -57,9 +60,9 @@ static __be64 *pnv_tce(struct iommu_table *tbl, bool user, long idx, bool alloc)
while (level) {
int n = (idx & mask) >> (level * shift);
- unsigned long tce;
+ unsigned long oldtce, tce = be64_to_cpu(READ_ONCE(tmp[n]));
- if (tmp[n] == 0) {
+ if (!tce) {
__be64 *tmp2;
if (!alloc)
@@ -70,10 +73,15 @@ static __be64 *pnv_tce(struct iommu_table *tbl, bool user, long idx, bool alloc)
if (!tmp2)
return NULL;
- tmp[n] = cpu_to_be64(__pa(tmp2) |
- TCE_PCI_READ | TCE_PCI_WRITE);
+ tce = __pa(tmp2) | TCE_PCI_READ | TCE_PCI_WRITE;
+ oldtce = be64_to_cpu(cmpxchg(&tmp[n], 0,
+ cpu_to_be64(tce)));
+ if (oldtce) {
+ pnv_pci_ioda2_table_do_free_pages(tmp2,
+ ilog2(tbl->it_level_size) + 3, 1);
+ tce = oldtce;
+ }
}
- tce = be64_to_cpu(tmp[n]);
tmp = __va(tce & ~(TCE_PCI_READ | TCE_PCI_WRITE));
idx &= ~mask;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA
2019-07-12 9:45 [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Alexey Kardashevskiy
2019-07-12 9:45 ` [PATCH kernel v4 1/4] powerpc/powernv/ioda: Fix race in TCE level allocation Alexey Kardashevskiy
@ 2019-07-12 9:45 ` Alexey Kardashevskiy
2019-07-12 15:20 ` Christoph Hellwig
2019-07-12 9:45 ` [PATCH kernel v4 3/4] powerpc/powernv/ioda2: Allocate TCE table levels on demand for default DMA window Alexey Kardashevskiy
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
POWER8 and newer support a bypass mode which maps all host memory to
PCI buses so an IOMMU table is not always required. However if we fail to
create such a table, the DMA setup fails and the kernel does not boot.
This skips the 32bit DMA setup check if the bypass is can be selected.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
This minor thing helped me debugging next 2 patches so it can help
somebody else too.
---
arch/powerpc/kernel/dma-iommu.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/dma-iommu.c b/arch/powerpc/kernel/dma-iommu.c
index a0879674a9c8..c963d704fa31 100644
--- a/arch/powerpc/kernel/dma-iommu.c
+++ b/arch/powerpc/kernel/dma-iommu.c
@@ -122,18 +122,17 @@ int dma_iommu_dma_supported(struct device *dev, u64 mask)
{
struct iommu_table *tbl = get_iommu_table_base(dev);
- if (!tbl) {
- dev_info(dev, "Warning: IOMMU dma not supported: mask 0x%08llx"
- ", table unavailable\n", mask);
- return 0;
- }
-
if (dev_is_pci(dev) && dma_iommu_bypass_supported(dev, mask)) {
dev->archdata.iommu_bypass = true;
dev_dbg(dev, "iommu: 64-bit OK, using fixed ops\n");
return 1;
}
+ if (!tbl) {
+ dev_err(dev, "Warning: IOMMU dma not supported: mask 0x%08llx, table unavailable\n", mask);
+ return 0;
+ }
+
if (tbl->it_offset > (mask >> tbl->it_page_shift)) {
dev_info(dev, "Warning: IOMMU offset too big for device mask\n");
dev_info(dev, "mask: 0x%08llx, table offset: 0x%08lx\n",
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA
2019-07-12 9:45 ` [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA Alexey Kardashevskiy
@ 2019-07-12 15:20 ` Christoph Hellwig
2019-07-15 8:33 ` Alexey Kardashevskiy
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2019-07-12 15:20 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
linuxppc-dev, David Gibson
> This skips the 32bit DMA setup check if the bypass is can be selected.
That sentence does not parse. I think you need to dop the "can be"
based on the actual patch.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA
2019-07-12 15:20 ` Christoph Hellwig
@ 2019-07-15 8:33 ` Alexey Kardashevskiy
2019-07-15 12:35 ` Christoph Hellwig
0 siblings, 1 reply; 12+ messages in thread
From: Alexey Kardashevskiy @ 2019-07-15 8:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
linuxppc-dev, David Gibson
On 13/07/2019 01:20, Christoph Hellwig wrote:
>> This skips the 32bit DMA setup check if the bypass is can be selected.
>
> That sentence does not parse. I think you need to dop the "can be"
> based on the actual patch.
"the 32bit DMA setup check" is
"if (!(tbl = get_iommu_table_base(dev)))".
I can rephrase though.
--
Alexey
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA
2019-07-15 8:33 ` Alexey Kardashevskiy
@ 2019-07-15 12:35 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2019-07-15 12:35 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Sam Bobroff, Alistair Popple, Christoph Hellwig,
Oliver O'Halloran, linuxppc-dev, David Gibson
On Mon, Jul 15, 2019 at 06:33:00PM +1000, Alexey Kardashevskiy wrote:
>
>
> On 13/07/2019 01:20, Christoph Hellwig wrote:
> > > This skips the 32bit DMA setup check if the bypass is can be selected.
> >
> > That sentence does not parse. I think you need to dop the "can be"
> > based on the actual patch.
>
>
> "the 32bit DMA setup check" is
> "if (!(tbl = get_iommu_table_base(dev)))".
>
> I can rephrase though.
What I mean is to replace
"This skips the 32bit DMA setup check if the bypass is can be selected."
with
"This skips the 32bit DMA setup check if the bypass is selected."
or alternatively:
"This skips the 32bit DMA setup check if the bypass can be selected."
but I think the first version is more accurate.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH kernel v4 3/4] powerpc/powernv/ioda2: Allocate TCE table levels on demand for default DMA window
2019-07-12 9:45 [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Alexey Kardashevskiy
2019-07-12 9:45 ` [PATCH kernel v4 1/4] powerpc/powernv/ioda: Fix race in TCE level allocation Alexey Kardashevskiy
2019-07-12 9:45 ` [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA Alexey Kardashevskiy
@ 2019-07-12 9:45 ` Alexey Kardashevskiy
2019-07-12 9:45 ` [PATCH kernel v4 4/4] powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU pages Alexey Kardashevskiy
2019-07-12 15:28 ` [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Christoph Hellwig
4 siblings, 0 replies; 12+ messages in thread
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
We allocate only the first level of multilevel TCE tables for KVM
already (alloc_userspace_copy==true), and the rest is allocated on demand.
This is not enabled though for bare metal.
This removes the KVM limitation (implicit, via the alloc_userspace_copy
parameter) and always allocates just the first level. The on-demand
allocation of missing levels is already implemented.
As from now on DMA map might happen with disabled interrupts, this
allocates TCEs with GFP_ATOMIC; otherwise lockdep reports errors 1].
In practice just a single page is allocated there so chances for failure
are quite low.
To save time when creating a new clean table, this skips non-allocated
indirect TCE entries in pnv_tce_free just like we already do in
the VFIO IOMMU TCE driver.
This changes the default level number from 1 to 2 to reduce the amount
of memory required for the default 32bit DMA window at the boot time.
The default window size is up to 2GB which requires 4MB of TCEs which is
unlikely to be used entirely or at all as most devices these days are
64bit capable so by switching to 2 levels by default we save 4032KB of
RAM per a device.
While at this, add __GFP_NOWARN to alloc_pages_node() as the userspace
can trigger this path via VFIO, see the failure and try creating a table
again with different parameters which might succeed.
[1]:
===
BUG: sleeping function called from invalid context at mm/page_alloc.c:4596
in_atomic(): 1, irqs_disabled(): 1, pid: 1038, name: scsi_eh_1
2 locks held by scsi_eh_1/1038:
#0: 000000005efd659a (&host->eh_mutex){+.+.}, at: ata_eh_acquire+0x34/0x80
#1: 0000000006cf56a6 (&(&host->lock)->rlock){....}, at: ata_exec_internal_sg+0xb0/0x5c0
irq event stamp: 500
hardirqs last enabled at (499): [<c000000000cb8a74>] _raw_spin_unlock_irqrestore+0x94/0xd0
hardirqs last disabled at (500): [<c000000000cb85c4>] _raw_spin_lock_irqsave+0x44/0x120
softirqs last enabled at (0): [<c000000000101120>] copy_process.isra.4.part.5+0x640/0x1a80
softirqs last disabled at (0): [<0000000000000000>] 0x0
CPU: 73 PID: 1038 Comm: scsi_eh_1 Not tainted 5.2.0-rc6-le_nv2_aikATfstn1-p1 #634
Call Trace:
[c000003d064cef50] [c000000000c8e6c4] dump_stack+0xe8/0x164 (unreliable)
[c000003d064cefa0] [c00000000014ed78] ___might_sleep+0x2f8/0x310
[c000003d064cf020] [c0000000003ca084] __alloc_pages_nodemask+0x2a4/0x1560
[c000003d064cf220] [c0000000000c2530] pnv_alloc_tce_level.isra.0+0x90/0x130
[c000003d064cf290] [c0000000000c2888] pnv_tce+0x128/0x3b0
[c000003d064cf360] [c0000000000c2c00] pnv_tce_build+0xb0/0xf0
[c000003d064cf3c0] [c0000000000bbd9c] pnv_ioda2_tce_build+0x3c/0xb0
[c000003d064cf400] [c00000000004cfe0] ppc_iommu_map_sg+0x210/0x550
[c000003d064cf510] [c00000000004b7a4] dma_iommu_map_sg+0x74/0xb0
[c000003d064cf530] [c000000000863944] ata_qc_issue+0x134/0x470
[c000003d064cf5b0] [c000000000863ec4] ata_exec_internal_sg+0x244/0x5c0
[c000003d064cf700] [c0000000008642d0] ata_exec_internal+0x90/0xe0
[c000003d064cf780] [c0000000008650ac] ata_dev_read_id+0x2ec/0x640
[c000003d064cf8d0] [c000000000878e28] ata_eh_recover+0x948/0x16d0
[c000003d064cfa10] [c00000000087d760] sata_pmp_error_handler+0x480/0xbf0
[c000003d064cfbc0] [c000000000884624] ahci_error_handler+0x74/0xe0
[c000003d064cfbf0] [c000000000879fa8] ata_scsi_port_error_handler+0x2d8/0x7c0
[c000003d064cfca0] [c00000000087a544] ata_scsi_error+0xb4/0x100
[c000003d064cfd00] [c000000000802450] scsi_error_handler+0x120/0x510
[c000003d064cfdb0] [c000000000140c48] kthread+0x1b8/0x1c0
[c000003d064cfe20] [c00000000000bd8c] ret_from_kernel_thread+0x5c/0x70
ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
irq event stamp: 2305
========================================================
hardirqs last enabled at (2305): [<c00000000000e4c8>] fast_exc_return_irq+0x28/0x34
hardirqs last disabled at (2303): [<c000000000cb9fd0>] __do_softirq+0x4a0/0x654
WARNING: possible irq lock inversion dependency detected
5.2.0-rc6-le_nv2_aikATfstn1-p1 #634 Tainted: G W
softirqs last enabled at (2304): [<c000000000cba054>] __do_softirq+0x524/0x654
softirqs last disabled at (2297): [<c00000000010f278>] irq_exit+0x128/0x180
--------------------------------------------------------
swapper/0/0 just changed the state of lock:
0000000006cf56a6 (&(&host->lock)->rlock){-...}, at: ahci_single_level_irq_intr+0xac/0x120
but this lock took another, HARDIRQ-unsafe lock in the past:
(fs_reclaim){+.+.}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
local_irq_disable();
lock(&(&host->lock)->rlock);
lock(fs_reclaim);
<Interrupt>
lock(&(&host->lock)->rlock);
*** DEADLOCK ***
no locks held by swapper/0/0.
the shortest dependencies between 2nd lock and 1st lock:
-> (fs_reclaim){+.+.} ops: 167579 {
HARDIRQ-ON-W at:
lock_acquire+0xf8/0x2a0
fs_reclaim_acquire.part.23+0x44/0x60
kmem_cache_alloc_node_trace+0x80/0x590
alloc_desc+0x64/0x270
__irq_alloc_descs+0x2e4/0x3a0
irq_domain_alloc_descs+0xb0/0x150
irq_create_mapping+0x168/0x2c0
xics_smp_probe+0x2c/0x98
pnv_smp_probe+0x40/0x9c
smp_prepare_cpus+0x524/0x6c4
kernel_init_freeable+0x1b4/0x650
kernel_init+0x2c/0x148
ret_from_kernel_thread+0x5c/0x70
SOFTIRQ-ON-W at:
lock_acquire+0xf8/0x2a0
fs_reclaim_acquire.part.23+0x44/0x60
kmem_cache_alloc_node_trace+0x80/0x590
alloc_desc+0x64/0x270
__irq_alloc_descs+0x2e4/0x3a0
irq_domain_alloc_descs+0xb0/0x150
irq_create_mapping+0x168/0x2c0
xics_smp_probe+0x2c/0x98
pnv_smp_probe+0x40/0x9c
smp_prepare_cpus+0x524/0x6c4
kernel_init_freeable+0x1b4/0x650
kernel_init+0x2c/0x148
ret_from_kernel_thread+0x5c/0x70
INITIAL USE at:
lock_acquire+0xf8/0x2a0
fs_reclaim_acquire.part.23+0x44/0x60
kmem_cache_alloc_node_trace+0x80/0x590
alloc_desc+0x64/0x270
__irq_alloc_descs+0x2e4/0x3a0
irq_domain_alloc_descs+0xb0/0x150
irq_create_mapping+0x168/0x2c0
xics_smp_probe+0x2c/0x98
pnv_smp_probe+0x40/0x9c
smp_prepare_cpus+0x524/0x6c4
kernel_init_freeable+0x1b4/0x650
kernel_init+0x2c/0x148
ret_from_kernel_thread+0x5c/0x70
}
===
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Alistair Popple <alistair@popple.id.au>
---
Changes:
v4:
* added lockdep output to the commit log to justify ATOMIC in alloc_pages_node
v2:
* added __GFP_NOWARN to alloc_pages_node
---
arch/powerpc/platforms/powernv/pci.h | 2 +-
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 20 +++++++++----------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 469c24463247..f914f0b14e4e 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -219,7 +219,7 @@ extern struct iommu_table_group *pnv_npu_compound_attach(
struct pnv_ioda_pe *pe);
/* pci-ioda-tce.c */
-#define POWERNV_IOMMU_DEFAULT_LEVELS 1
+#define POWERNV_IOMMU_DEFAULT_LEVELS 2
#define POWERNV_IOMMU_MAX_LEVELS 5
extern int pnv_tce_build(struct iommu_table *tbl, long index, long npages,
diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
index 8d6569590161..a0b9c0c23ed2 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
@@ -36,7 +36,8 @@ static __be64 *pnv_alloc_tce_level(int nid, unsigned int shift)
struct page *tce_mem = NULL;
__be64 *addr;
- tce_mem = alloc_pages_node(nid, GFP_KERNEL, shift - PAGE_SHIFT);
+ tce_mem = alloc_pages_node(nid, GFP_ATOMIC | __GFP_NOWARN,
+ shift - PAGE_SHIFT);
if (!tce_mem) {
pr_err("Failed to allocate a TCE memory, level shift=%d\n",
shift);
@@ -169,6 +170,9 @@ void pnv_tce_free(struct iommu_table *tbl, long index, long npages)
if (ptce)
*ptce = cpu_to_be64(0);
+ else
+ /* Skip the rest of the level */
+ i |= tbl->it_level_size - 1;
}
}
@@ -268,7 +272,6 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
unsigned int table_shift = max_t(unsigned int, entries_shift + 3,
PAGE_SHIFT);
const unsigned long tce_table_size = 1UL << table_shift;
- unsigned int tmplevels = levels;
if (!levels || (levels > POWERNV_IOMMU_MAX_LEVELS))
return -EINVAL;
@@ -276,9 +279,6 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
if (!is_power_of_2(window_size))
return -EINVAL;
- if (alloc_userspace_copy && (window_size > (1ULL << 32)))
- tmplevels = 1;
-
/* Adjust direct table size from window_size and levels */
entries_shift = (entries_shift + levels - 1) / levels;
level_shift = entries_shift + 3;
@@ -289,7 +289,7 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
/* Allocate TCE table */
addr = pnv_pci_ioda2_table_do_alloc_pages(nid, level_shift,
- tmplevels, tce_table_size, &offset, &total_allocated);
+ 1, tce_table_size, &offset, &total_allocated);
/* addr==NULL means that the first level allocation failed */
if (!addr)
@@ -300,18 +300,18 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
* we did not allocate as much as we wanted,
* release partially allocated table.
*/
- if (tmplevels == levels && offset < tce_table_size)
+ if (levels == 1 && offset < tce_table_size)
goto free_tces_exit;
/* Allocate userspace view of the TCE table */
if (alloc_userspace_copy) {
offset = 0;
uas = pnv_pci_ioda2_table_do_alloc_pages(nid, level_shift,
- tmplevels, tce_table_size, &offset,
+ 1, tce_table_size, &offset,
&total_allocated_uas);
if (!uas)
goto free_tces_exit;
- if (tmplevels == levels && (offset < tce_table_size ||
+ if (levels == 1 && (offset < tce_table_size ||
total_allocated_uas != total_allocated))
goto free_uas_exit;
}
@@ -326,7 +326,7 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
pr_debug("Created TCE table: ws=%08llx ts=%lx @%08llx base=%lx uas=%p levels=%d/%d\n",
window_size, tce_table_size, bus_offset, tbl->it_base,
- tbl->it_userspace, tmplevels, levels);
+ tbl->it_userspace, 1, levels);
return 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH kernel v4 4/4] powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU pages
2019-07-12 9:45 [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Alexey Kardashevskiy
` (2 preceding siblings ...)
2019-07-12 9:45 ` [PATCH kernel v4 3/4] powerpc/powernv/ioda2: Allocate TCE table levels on demand for default DMA window Alexey Kardashevskiy
@ 2019-07-12 9:45 ` Alexey Kardashevskiy
2019-07-12 15:25 ` Christoph Hellwig
2019-07-12 15:28 ` [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Christoph Hellwig
4 siblings, 1 reply; 12+ messages in thread
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
At the moment we create a small window only for 32bit devices, the window
maps 0..2GB of the PCI space only. For other devices we either use
a sketchy bypass or hardware bypass but the former can only work if
the amount of RAM is no bigger than the device's DMA mask and the latter
requires devices to support at least 59bit DMA.
This extends the default DMA window to the maximum size possible to allow
a wider DMA mask than just 32bit. The default window size is now limited
by the the iommu_table::it_map allocation bitmap which is a contiguous
array, 1 bit per an IOMMU page.
This increases the default IOMMU page size from hard coded 4K to
the system page size to allow wider DMA masks.
This increases the level number to not exceed the max order allocation
limit per TCE level. By the same time, this keeps minimal levels number
as 2 in order to save memory.
As the extended window now overlaps the 32bit MMIO region, this adds
an area reservation to iommu_init_table().
After this change the default window size is 0x80000000000==1<<43 so
devices limited to DMA mask smaller than the amount of system RAM can
still use more than just 2GB of memory for DMA.
With the on-demand allocation of indirect TCE table levels enabled and
2 levels, the first TCE level size is just
1<<ceil((log2(0x7ffffffffff+1)-16)/2)=16384 TCEs or 2 system pages.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* fixed take/release ownership handlers
* fixed reserved region for tables with it_offset!=0 (this is not going
to be exploited here but still this is a correct behavior)
v3:
* fixed tce levels calculation
v2:
* adjusted level number to the max order
---
arch/powerpc/include/asm/iommu.h | 8 ++-
arch/powerpc/kernel/iommu.c | 74 ++++++++++++++++-------
arch/powerpc/platforms/powernv/pci-ioda.c | 40 ++++++++++--
3 files changed, 92 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 18d342b815e4..a527a5fe1b01 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -111,6 +111,8 @@ struct iommu_table {
struct iommu_table_ops *it_ops;
struct kref it_kref;
int it_nid;
+ unsigned long it_reserved_start; /* Start of not-DMA-able (MMIO) area */
+ unsigned long it_reserved_end;
};
#define IOMMU_TABLE_USERSPACE_ENTRY_RO(tbl, entry) \
@@ -149,8 +151,10 @@ extern int iommu_tce_table_put(struct iommu_table *tbl);
/* Initializes an iommu_table based in values set in the passed-in
* structure
*/
-extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
- int nid);
+extern struct iommu_table *iommu_init_table_res(struct iommu_table *tbl,
+ int nid, unsigned long res_start, unsigned long res_end);
+#define iommu_init_table(tbl, nid) iommu_init_table_res((tbl), (nid), 0, 0)
+
#define IOMMU_TABLE_GROUP_MAX_TABLES 2
struct iommu_table_group;
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 0a67ce9f827e..2b501dc15352 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -633,11 +633,54 @@ static void iommu_table_clear(struct iommu_table *tbl)
#endif
}
+static void iommu_table_reserve_pages(struct iommu_table *tbl,
+ unsigned long res_start, unsigned long res_end)
+{
+ int i;
+
+ WARN_ON_ONCE(res_end >= res_start);
+ /*
+ * Reserve page 0 so it will not be used for any mappings.
+ * This avoids buggy drivers that consider page 0 to be invalid
+ * to crash the machine or even lose data.
+ */
+ if (tbl->it_offset == 0)
+ set_bit(0, tbl->it_map);
+
+ tbl->it_reserved_start = res_start;
+ tbl->it_reserved_end = res_end;
+
+ /* Check if res_start..res_end isn't empty and overlaps the table */
+ if (res_start && res_end &&
+ (tbl->it_offset + tbl->it_size < res_start ||
+ res_end < tbl->it_offset))
+ return;
+
+ for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
+ set_bit(i - tbl->it_offset, tbl->it_map);
+}
+
+static void iommu_table_release_pages(struct iommu_table *tbl)
+{
+ int i;
+
+ /*
+ * In case we have reserved the first bit, we should not emit
+ * the warning below.
+ */
+ if (tbl->it_offset == 0)
+ clear_bit(0, tbl->it_map);
+
+ for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
+ clear_bit(i - tbl->it_offset, tbl->it_map);
+}
+
/*
* Build a iommu_table structure. This contains a bit map which
* is used to manage allocation of the tce space.
*/
-struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
+struct iommu_table *iommu_init_table_res(struct iommu_table *tbl, int nid,
+ unsigned long res_start, unsigned long res_end)
{
unsigned long sz;
static int welcomed = 0;
@@ -656,13 +699,7 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
tbl->it_map = page_address(page);
memset(tbl->it_map, 0, sz);
- /*
- * Reserve page 0 so it will not be used for any mappings.
- * This avoids buggy drivers that consider page 0 to be invalid
- * to crash the machine or even lose data.
- */
- if (tbl->it_offset == 0)
- set_bit(0, tbl->it_map);
+ iommu_table_reserve_pages(tbl, res_start, res_end);
/* We only split the IOMMU table if we have 1GB or more of space */
if ((tbl->it_size << tbl->it_page_shift) >= (1UL * 1024 * 1024 * 1024))
@@ -714,12 +751,7 @@ static void iommu_table_free(struct kref *kref)
return;
}
- /*
- * In case we have reserved the first bit, we should not emit
- * the warning below.
- */
- if (tbl->it_offset == 0)
- clear_bit(0, tbl->it_map);
+ iommu_table_release_pages(tbl);
/* verify that table contains no entries */
if (!bitmap_empty(tbl->it_map, tbl->it_size))
@@ -1024,15 +1056,14 @@ int iommu_take_ownership(struct iommu_table *tbl)
for (i = 0; i < tbl->nr_pools; i++)
spin_lock(&tbl->pools[i].lock);
- if (tbl->it_offset == 0)
- clear_bit(0, tbl->it_map);
+ iommu_table_release_pages(tbl);
if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
pr_err("iommu_tce: it_map is not empty");
ret = -EBUSY;
- /* Restore bit#0 set by iommu_init_table() */
- if (tbl->it_offset == 0)
- set_bit(0, tbl->it_map);
+ /* Undo iommu_table_release_pages, i.e. restore bit#0, etc */
+ iommu_table_reserve_pages(tbl, tbl->it_reserved_start,
+ tbl->it_reserved_end);
} else {
memset(tbl->it_map, 0xff, sz);
}
@@ -1055,9 +1086,8 @@ void iommu_release_ownership(struct iommu_table *tbl)
memset(tbl->it_map, 0, sz);
- /* Restore bit#0 set by iommu_init_table() */
- if (tbl->it_offset == 0)
- set_bit(0, tbl->it_map);
+ iommu_table_reserve_pages(tbl, tbl->it_reserved_start,
+ tbl->it_reserved_end);
for (i = 0; i < tbl->nr_pools; i++)
spin_unlock(&tbl->pools[i].lock);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index d8080558d020..874909c6f314 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2420,6 +2420,7 @@ static long pnv_pci_ioda2_setup_default_config(struct pnv_ioda_pe *pe)
{
struct iommu_table *tbl = NULL;
long rc;
+ unsigned long res_start, res_end;
/*
* crashkernel= specifies the kdump kernel's maximum memory at
@@ -2433,19 +2434,46 @@ static long pnv_pci_ioda2_setup_default_config(struct pnv_ioda_pe *pe)
* DMA window can be larger than available memory, which will
* cause errors later.
*/
- const u64 window_size = min((u64)pe->table_group.tce32_size, max_memory);
+ const u64 maxblock = 1UL << (PAGE_SHIFT + MAX_ORDER - 1);
- rc = pnv_pci_ioda2_create_table(&pe->table_group, 0,
- IOMMU_PAGE_SHIFT_4K,
- window_size,
- POWERNV_IOMMU_DEFAULT_LEVELS, false, &tbl);
+ /*
+ * We create the default window as big as we can. The constraint is
+ * the max order of allocation possible. The TCE tableis likely to
+ * end up being multilevel and with on-demand allocation in place,
+ * the initial use is not going to be huge as the default window aims
+ * to support cripplied devices (i.e. not fully 64bit DMAble) only.
+ */
+ /* iommu_table::it_map uses 1 bit per IOMMU page, hence 8 */
+ const u64 window_size = min((maxblock * 8) << PAGE_SHIFT, max_memory);
+ /* Each TCE level cannot exceed maxblock so go multilevel if needed */
+ unsigned long tces_order = ilog2(window_size >> PAGE_SHIFT);
+ unsigned long tcelevel_order = ilog2(maxblock >> 3);
+ unsigned int levels = tces_order / tcelevel_order;
+
+ if (tces_order % tcelevel_order)
+ levels += 1;
+ /*
+ * We try to stick to default levels (which is >1 at the moment) in
+ * order to save memory by relying on on-demain TCE level allocation.
+ */
+ levels = max_t(unsigned int, levels, POWERNV_IOMMU_DEFAULT_LEVELS);
+
+ rc = pnv_pci_ioda2_create_table(&pe->table_group, 0, PAGE_SHIFT,
+ window_size, levels, false, &tbl);
if (rc) {
pe_err(pe, "Failed to create 32-bit TCE table, err %ld",
rc);
return rc;
}
- iommu_init_table(tbl, pe->phb->hose->node);
+ /* We use top part of 32bit space for MMIO so exclude it from DMA */
+ res_start = 0;
+ res_end = 0;
+ if (window_size > pe->phb->ioda.m32_pci_base) {
+ res_start = pe->phb->ioda.m32_pci_base >> tbl->it_page_shift;
+ res_end = min(window_size, SZ_4G) >> tbl->it_page_shift;
+ }
+ iommu_init_table_res(tbl, pe->phb->hose->node, res_start, res_end);
rc = pnv_pci_ioda2_set_window(&pe->table_group, 0, tbl);
if (rc) {
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH kernel v4 4/4] powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU pages
2019-07-12 9:45 ` [PATCH kernel v4 4/4] powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU pages Alexey Kardashevskiy
@ 2019-07-12 15:25 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2019-07-12 15:25 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
linuxppc-dev, David Gibson
> -extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
> - int nid);
> +extern struct iommu_table *iommu_init_table_res(struct iommu_table *tbl,
> + int nid, unsigned long res_start, unsigned long res_end);
> +#define iommu_init_table(tbl, nid) iommu_init_table_res((tbl), (nid), 0, 0)
I'd just pass the two additional paramters to iommu_init_table, there
are only 10 callers of it.
> + * the max order of allocation possible. The TCE tableis likely to
Missing whitespace between tabke and is.
> + * end up being multilevel and with on-demand allocation in place,
> + * the initial use is not going to be huge as the default window aims
> + * to support cripplied devices (i.e. not fully 64bit DMAble) only.
s/cripplied/crippled/
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59
2019-07-12 9:45 [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Alexey Kardashevskiy
` (3 preceding siblings ...)
2019-07-12 9:45 ` [PATCH kernel v4 4/4] powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU pages Alexey Kardashevskiy
@ 2019-07-12 15:28 ` Christoph Hellwig
2019-07-15 8:43 ` Alexey Kardashevskiy
4 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2019-07-12 15:28 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
linuxppc-dev, David Gibson
On Fri, Jul 12, 2019 at 07:45:05PM +1000, Alexey Kardashevskiy wrote:
> This is an attempt to allow DMA masks between 32..59 which are not large
> enough to use either a PHB3 bypass mode or a sketchy bypass. Depending
> on the max order, up to 40 is usually available.
Can you elaborate what you man with supported in detail? In the end
a DMA devices DMA capability is only really interesting as a lower
bound.
e.g. if you have a DMA that supports 40-bit DMA addressing we could
always treat it as if supports 32-bit addressing, and I thought the
powerpc code does that, as the DMA API now relies on that. Did I miss
something and it explicitly rejected that (in which case I didn't spot
the fix in this series), or is this just an optimization to handle these
devices more optimally, in which case maybe the changelog could be
improved a bit.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59
2019-07-12 15:28 ` [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59 Christoph Hellwig
@ 2019-07-15 8:43 ` Alexey Kardashevskiy
2019-07-15 12:39 ` Christoph Hellwig
0 siblings, 1 reply; 12+ messages in thread
From: Alexey Kardashevskiy @ 2019-07-15 8:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
linuxppc-dev, David Gibson
On 13/07/2019 01:28, Christoph Hellwig wrote:
> On Fri, Jul 12, 2019 at 07:45:05PM +1000, Alexey Kardashevskiy wrote:
>> This is an attempt to allow DMA masks between 32..59 which are not large
>> enough to use either a PHB3 bypass mode or a sketchy bypass. Depending
>> on the max order, up to 40 is usually available.
>
> Can you elaborate what you man with supported in detail? In the end
> a DMA devices DMA capability is only really interesting as a lower
> bound.
>
> e.g. if you have a DMA that supports 40-bit DMA addressing we could
> always treat it as if supports 32-bit addressing,and I thought the
> powerpc code does that,
powerpc does that and this is what the patchset is changing as people
complained that 2GB DMA window has bad effects on AMD GPUs (cannot
allocate enough buffers) and 40/100Gbit devices (lower performance), I
do not have the details handy.
> as the DMA API now relies on that.
Relies on what precisely? If a device cannot do full 64bit, then it has
to be no more than just 32bit?
> Did I miss
> something and it explicitly rejected that (in which case I didn't spot
> the fix in this series), or is this just an optimization to handle these
> devices more optimally, in which case maybe the changelog could be
> improved a bit.
4/4 did this essentially:
- const u64 window_size = min((u64)pe->table_group.tce32_size, max_memory);
+ const u64 maxblock = 1UL << (PAGE_SHIFT + MAX_ORDER - 1);
+ const u64 window_size = min((maxblock * 8) << PAGE_SHIFT, max_memory);
where tce32_size==2<<30. The 4/4 commit log has the details, what is
missing there? Thanks,
--
Alexey
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59
2019-07-15 8:43 ` Alexey Kardashevskiy
@ 2019-07-15 12:39 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2019-07-15 12:39 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Sam Bobroff, Alistair Popple, Christoph Hellwig,
Oliver O'Halloran, linuxppc-dev, David Gibson
On Mon, Jul 15, 2019 at 06:43:12PM +1000, Alexey Kardashevskiy wrote:
> > e.g. if you have a DMA that supports 40-bit DMA addressing we could
> > always treat it as if supports 32-bit addressing,and I thought the
> > powerpc code does that,
>
> powerpc does that and this is what the patchset is changing as people
> complained that 2GB DMA window has bad effects on AMD GPUs (cannot allocate
> enough buffers) and 40/100Gbit devices (lower performance), I do not have
> the details handy.
Make sense. I'm just surprised about the complains from the habalabs
folks, which sounded like a 40something bit DMA mask did not work at all
for them on power9, which did not fit my reading of the code.
>
> > as the DMA API now relies on that.
>
> Relies on what precisely? If a device cannot do full 64bit, then it has to
> be no more than just 32bit?
The fact that if say you iommu only supports mode that return up to
32-bit iova and a driver sets a 48 or 64-bit mask you still return
success instead of letting the driver handle the failure and set a
32-bit mask in the fallback code. As said I think the powerpc code
is fine based on my reading from it.
> > Did I miss
> > something and it explicitly rejected that (in which case I didn't spot
> > the fix in this series), or is this just an optimization to handle these
> > devices more optimally, in which case maybe the changelog could be
> > improved a bit.
>
>
> 4/4 did this essentially:
As long as the above is fine (which I think it is) just make it a little
mor clear that this is a simple optimization, not a bug fix for DMA API
usage.
^ permalink raw reply [flat|nested] 12+ messages in thread