Hi,
Commit 249baa547901 ("dma-mapping: provide a better default
->get_required_mask") causes an error on ehci-pci for me.
Either reverting the commit or disabling iommu=pt seems to fix this.
[ 9.000081] usb 1-1: new high-speed USB device number 2 using ehci-pci
[ 9.000755] ehci-pci 0000:00:1a.0: swiotlb buffer is full (sz: 8 bytes), total 0 (slots), used 0 (slots)
[ 9.001179] ehci-pci 0000:00:1a.0: overflow 0x000000042f541790+8 of DMA mask ffffffff bus mask 0
[ 9.001552] ------------[ cut here ]------------
[ 9.001933] WARNING: CPU: 0 PID: 7 at kernel/dma/direct.c:35 report_addr+0x3c/0x70
[ 9.002355] Modules linked in:
[ 9.002802] CPU: 0 PID: 7 Comm: kworker/0:1 Not tainted 5.4.0-rc2-stable-rani-zfs+ #67
[ 9.003270] Hardware name: ASUSTeK COMPUTER INC. Z10PE-D8 WS/Z10PE-D8 WS, BIOS 3703 04/13/2018
[ 9.003761] Workqueue: usb_hub_wq hub_event
[ 9.004241] RIP: 0010:report_addr+0x3c/0x70
[ 9.004722] Code: 48 89 34 24 48 8b 85 e8 01 00 00 48 85 c0 74 30 4c 8b 00 b8 fe ff ff ff 49 39 c0 76 17 80 3d 86 6f 23 01 00 0f 84 df 06 00 00 <0f> 0b 48 83 c4 08 5d 41 5c c3 48 83 bd f8 01 00 00 00 74 ec eb dd
[ 9.005743] RSP: 0000:ffffb25706307af8 EFLAGS: 00010286
[ 9.006261] RAX: 0000000000000000 RBX: ffffe0e3d0bd5040 RCX: 00000000000006fc
[ 9.006783] RDX: 0000000000000000 RSI: 0000000000000092 RDI: ffffffffab163fe8
[ 9.007301] RBP: ffffa38c678a00b0 R08: 0000000000000000 R09: 00000000000006fc
[ 9.007826] R10: 0000000000aaaaaa R11: 00000000ff000000 R12: 0000000000000008
[ 9.008339] R13: 0000000000000000 R14: 0000000000000790 R15: ffffa38c678a00b0
[ 9.008849] FS: 0000000000000000(0000) GS:ffffa38b5f600000(0000) knlGS:0000000000000000
[ 9.009355] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9.009866] CR2: ffffa396b4001000 CR3: 00000013b340a001 CR4: 00000000003606f0
[ 9.010498] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 9.011034] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 9.011580] Call Trace:
[ 9.012110] dma_direct_map_page+0xf8/0x110
[ 9.012632] usb_hcd_map_urb_for_dma+0x204/0x530
[ 9.013149] usb_hcd_submit_urb+0x375/0xb70
[ 9.013668] usb_start_wait_urb+0x8a/0x190
[ 9.014188] usb_control_msg+0xe5/0x150
[ 9.014693] hub_port_init+0x21b/0xb40
[ 9.015190] hub_event+0xb21/0x14f0
[ 9.015710] process_one_work+0x1e5/0x390
[ 9.016215] worker_thread+0x4d/0x3d0
[ 9.016720] kthread+0xfd/0x130
[ 9.017207] ? process_one_work+0x390/0x390
[ 9.017687] ? kthread_park+0x90/0x90
[ 9.018163] ret_from_fork+0x3a/0x50
[ 9.018642] ---[ end trace 55eaa8968ea11ab5 ]---
[ 9.019124] ehci-pci 0000:00:1a.0: swiotlb buffer is full (sz: 8 bytes), total 0 (slots), used 0 (slots)
cpuinfo:
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 79
model name : Intel(R) Xeon(R) CPU E5-2696 v4 @ 2.20GHz
stepping : 1
microcode : 0xb000038
cpu MHz : 3353.679
cache size : 28160 KB
physical id : 0
siblings : 44
core id : 0
cpu cores : 22
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 20
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc cpuid aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm 3dnowprefetch cpuid_fault epb cat_l3 cdp_l3 invpcid_single pti intel_ppin ssbd ibrs ibpb stibp tpr_shadow vnmi flexpriority ept vpid ept_ad fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm cqm rdt_a rdseed adx smap intel_pt xsaveopt cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local dtherm ida arat pln pts hwp md_clear flush_l1d
bugs : cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf mds swapgs
bogomips : 4390.26
clflush size : 64
cache_alignment : 64
address sizes : 46 bits physical, 48 bits virtual
power management:
lspci:
00:1a.0 USB controller: Intel Corporation C610/X99 series chipset USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
Subsystem: ASUSTeK Computer Inc. C610/X99 series chipset USB Enhanced Host Controller
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin C routed to IRQ 19
NUMA node: 0
Region 0: Memory at bc112000 (32-bit, non-prefetchable) [size=1K]
Capabilities: [50] Power Management version 2
Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [58] Debug port: BAR=1 offset=00a0
Capabilities: [98] PCI Advanced Features
AFCap: TP+ FLR+
AFCtrl: FLR-
AFStatus: TP-
Kernel driver in use: ehci-pci
Hi Arvind,
can you try the patch below?
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3f974919d3bd..52b709bf2b55 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3775,6 +3775,13 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
return nelems;
}
+static u64 intel_get_required_mask(struct device *dev)
+{
+ if (!iommu_need_mapping(dev))
+ return dma_direct_get_required_mask(dev);
+ return DMA_BIT_MASK(32);
+}
+
static const struct dma_map_ops intel_dma_ops = {
.alloc = intel_alloc_coherent,
.free = intel_free_coherent,
@@ -3787,6 +3794,7 @@ static const struct dma_map_ops intel_dma_ops = {
.dma_supported = dma_direct_supported,
.mmap = dma_common_mmap,
.get_sgtable = dma_common_get_sgtable,
+ .get_required_mask = intel_get_required_mask,
};
static void
On Mon, Oct 07, 2019 at 09:34:48AM +0200, Christoph Hellwig wrote:
> Hi Arvind,
>
> can you try the patch below?
>
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3f974919d3bd..52b709bf2b55 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3775,6 +3775,13 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
> return nelems;
> }
>
> +static u64 intel_get_required_mask(struct device *dev)
> +{
> + if (!iommu_need_mapping(dev))
> + return dma_direct_get_required_mask(dev);
> + return DMA_BIT_MASK(32);
> +}
> +
> static const struct dma_map_ops intel_dma_ops = {
> .alloc = intel_alloc_coherent,
> .free = intel_free_coherent,
> @@ -3787,6 +3794,7 @@ static const struct dma_map_ops intel_dma_ops = {
> .dma_supported = dma_direct_supported,
> .mmap = dma_common_mmap,
> .get_sgtable = dma_common_get_sgtable,
> + .get_required_mask = intel_get_required_mask,
> };
>
> static void
It doesn't boot with the patch. Won't it go
dma_get_required_mask
-> intel_get_required_mask
-> iommu_need_mapping
-> dma_get_required_mask
?
Should the call to dma_get_required_mask in iommu_need_mapping be
replaced with dma_direct_get_required_mask on top of your patch?
On Mon, Oct 07, 2019 at 07:55:28PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> > It doesn't boot with the patch. Won't it go
> > dma_get_required_mask
> > -> intel_get_required_mask
> > -> iommu_need_mapping
> > -> dma_get_required_mask
> > ?
> >
> > Should the call to dma_get_required_mask in iommu_need_mapping be
> > replaced with dma_direct_get_required_mask on top of your patch?
>
> Yes, sorry.
Actually my patch already calls dma_direct_get_required_mask.
How did you get the loop?
On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> It doesn't boot with the patch. Won't it go
> dma_get_required_mask
> -> intel_get_required_mask
> -> iommu_need_mapping
> -> dma_get_required_mask
> ?
>
> Should the call to dma_get_required_mask in iommu_need_mapping be
> replaced with dma_direct_get_required_mask on top of your patch?
Yes, sorry.
On Mon, Oct 07, 2019 at 10:56:30AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 07, 2019 at 07:55:28PM +0200, Christoph Hellwig wrote:
> > On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> > > It doesn't boot with the patch. Won't it go
> > > dma_get_required_mask
> > > -> intel_get_required_mask
> > > -> iommu_need_mapping
> > > -> dma_get_required_mask
> > > ?
> > >
> > > Should the call to dma_get_required_mask in iommu_need_mapping be
> > > replaced with dma_direct_get_required_mask on top of your patch?
> >
> > Yes, sorry.
>
> Actually my patch already calls dma_direct_get_required_mask.
> How did you get the loop?
The function iommu_need_mapping (not changed by your patch) calls
dma_get_required_mask internally, to check whether the device's dma_mask
is big enough or not. That's the call I was asking whether it needs to
be changed.
On Mon, Oct 07, 2019 at 01:58:57PM -0400, Arvind Sankar wrote:
> On Mon, Oct 07, 2019 at 10:56:30AM -0700, Christoph Hellwig wrote:
> > On Mon, Oct 07, 2019 at 07:55:28PM +0200, Christoph Hellwig wrote:
> > > On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> > > > It doesn't boot with the patch. Won't it go
> > > > dma_get_required_mask
> > > > -> intel_get_required_mask
> > > > -> iommu_need_mapping
> > > > -> dma_get_required_mask
> > > > ?
> > > >
> > > > Should the call to dma_get_required_mask in iommu_need_mapping be
> > > > replaced with dma_direct_get_required_mask on top of your patch?
> > >
> > > Yes, sorry.
> >
> > Actually my patch already calls dma_direct_get_required_mask.
> > How did you get the loop?
>
> The function iommu_need_mapping (not changed by your patch) calls
> dma_get_required_mask internally, to check whether the device's dma_mask
> is big enough or not. That's the call I was asking whether it needs to
> be changed.
Yeah the attached patch seems to fix it.
On Mon, Oct 07, 2019 at 02:32:07PM -0400, Arvind Sankar wrote:
> On Mon, Oct 07, 2019 at 01:58:57PM -0400, Arvind Sankar wrote:
> > On Mon, Oct 07, 2019 at 10:56:30AM -0700, Christoph Hellwig wrote:
> > > On Mon, Oct 07, 2019 at 07:55:28PM +0200, Christoph Hellwig wrote:
> > > > On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> > > > > It doesn't boot with the patch. Won't it go
> > > > > dma_get_required_mask
> > > > > -> intel_get_required_mask
> > > > > -> iommu_need_mapping
> > > > > -> dma_get_required_mask
> > > > > ?
> > > > >
> > > > > Should the call to dma_get_required_mask in iommu_need_mapping be
> > > > > replaced with dma_direct_get_required_mask on top of your patch?
> > > >
> > > > Yes, sorry.
> > >
> > > Actually my patch already calls dma_direct_get_required_mask.
> > > How did you get the loop?
> >
> > The function iommu_need_mapping (not changed by your patch) calls
> > dma_get_required_mask internally, to check whether the device's dma_mask
> > is big enough or not. That's the call I was asking whether it needs to
> > be changed.
>
> Yeah the attached patch seems to fix it.
That looks fine to me:
Acked-by: Christoph Hellwig <[email protected]>
On Mon, Oct 07, 2019 at 08:47:54PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 07, 2019 at 02:32:07PM -0400, Arvind Sankar wrote:
> > On Mon, Oct 07, 2019 at 01:58:57PM -0400, Arvind Sankar wrote:
> > > On Mon, Oct 07, 2019 at 10:56:30AM -0700, Christoph Hellwig wrote:
> > > > On Mon, Oct 07, 2019 at 07:55:28PM +0200, Christoph Hellwig wrote:
> > > > > On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> > > > > > It doesn't boot with the patch. Won't it go
> > > > > > dma_get_required_mask
> > > > > > -> intel_get_required_mask
> > > > > > -> iommu_need_mapping
> > > > > > -> dma_get_required_mask
> > > > > > ?
> > > > > >
> > > > > > Should the call to dma_get_required_mask in iommu_need_mapping be
> > > > > > replaced with dma_direct_get_required_mask on top of your patch?
> > > > >
> > > > > Yes, sorry.
> > > >
> > > > Actually my patch already calls dma_direct_get_required_mask.
> > > > How did you get the loop?
> > >
> > > The function iommu_need_mapping (not changed by your patch) calls
> > > dma_get_required_mask internally, to check whether the device's dma_mask
> > > is big enough or not. That's the call I was asking whether it needs to
> > > be changed.
> >
> > Yeah the attached patch seems to fix it.
>
> That looks fine to me:
>
> Acked-by: Christoph Hellwig <[email protected]>
Do you want me to resend the patch as its own mail, or do you just take
it with a Tested-by: from me? If the former, I assume you're ok with me
adding your Signed-off-by?
Thanks
On Mon, Oct 07, 2019 at 06:10:55PM -0400, Arvind Sankar wrote:
> On Mon, Oct 07, 2019 at 08:47:54PM +0200, Christoph Hellwig wrote:
> > On Mon, Oct 07, 2019 at 02:32:07PM -0400, Arvind Sankar wrote:
> > > On Mon, Oct 07, 2019 at 01:58:57PM -0400, Arvind Sankar wrote:
> > > > On Mon, Oct 07, 2019 at 10:56:30AM -0700, Christoph Hellwig wrote:
> > > > > On Mon, Oct 07, 2019 at 07:55:28PM +0200, Christoph Hellwig wrote:
> > > > > > On Mon, Oct 07, 2019 at 01:54:32PM -0400, Arvind Sankar wrote:
> > > > > > > It doesn't boot with the patch. Won't it go
> > > > > > > dma_get_required_mask
> > > > > > > -> intel_get_required_mask
> > > > > > > -> iommu_need_mapping
> > > > > > > -> dma_get_required_mask
> > > > > > > ?
> > > > > > >
> > > > > > > Should the call to dma_get_required_mask in iommu_need_mapping be
> > > > > > > replaced with dma_direct_get_required_mask on top of your patch?
> > > > > >
> > > > > > Yes, sorry.
> > > > >
> > > > > Actually my patch already calls dma_direct_get_required_mask.
> > > > > How did you get the loop?
> > > >
> > > > The function iommu_need_mapping (not changed by your patch) calls
> > > > dma_get_required_mask internally, to check whether the device's dma_mask
> > > > is big enough or not. That's the call I was asking whether it needs to
> > > > be changed.
> > >
> > > Yeah the attached patch seems to fix it.
> >
> > That looks fine to me:
> >
> > Acked-by: Christoph Hellwig <[email protected]>
>
> Do you want me to resend the patch as its own mail, or do you just take
> it with a Tested-by: from me? If the former, I assume you're ok with me
> adding your Signed-off-by?
>
> Thanks
A question on the original change though -- what happens if a single
device (or a single IOMMU domain really) does want >4G DMA address
space? Was that not previously allowed either?
On Mon, Oct 07, 2019 at 06:10:55PM -0400, Arvind Sankar wrote:
> > Acked-by: Christoph Hellwig <[email protected]>
>
> Do you want me to resend the patch as its own mail, or do you just take
> it with a Tested-by: from me? If the former, I assume you're ok with me
> adding your Signed-off-by?
Either way is fine with me. The original patch was from me, but you
actually did all the work of reporting it, fixing it and testing it so
I'm perfectly fine with you sending it under your name.
On Mon, Oct 07, 2019 at 07:54:02PM -0400, Arvind Sankar wrote:
> > Do you want me to resend the patch as its own mail, or do you just take
> > it with a Tested-by: from me? If the former, I assume you're ok with me
> > adding your Signed-off-by?
> >
> > Thanks
>
> A question on the original change though -- what happens if a single
> device (or a single IOMMU domain really) does want >4G DMA address
> space? Was that not previously allowed either?
Your EHCI device actually supports the larger addressing. Without an
IOMMU (or with accidentally enabled passthrough mode as in your report)
that will use bounce buffers for physical address that are too large.
With an iommu we can just remap, and by default those remap addresses
are under 32-bit just to make everyones life easier.
The dma_get_required_mask function is misnamed unfortunately, what it
really means is the optimal mask, that is one that avoids bounce
buffering or other complications.
On Tue, Oct 08, 2019 at 09:32:10AM +0200, Christoph Hellwig wrote:
> On Mon, Oct 07, 2019 at 07:54:02PM -0400, Arvind Sankar wrote:
> > > Do you want me to resend the patch as its own mail, or do you just take
> > > it with a Tested-by: from me? If the former, I assume you're ok with me
> > > adding your Signed-off-by?
> > >
> > > Thanks
> >
> > A question on the original change though -- what happens if a single
> > device (or a single IOMMU domain really) does want >4G DMA address
> > space? Was that not previously allowed either?
>
> Your EHCI device actually supports the larger addressing. Without an
> IOMMU (or with accidentally enabled passthrough mode as in your report)
> that will use bounce buffers for physical address that are too large.
> With an iommu we can just remap, and by default those remap addresses
> are under 32-bit just to make everyones life easier.
>
> The dma_get_required_mask function is misnamed unfortunately, what it
> really means is the optimal mask, that is one that avoids bounce
> buffering or other complications.
I understand that my EHCI device, even though it only supports 32-bit
adddressing, will be able to DMA into anywhere in physical RAM, whether
below 4G or not, via the IOMMU or bounce buffering.
What I mean is, do there exist devices (which would necessarily support
64-bit DMA) that want to DMA using bigger than 4Gb buffers. Eg a GPU
accelerator card with 16Gb of RAM on-board that wants to map 6Gb for DMA
in one go, or 5 accelerator cards that are in one IOMMU domain and want
to simultaneously map 1Gb each.
On Tue, Oct 08, 2019 at 07:51:03AM -0400, Arvind Sankar wrote:
> What I mean is, do there exist devices (which would necessarily support
> 64-bit DMA) that want to DMA using bigger than 4Gb buffers. Eg a GPU
> accelerator card with 16Gb of RAM on-board that wants to map 6Gb for DMA
> in one go, or 5 accelerator cards that are in one IOMMU domain and want
> to simultaneously map 1Gb each.
If you allocate more than 32-bits worth of address space the IOMMU
address space allocator will dip into 64-bit values if the device
dma mask supports that.
We must return a mask covering the full physical RAM when bypassing the
IOMMU mapping. Also, in iommu_need_mapping, we need to check using
dma_direct_get_required_mask to ensure that the device's dma_mask can
cover physical RAM before deciding to bypass IOMMU mapping.
Fixes: 249baa547901 ("dma-mapping: provide a better default ->get_required_mask")
Reported-by: Arvind Sankar <[email protected]>
Tested-by: Arvind Sankar <[email protected]>
Originally-by: Christoph Hellwig <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
Fixed-by: Arvind Sankar <[email protected]>
Signed-off-by: Arvind Sankar <[email protected]>
---
drivers/iommu/intel-iommu.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3f974919d3bd..79e35b3180ac 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3471,7 +3471,7 @@ static bool iommu_need_mapping(struct device *dev)
if (dev->coherent_dma_mask && dev->coherent_dma_mask < dma_mask)
dma_mask = dev->coherent_dma_mask;
- if (dma_mask >= dma_get_required_mask(dev))
+ if (dma_mask >= dma_direct_get_required_mask(dev))
return false;
/*
@@ -3775,6 +3775,13 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
return nelems;
}
+static u64 intel_get_required_mask(struct device *dev)
+{
+ if (!iommu_need_mapping(dev))
+ return dma_direct_get_required_mask(dev);
+ return DMA_BIT_MASK(32);
+}
+
static const struct dma_map_ops intel_dma_ops = {
.alloc = intel_alloc_coherent,
.free = intel_free_coherent,
@@ -3787,6 +3794,7 @@ static const struct dma_map_ops intel_dma_ops = {
.dma_supported = dma_direct_supported,
.mmap = dma_common_mmap,
.get_sgtable = dma_common_get_sgtable,
+ .get_required_mask = intel_get_required_mask,
};
static void
--
2.21.0
On Tue, Oct 08, 2019 at 07:51:03AM -0400, Arvind Sankar wrote:
> On Tue, Oct 08, 2019 at 09:32:10AM +0200, Christoph Hellwig wrote:
> > On Mon, Oct 07, 2019 at 07:54:02PM -0400, Arvind Sankar wrote:
> > > > Do you want me to resend the patch as its own mail, or do you just take
> > > > it with a Tested-by: from me? If the former, I assume you're ok with me
> > > > adding your Signed-off-by?
> > > >
> > > > Thanks
> > >
> > > A question on the original change though -- what happens if a single
> > > device (or a single IOMMU domain really) does want >4G DMA address
> > > space? Was that not previously allowed either?
> >
> > Your EHCI device actually supports the larger addressing. Without an
> > IOMMU (or with accidentally enabled passthrough mode as in your report)
> > that will use bounce buffers for physical address that are too large.
> > With an iommu we can just remap, and by default those remap addresses
> > are under 32-bit just to make everyones life easier.
> >
> > The dma_get_required_mask function is misnamed unfortunately, what it
> > really means is the optimal mask, that is one that avoids bounce
> > buffering or other complications.
>
> I understand that my EHCI device, even though it only supports 32-bit
> adddressing, will be able to DMA into anywhere in physical RAM, whether
> below 4G or not, via the IOMMU or bounce buffering.
>
> What I mean is, do there exist devices (which would necessarily support
> 64-bit DMA) that want to DMA using bigger than 4Gb buffers. Eg a GPU
> accelerator card with 16Gb of RAM on-board that wants to map 6Gb for DMA
> in one go, or 5 accelerator cards that are in one IOMMU domain and want
> to simultaneously map 1Gb each.
Ok, I see that almost nothing actually uses dma_get_required_mask. So if
something did need >4Gb space, the IOMMU would allocate it anyway
regardless of dma_get_required_mask.
I'm thinking this means that we actually only needed to change
dma_get_required_mask to dma_direct_get_required_mask in
iommu_need_mapping, and the rest of the change is unnecessary?
Below is list of stuff calling dma_get_required_mask currently:
/usr/src/git/linux # find . -type f -name \*.[ch] | xargs grep -w dma_get_required_mask
./drivers/mmc/host/sdhci-of-dwcmshc.c: extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
./drivers/pci/controller/vmd.c: return dma_get_required_mask(to_vmd_dev(dev));
./drivers/gpu/drm/etnaviv/etnaviv_gpu.c: u32 dma_mask = (u32)dma_get_required_mask(gpu->dev);
./drivers/message/fusion/mptbase.c: const uint64_t required_mask = dma_get_required_mask
./drivers/scsi/dpt_i2o.c: dma_get_required_mask(&pDev->dev) > DMA_BIT_MASK(32) &&
./drivers/scsi/aic7xxx/aic79xx_osm_pci.c: const u64 required_mask = dma_get_required_mask(dev);
./drivers/scsi/aic7xxx/aic7xxx_osm_pci.c: && dma_get_required_mask(dev) > DMA_BIT_MASK(32)) {
./drivers/scsi/mpt3sas/mpt3sas_base.c: required_mask = dma_get_required_mask(&pdev->dev);
./drivers/scsi/qla2xxx/qla_os.c: if (MSD(dma_get_required_mask(&ha->pdev->dev)) &&
./drivers/scsi/aacraid/aachba.c: if (dma_get_required_mask(&dev->pdev->dev) > DMA_BIT_MASK(32))
./drivers/scsi/aacraid/comminit.c: dma_get_required_mask(&dev->pdev->dev) >> 12;
./drivers/scsi/esas2r/esas2r_init.c: dma_get_required_mask(&pcid->dev) > DMA_BIT_MASK(32) &&
./drivers/infiniband/sw/rxe/rxe_verbs.c: dma_get_required_mask(&dev->dev));
./include/linux/dma-mapping.h:u64 dma_get_required_mask(struct device *dev);
./include/linux/dma-mapping.h:static inline u64 dma_get_required_mask(struct device *dev)
./include/linux/dma-mapping.h: dma_get_required_mask(dev);
./kernel/dma/mapping.c:u64 dma_get_required_mask(struct device *dev)
./kernel/dma/mapping.c:EXPORT_SYMBOL_GPL(dma_get_required_mask);
For the drivers that do currently use dma_get_required_mask, I believe
we will need to replace most of them with dma_direct_get_required_mask
as well to maintain passthrough functionality -- the fusion, scsi, and
infinband drivers all seem to be using this call to determine whether to
expose the device's 64-bit DMA capability or not. With the change they
will think they only need 32-bit DMA, which in turn will disable
passthrough on them.
The etnaviv driver is doing something funky that I'm not sure about, but
I *think* that one might want the real physical range as well. The mmc
driver I think might be ok with the 32-bit range.
The vmd controller one not sure about.
In dma-mapping.h, the function is used in dma_addressing_limited, which
in turn is used in a couple of amd drm drivers, again not sure about
these.
Hi,
On 10/8/19 10:33 PM, Arvind Sankar wrote:
> We must return a mask covering the full physical RAM when bypassing the
> IOMMU mapping. Also, in iommu_need_mapping, we need to check using
> dma_direct_get_required_mask to ensure that the device's dma_mask can
> cover physical RAM before deciding to bypass IOMMU mapping.
>
> Fixes: 249baa547901 ("dma-mapping: provide a better default ->get_required_mask")
> Reported-by: Arvind Sankar <[email protected]>
> Tested-by: Arvind Sankar <[email protected]>
> Originally-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
> Fixed-by: Arvind Sankar <[email protected]>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> drivers/iommu/intel-iommu.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3f974919d3bd..79e35b3180ac 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3471,7 +3471,7 @@ static bool iommu_need_mapping(struct device *dev)
> if (dev->coherent_dma_mask && dev->coherent_dma_mask < dma_mask)
> dma_mask = dev->coherent_dma_mask;
>
> - if (dma_mask >= dma_get_required_mask(dev))
> + if (dma_mask >= dma_direct_get_required_mask(dev))
> return false;
>
> /*
> @@ -3775,6 +3775,13 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
> return nelems;
> }
>
> +static u64 intel_get_required_mask(struct device *dev)
> +{
> + if (!iommu_need_mapping(dev))
> + return dma_direct_get_required_mask(dev);
> + return DMA_BIT_MASK(32);
Do you mind explaining why we always return 32 bit here?
Best regards,
Baolu
> +}
> +
> static const struct dma_map_ops intel_dma_ops = {
> .alloc = intel_alloc_coherent,
> .free = intel_free_coherent,
> @@ -3787,6 +3794,7 @@ static const struct dma_map_ops intel_dma_ops = {
> .dma_supported = dma_direct_supported,
> .mmap = dma_common_mmap,
> .get_sgtable = dma_common_get_sgtable,
> + .get_required_mask = intel_get_required_mask,
> };
>
> static void
>
On Tue, Oct 08, 2019 at 11:47:31AM -0400, Arvind Sankar wrote:
> Ok, I see that almost nothing actually uses dma_get_required_mask. So if
> something did need >4Gb space, the IOMMU would allocate it anyway
> regardless of dma_get_required_mask.
Yes. And with the direct mapping it also isn't an issue.
> I'm thinking this means that we actually only needed to change
> dma_get_required_mask to dma_direct_get_required_mask in
> iommu_need_mapping, and the rest of the change is unnecessary?
>
> Below is list of stuff calling dma_get_required_mask currently:
I guess that would actually work ok, but I prefer the more verbose
version as it explain what is going on, and will lead people to do
the right thing if we split the iommu vs passthrough case into
different ops (we already had a patch for that out on the list).
> For the drivers that do currently use dma_get_required_mask, I believe
> we will need to replace most of them with dma_direct_get_required_mask
> as well to maintain passthrough functionality -- the fusion, scsi, and
> infinband drivers all seem to be using this call to determine whether to
> expose the device's 64-bit DMA capability or not. With the change they
> will think they only need 32-bit DMA, which in turn will disable
> passthrough on them.
At least for some of the legacy SCSI drivers that is intentional, and
the reason why dma_get_required_mask was originally added. On actual
PCI (and PCI-X, but not PCIe which everyone uses now) the 64-bit
addressing even if supported is not very efficient as it needs two
bus cycles. So we prefer to just use the iommu.
> The etnaviv driver is doing something funky that I'm not sure about, but
> I *think* that one might want the real physical range as well. The mmc
> driver I think might be ok with the 32-bit range.
etnaviv is never used on systems with the intel iommu anyway.
> The vmd controller one not sure about.
That just passes through the dma ops to work around really stupid
intel chipsets.
> In dma-mapping.h, the function is used in dma_addressing_limited, which
> in turn is used in a couple of amd drm drivers, again not sure about
> these.
---end quoted text---
On Wed, Oct 09, 2019 at 10:45:15AM +0800, Lu Baolu wrote:
> Do you mind explaining why we always return 32 bit here?
See the comment in dma_get_required_mask().
On Wed, Oct 09, 2019 at 08:50:43AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 11:47:31AM -0400, Arvind Sankar wrote:
> > Ok, I see that almost nothing actually uses dma_get_required_mask. So if
> > something did need >4Gb space, the IOMMU would allocate it anyway
> > regardless of dma_get_required_mask.
>
> Yes. And with the direct mapping it also isn't an issue.
>
> > I'm thinking this means that we actually only needed to change
> > dma_get_required_mask to dma_direct_get_required_mask in
> > iommu_need_mapping, and the rest of the change is unnecessary?
> >
> > Below is list of stuff calling dma_get_required_mask currently:
>
> I guess that would actually work ok, but I prefer the more verbose
> version as it explain what is going on, and will lead people to do
> the right thing if we split the iommu vs passthrough case into
> different ops (we already had a patch for that out on the list).
>
> > For the drivers that do currently use dma_get_required_mask, I believe
> > we will need to replace most of them with dma_direct_get_required_mask
> > as well to maintain passthrough functionality -- the fusion, scsi, and
> > infinband drivers all seem to be using this call to determine whether to
> > expose the device's 64-bit DMA capability or not. With the change they
> > will think they only need 32-bit DMA, which in turn will disable
> > passthrough on them.
>
> At least for some of the legacy SCSI drivers that is intentional, and
> the reason why dma_get_required_mask was originally added. On actual
> PCI (and PCI-X, but not PCIe which everyone uses now) the 64-bit
> addressing even if supported is not very efficient as it needs two
> bus cycles. So we prefer to just use the iommu.
>
> > The etnaviv driver is doing something funky that I'm not sure about, but
> > I *think* that one might want the real physical range as well. The mmc
> > driver I think might be ok with the 32-bit range.
>
> etnaviv is never used on systems with the intel iommu anyway.
>
> > The vmd controller one not sure about.
>
> That just passes through the dma ops to work around really stupid
> intel chipsets.
>
> > In dma-mapping.h, the function is used in dma_addressing_limited, which
> > in turn is used in a couple of amd drm drivers, again not sure about
> > these.
> ---end quoted text---
Thanks for the detailed explanation!
That means your changes actually improve the situation for those scsi
drivers -- previously they would have been using 64-bit addressing if
physical RAM needed it, regardless of IOMMU availability/use, right?
Hi Christoph,
On 10/9/19 2:51 PM, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 10:45:15AM +0800, Lu Baolu wrote:
>> Do you mind explaining why we always return 32 bit here?
>
> See the comment in dma_get_required_mask().
>
Got it. Thank you.
Best regards,
Baolu
Hi,
On 10/8/19 10:33 PM, Arvind Sankar wrote:
> We must return a mask covering the full physical RAM when bypassing the
> IOMMU mapping. Also, in iommu_need_mapping, we need to check using
> dma_direct_get_required_mask to ensure that the device's dma_mask can
> cover physical RAM before deciding to bypass IOMMU mapping.
>
> Fixes: 249baa547901 ("dma-mapping: provide a better default ->get_required_mask")
> Reported-by: Arvind Sankar <[email protected]>
> Tested-by: Arvind Sankar <[email protected]>
> Originally-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
> Fixed-by: Arvind Sankar <[email protected]>
> Signed-off-by: Arvind Sankar <[email protected]>
This patch looks good to me.
Reviewed-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel-iommu.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3f974919d3bd..79e35b3180ac 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3471,7 +3471,7 @@ static bool iommu_need_mapping(struct device *dev)
> if (dev->coherent_dma_mask && dev->coherent_dma_mask < dma_mask)
> dma_mask = dev->coherent_dma_mask;
>
> - if (dma_mask >= dma_get_required_mask(dev))
> + if (dma_mask >= dma_direct_get_required_mask(dev))
> return false;
>
> /*
> @@ -3775,6 +3775,13 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
> return nelems;
> }
>
> +static u64 intel_get_required_mask(struct device *dev)
> +{
> + if (!iommu_need_mapping(dev))
> + return dma_direct_get_required_mask(dev);
> + return DMA_BIT_MASK(32);
> +}
> +
> static const struct dma_map_ops intel_dma_ops = {
> .alloc = intel_alloc_coherent,
> .free = intel_free_coherent,
> @@ -3787,6 +3794,7 @@ static const struct dma_map_ops intel_dma_ops = {
> .dma_supported = dma_direct_supported,
> .mmap = dma_common_mmap,
> .get_sgtable = dma_common_get_sgtable,
> + .get_required_mask = intel_get_required_mask,
> };
>
> static void
>
On Thu, Oct 10, 2019 at 09:26:37AM +0800, Lu Baolu wrote:
> Hi,
>
> On 10/8/19 10:33 PM, Arvind Sankar wrote:
> > We must return a mask covering the full physical RAM when bypassing the
> > IOMMU mapping. Also, in iommu_need_mapping, we need to check using
> > dma_direct_get_required_mask to ensure that the device's dma_mask can
> > cover physical RAM before deciding to bypass IOMMU mapping.
> >
> > Fixes: 249baa547901 ("dma-mapping: provide a better default ->get_required_mask")
> > Reported-by: Arvind Sankar <[email protected]>
> > Tested-by: Arvind Sankar <[email protected]>
> > Originally-by: Christoph Hellwig <[email protected]>
> > Signed-off-by: Christoph Hellwig <[email protected]>
> > Fixed-by: Arvind Sankar <[email protected]>
> > Signed-off-by: Arvind Sankar <[email protected]>
>
> This patch looks good to me.
>
> Reviewed-by: Lu Baolu <[email protected]>
>
Hi Christoph, will you be taking this through your dma-mapping branch?
Thanks.
On Wed, Oct 16, 2019 at 03:15:52PM -0400, Arvind Sankar wrote:
> > > Reported-by: Arvind Sankar <[email protected]>
> > > Tested-by: Arvind Sankar <[email protected]>
> > > Originally-by: Christoph Hellwig <[email protected]>
> > > Signed-off-by: Christoph Hellwig <[email protected]>
> > > Fixed-by: Arvind Sankar <[email protected]>
> > > Signed-off-by: Arvind Sankar <[email protected]>
> >
> > This patch looks good to me.
> >
> > Reviewed-by: Lu Baolu <[email protected]>
> >
>
> Hi Christoph, will you be taking this through your dma-mapping branch?
Given this is a patch to intel-iommu I expect Joerg to pick it up.
But if he is fine with that I can also queue it up instead.
On Thu, Oct 17, 2019 at 09:08:47AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 03:15:52PM -0400, Arvind Sankar wrote:
> > > > Reported-by: Arvind Sankar <[email protected]>
> > > > Tested-by: Arvind Sankar <[email protected]>
> > > > Originally-by: Christoph Hellwig <[email protected]>
> > > > Signed-off-by: Christoph Hellwig <[email protected]>
> > > > Fixed-by: Arvind Sankar <[email protected]>
> > > > Signed-off-by: Arvind Sankar <[email protected]>
> > >
> > > This patch looks good to me.
> > >
> > > Reviewed-by: Lu Baolu <[email protected]>
> > >
> >
> > Hi Christoph, will you be taking this through your dma-mapping branch?
>
> Given this is a patch to intel-iommu I expect Joerg to pick it up.
> But if he is fine with that I can also queue it up instead.
David Woodhouse is listed as maintainer for intel-iommu. Cc'ing him as
well.
On Thu, Oct 17, 2019 at 09:08:47AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 03:15:52PM -0400, Arvind Sankar wrote:
> > > > Reported-by: Arvind Sankar <[email protected]>
> > > > Tested-by: Arvind Sankar <[email protected]>
> > > > Originally-by: Christoph Hellwig <[email protected]>
> > > > Signed-off-by: Christoph Hellwig <[email protected]>
> > > > Fixed-by: Arvind Sankar <[email protected]>
> > > > Signed-off-by: Arvind Sankar <[email protected]>
> > >
> > > This patch looks good to me.
> > >
> > > Reviewed-by: Lu Baolu <[email protected]>
> > >
> >
> > Hi Christoph, will you be taking this through your dma-mapping branch?
>
> Given this is a patch to intel-iommu I expect Joerg to pick it up.
> But if he is fine with that I can also queue it up instead.
Fine with me.
Acked-by: Joerg Roedel <[email protected]>
On Fri, Oct 18, 2019 at 05:14:53PM +0200, Christoph Hellwig wrote:
> On Fri, Oct 18, 2019 at 11:50:37AM +0200, Joerg Roedel wrote:
> > On Thu, Oct 17, 2019 at 09:08:47AM +0200, Christoph Hellwig wrote:
> > > On Wed, Oct 16, 2019 at 03:15:52PM -0400, Arvind Sankar wrote:
> > > > > > Reported-by: Arvind Sankar <[email protected]>
> > > > > > Tested-by: Arvind Sankar <[email protected]>
> > > > > > Originally-by: Christoph Hellwig <[email protected]>
> > > > > > Signed-off-by: Christoph Hellwig <[email protected]>
> > > > > > Fixed-by: Arvind Sankar <[email protected]>
> > > > > > Signed-off-by: Arvind Sankar <[email protected]>
> > > > >
> > > > > This patch looks good to me.
> > > > >
> > > > > Reviewed-by: Lu Baolu <[email protected]>
> > > > >
> > > >
> > > > Hi Christoph, will you be taking this through your dma-mapping branch?
> > >
> > > Given this is a patch to intel-iommu I expect Joerg to pick it up.
> > > But if he is fine with that I can also queue it up instead.
> >
> > Fine with me.
> >
> > Acked-by: Joerg Roedel <[email protected]>
>
> That means you want me to queue it up?
Yes, feel free to take it into your tree.
On Fri, Oct 18, 2019 at 05:21:49PM +0200, Joerg Roedel wrote:
> > > Fine with me.
> > >
> > > Acked-by: Joerg Roedel <[email protected]>
> >
> > That means you want me to queue it up?
>
> Yes, feel free to take it into your tree.
Done.
On Fri, Oct 18, 2019 at 11:50:37AM +0200, Joerg Roedel wrote:
> On Thu, Oct 17, 2019 at 09:08:47AM +0200, Christoph Hellwig wrote:
> > On Wed, Oct 16, 2019 at 03:15:52PM -0400, Arvind Sankar wrote:
> > > > > Reported-by: Arvind Sankar <[email protected]>
> > > > > Tested-by: Arvind Sankar <[email protected]>
> > > > > Originally-by: Christoph Hellwig <[email protected]>
> > > > > Signed-off-by: Christoph Hellwig <[email protected]>
> > > > > Fixed-by: Arvind Sankar <[email protected]>
> > > > > Signed-off-by: Arvind Sankar <[email protected]>
> > > >
> > > > This patch looks good to me.
> > > >
> > > > Reviewed-by: Lu Baolu <[email protected]>
> > > >
> > >
> > > Hi Christoph, will you be taking this through your dma-mapping branch?
> >
> > Given this is a patch to intel-iommu I expect Joerg to pick it up.
> > But if he is fine with that I can also queue it up instead.
>
> Fine with me.
>
> Acked-by: Joerg Roedel <[email protected]>
That means you want me to queue it up?