Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Mon, Aug 6, 2018 at 2:08 PM Christoph Hellwig wrote: > > On Mon, Aug 06, 2018 at 12:01:34PM +0200, Arnd Bergmann wrote: > > There are a few subtle corner cases here, in particular in which cases > > the new dma_set_mask() behavior on arm64 reports success or > > failure when truncating the mask to the bus_dma_mask. > > Going forward my plan was to make dma_set_mask() never fail. The idea > is that it sets the mask that the device is capable of, and the core > dma code is responsible for also looking at bus_dma_mask and otherwise > make things just work. > > Robin brought up the case where a platform can't handle a given limitation > ever, e.g. a PCI(e) device with a 24-bit dma mask on a device with a dma > offset that means we'll never have any physical memory reachable in that > range. So we'll either still need to allow it to fail for such corner > cases or delay such error until later, e.g. when dma_alloc_* (or in the > corner case of the corner case dma_map_*) is called. I'm still undecided > which way to go, but not allowing error returns from dma_set_mask and > its variants sounds very tempting. Another case that I think can happen in practice are devices that need a DMA mask smaller than 32-bit wide (either asked for by the driver or according to bus limitations) on a platform that has no ZONE_DMA. However, IIRC there was no reason to ever fail a dma_set_mask() to something wider than 32 bit even if the resulting mask still stays at the default 32-bit mask or something inbetween. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Mon, Aug 06, 2018 at 12:01:34PM +0200, Arnd Bergmann wrote: > There are a few subtle corner cases here, in particular in which cases > the new dma_set_mask() behavior on arm64 reports success or > failure when truncating the mask to the bus_dma_mask. Going forward my plan was to make dma_set_mask() never fail. The idea is that it sets the mask that the device is capable of, and the core dma code is responsible for also looking at bus_dma_mask and otherwise make things just work. Robin brought up the case where a platform can't handle a given limitation ever, e.g. a PCI(e) device with a 24-bit dma mask on a device with a dma offset that means we'll never have any physical memory reachable in that range. So we'll either still need to allow it to fail for such corner cases or delay such error until later, e.g. when dma_alloc_* (or in the corner case of the corner case dma_map_*) is called. I'm still undecided which way to go, but not allowing error returns from dma_set_mask and its variants sounds very tempting. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Tue, Jul 31, 2018 at 1:30 PM Robin Murphy wrote: > On 29/07/18 13:32, Arnd Bergmann wrote: > > On Tue, Jul 24, 2018 at 12:16 AM, Robin Murphy wrote: > > Thanks a lot for working on this, this has bugged me for many years, > > and I've discussed possible solutions with lots of people over time. > > > > I /think/ all your patches are good, but I'm currently travelling and don't > > have a chance to review the resulting overall implementation. > > Could you summarize what happens in the following corner cases of > > DT dma-ranges after your changes (with a driver not setting a mask, > > setting a 64-bit mask and setting a 24-bit mask, respectively)? > > > > a) a device with no dma-ranges property anywhere in its parents > > b) a device with with a 64-bit dma-ranges translation in its parent > > but none in its grandparent > > c) a device with no dma-ranges in its parent but a 64-bit mask > > in its grandparent > > d) a device with a 24-bit mask in its parent. > > In terms of the actual dma-ranges parsing, nothing should be changed by > these patches, so the weirdness and inconsistency that I'm pretty sure > exists for some of those cases will still be there for the moment - I'm > starting on actually fixing of_dma_get_range() now. Right, but I'm interested in the combination of of_dma_get_range() with dma_set_mask(), which is the part that was traditionally broken on arm64 and should be fixed now. There are a few subtle corner cases here, in particular in which cases the new dma_set_mask() behavior on arm64 reports success or failure when truncating the mask to the bus_dma_mask. > The effect after these patches is that a device with a "valid" (per the > current of_dma_get_range() implementation) dma-ranges translation gets > it bus_dma_mask set to cover the given range, whereas a device with no > valid dma-ranges effectively gets a 32-bit bus_dma_mask. That's slightly > different from the ACPI default behaviour, due to subtle spec > differences, but I think it's in line with what you've proposed before > for DT, and it's certainly still flexible if anyone has a different > view. The bus_dma_mask in itself should also be low-impact, since it > will only currently be enforced in the generic dma-direct and iommu-dma > paths, so the likes of powerpc shouldn't see any change at all just yet. Ok. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
Hi Arnd,
On 29/07/18 13:32, Arnd Bergmann wrote:
On Tue, Jul 24, 2018 at 12:16 AM, Robin Murphy wrote:
Whilst the common firmware code invoked by dma_configure() initialises
devices' DMA masks according to limitations described by the respective
properties ("dma-ranges" for OF and _DMA/IORT for ACPI), the nature of
the dma_set_mask() API leads to that information getting lost when
well-behaved drivers probe and set a 64-bit mask, since in general
there's no way to tell the difference between a firmware-described mask
(which should be respected) and whatever default may have come from the
bus code (which should be replaced outright). This can break DMA on
systems with certain IOMMU topologies (e.g. [1]) where the IOMMU driver
only knows its maximum supported address size, not how many of those
address bits might actually be wired up between any of its input
interfaces and the associated DMA master devices. Similarly, some PCIe
root complexes only have a 32-bit native interface on their host bridge,
which leads to the same DMA-address-truncation problem in systems with a
larger physical memory map and RAM above 4GB (e.g. [2]).
These patches attempt to deal with this in the simplest way possible by
generalising the specific quirk for 32-bit bridges into an arbitrary
mask which can then also be plumbed into the firmware code. In the
interest of being minimally invasive, I've only included a point fix
for the IOMMU issue as seen on arm64 - there may be further tweaks
needed in DMA ops (e.g. in arch/arm/ and other OF users) to catch all
possible incarnations of this problem, but at least any that I'm not
fixing here have always been broken. It is also noteworthy that
of_dma_get_range() has never worked properly for the way PCI host
bridges are passed into of_dma_configure() - I'll be working on
further patches to sort that out once this part is done.
Thanks a lot for working on this, this has bugged me for many years,
and I've discussed possible solutions with lots of people over time.
I /think/ all your patches are good, but I'm currently travelling and don't
have a chance to review the resulting overall implementation.
Could you summarize what happens in the following corner cases of
DT dma-ranges after your changes (with a driver not setting a mask,
setting a 64-bit mask and setting a 24-bit mask, respectively)?
a) a device with no dma-ranges property anywhere in its parents
b) a device with with a 64-bit dma-ranges translation in its parent
but none in its grandparent
c) a device with no dma-ranges in its parent but a 64-bit mask
in its grandparent
d) a device with a 24-bit mask in its parent.
In terms of the actual dma-ranges parsing, nothing should be changed by
these patches, so the weirdness and inconsistency that I'm pretty sure
exists for some of those cases will still be there for the moment - I'm
starting on actually fixing of_dma_get_range() now.
The effect after these patches is that a device with a "valid" (per the
current of_dma_get_range() implementation) dma-ranges translation gets
it bus_dma_mask set to cover the given range, whereas a device with no
valid dma-ranges effectively gets a 32-bit bus_dma_mask. That's slightly
different from the ACPI default behaviour, due to subtle spec
differences, but I think it's in line with what you've proposed before
for DT, and it's certainly still flexible if anyone has a different
view. The bus_dma_mask in itself should also be low-impact, since it
will only currently be enforced in the generic dma-direct and iommu-dma
paths, so the likes of powerpc shouldn't see any change at all just yet.
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Tue, Jul 24, 2018 at 12:16 AM, Robin Murphy wrote:
> Whilst the common firmware code invoked by dma_configure() initialises
> devices' DMA masks according to limitations described by the respective
> properties ("dma-ranges" for OF and _DMA/IORT for ACPI), the nature of
> the dma_set_mask() API leads to that information getting lost when
> well-behaved drivers probe and set a 64-bit mask, since in general
> there's no way to tell the difference between a firmware-described mask
> (which should be respected) and whatever default may have come from the
> bus code (which should be replaced outright). This can break DMA on
> systems with certain IOMMU topologies (e.g. [1]) where the IOMMU driver
> only knows its maximum supported address size, not how many of those
> address bits might actually be wired up between any of its input
> interfaces and the associated DMA master devices. Similarly, some PCIe
> root complexes only have a 32-bit native interface on their host bridge,
> which leads to the same DMA-address-truncation problem in systems with a
> larger physical memory map and RAM above 4GB (e.g. [2]).
>
> These patches attempt to deal with this in the simplest way possible by
> generalising the specific quirk for 32-bit bridges into an arbitrary
> mask which can then also be plumbed into the firmware code. In the
> interest of being minimally invasive, I've only included a point fix
> for the IOMMU issue as seen on arm64 - there may be further tweaks
> needed in DMA ops (e.g. in arch/arm/ and other OF users) to catch all
> possible incarnations of this problem, but at least any that I'm not
> fixing here have always been broken. It is also noteworthy that
> of_dma_get_range() has never worked properly for the way PCI host
> bridges are passed into of_dma_configure() - I'll be working on
> further patches to sort that out once this part is done.
Thanks a lot for working on this, this has bugged me for many years,
and I've discussed possible solutions with lots of people over time.
I /think/ all your patches are good, but I'm currently travelling and don't
have a chance to review the resulting overall implementation.
Could you summarize what happens in the following corner cases of
DT dma-ranges after your changes (with a driver not setting a mask,
setting a 64-bit mask and setting a 24-bit mask, respectively)?
a) a device with no dma-ranges property anywhere in its parents
b) a device with with a 64-bit dma-ranges translation in its parent
but none in its grandparent
c) a device with no dma-ranges in its parent but a 64-bit mask
in its grandparent
d) a device with a 24-bit mask in its parent.
Thanks,
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On 07/27/2018 05:55 AM, Robin Murphy wrote: > Hi Grygorii, > > Thanks for the report. > > On 27/07/18 00:45, Grygorii Strashko wrote: > [...] >> With this series applied I can't boot TI ARM32 am574x-idk any more. >> >> And log output is full of "DMA mask not set" - > > That's somewhat expected - as the relevant commit message mentions, > there will be bugs flushed out by this change, but the point is to fix > them :) > >> nobody sets dma_mask for platform bus in case of OF boot :( > > Right, after a brief investigation, that one turns out to be nice and > straightforward to explain and fix; I'll write up the patch shortly. > > [...] >> [ 3.793493] ti-qspi 4b30.qspi: dma_alloc_coherent failed, using >> PIO mode >> [ 3.801088] Unable to handle kernel NULL pointer dereference at >> virtual address 0080 > [...] >> [ 4.602777] [] (omap_dma_write) from [] >> (omap_dma_start_desc+0x78/0x150) >> [ 4.611258] [] (omap_dma_start_desc) from [] >> (omap_dma_issue_pending+0x90/0x98) >> [ 4.620347] r9:ed002780 r8: r7:0010 r6:ee2bdd00 >> r5:ee2bdd4c r4:ee2bdd00 >> [ 4.628130] [] (omap_dma_issue_pending) from [] >> (ti_qspi_dma_xfer+0x8c/0x13c) >> [ 4.637039] r5: r4:ee7e1b28 >> [ 4.640636] [] (ti_qspi_dma_xfer) from [] >> (ti_qspi_exec_mem_op+0x2e4/0x32c) > > Hooray, a real bug! Since dma_alloc_coherent() could have failed due to > lack of memory or any other reason, that's not even directly related to > this change, we've simply helped uncover it. I guess that PIO fallback > path hasn't been tested recently, because it clearly doesn't work. It > would be good if someone who knows the ti-qspi driver and has the means > to test it could figure that one out before also adding the missing > dma_set_coherent_mask() call. yes. ti-qspi has an issues with PIO mode. But, reason of failure is this warning: [3.482502] WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 ti_qspi_probe+0x4a4/0x50c which is caused by this series - which makes coherent_dma_mask constantly 0 by default, so dma_alloc_coherent() - fails. > Robin. -- regards, -grygorii ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
Hi Grygorii, Thanks for the report. On 27/07/18 00:45, Grygorii Strashko wrote: [...] With this series applied I can't boot TI ARM32 am574x-idk any more. And log output is full of "DMA mask not set" - That's somewhat expected - as the relevant commit message mentions, there will be bugs flushed out by this change, but the point is to fix them :) nobody sets dma_mask for platform bus in case of OF boot :( Right, after a brief investigation, that one turns out to be nice and straightforward to explain and fix; I'll write up the patch shortly. [...] [3.793493] ti-qspi 4b30.qspi: dma_alloc_coherent failed, using PIO mode [3.801088] Unable to handle kernel NULL pointer dereference at virtual address 0080 [...] [4.602777] [] (omap_dma_write) from [] (omap_dma_start_desc+0x78/0x150) [4.611258] [] (omap_dma_start_desc) from [] (omap_dma_issue_pending+0x90/0x98) [4.620347] r9:ed002780 r8: r7:0010 r6:ee2bdd00 r5:ee2bdd4c r4:ee2bdd00 [4.628130] [] (omap_dma_issue_pending) from [] (ti_qspi_dma_xfer+0x8c/0x13c) [4.637039] r5: r4:ee7e1b28 [4.640636] [] (ti_qspi_dma_xfer) from [] (ti_qspi_exec_mem_op+0x2e4/0x32c) Hooray, a real bug! Since dma_alloc_coherent() could have failed due to lack of memory or any other reason, that's not even directly related to this change, we've simply helped uncover it. I guess that PIO fallback path hasn't been tested recently, because it clearly doesn't work. It would be good if someone who knows the ti-qspi driver and has the means to test it could figure that one out before also adding the missing dma_set_coherent_mask() call. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On 07/23/2018 05:16 PM, Robin Murphy wrote:
> Whilst the common firmware code invoked by dma_configure() initialises
> devices' DMA masks according to limitations described by the respective
> properties ("dma-ranges" for OF and _DMA/IORT for ACPI), the nature of
> the dma_set_mask() API leads to that information getting lost when
> well-behaved drivers probe and set a 64-bit mask, since in general
> there's no way to tell the difference between a firmware-described mask
> (which should be respected) and whatever default may have come from the
> bus code (which should be replaced outright). This can break DMA on
> systems with certain IOMMU topologies (e.g. [1]) where the IOMMU driver
> only knows its maximum supported address size, not how many of those
> address bits might actually be wired up between any of its input
> interfaces and the associated DMA master devices. Similarly, some PCIe
> root complexes only have a 32-bit native interface on their host bridge,
> which leads to the same DMA-address-truncation problem in systems with a
> larger physical memory map and RAM above 4GB (e.g. [2]).
>
> These patches attempt to deal with this in the simplest way possible by
> generalising the specific quirk for 32-bit bridges into an arbitrary
> mask which can then also be plumbed into the firmware code. In the
> interest of being minimally invasive, I've only included a point fix
> for the IOMMU issue as seen on arm64 - there may be further tweaks
> needed in DMA ops (e.g. in arch/arm/ and other OF users) to catch all
> possible incarnations of this problem, but at least any that I'm not
> fixing here have always been broken. It is also noteworthy that
> of_dma_get_range() has never worked properly for the way PCI host
> bridges are passed into of_dma_configure() - I'll be working on
> further patches to sort that out once this part is done.
>
> Changes since v1 (RFC):
> - Pull in patch #1 (previously sent separately) to avoid conflicts
> - Fix up comment and silly build-breaking typo in patch #2
> - Add patches #6 and #7 since fiddling with coherent masks no longer
>serves a reasonable purpose
>
> Robin.
>
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/580804.html
> [2]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474443.html
>
> Robin Murphy (7):
>ACPI/IORT: Support address size limit for root complexes
>dma-mapping: Generalise dma_32bit_limit flag
>ACPI/IORT: Set bus DMA mask as appropriate
>of/device: Set bus DMA mask as appropriate
>iommu/dma: Respect bus DMA limit for IOVAs
>ACPI/IORT: Don't set default coherent DMA mask
>OF: Don't set default coherent DMA mask
>
> arch/x86/kernel/pci-dma.c | 2 +-
> drivers/acpi/arm64/iort.c | 50 ---
> drivers/iommu/dma-iommu.c | 3 +++
> drivers/of/device.c | 21
> include/linux/device.h| 6 ++---
> kernel/dma/direct.c | 6 ++---
> 6 files changed, 57 insertions(+), 31 deletions(-)
>
With this series applied I can't boot TI ARM32 am574x-idk any more.
And log output is full of "DMA mask not set" -
nobody sets dma_mask for platform bus in case of OF boot :(
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.18.0-rc6-00159-g0509411
(a0226610local@uda0226610) (gcc version 6.2.1 20161016 (Linaro GCC
6.2-2016.11)) #33 SMP PREEMPT Thu Jul 26 18:27:16 CDT 2018
[0.00] CPU: ARMv7 Processor [412fc0f2] revision 2 (ARMv7), cr=10c5387d
[0.00] CPU: div instructions available: patching division code
[0.00] CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
[0.00] OF: fdt: Machine model: TI AM5748 IDK
[0.00] Memory policy: Data cache writealloc
[0.00] efi: Getting EFI parameters from FDT:
[0.00] efi: UEFI not found.
[0.00] OMAP4: Map 0xffd0 to (ptrval) for dram barrier
[0.00] DRA762 ES1.0
[0.00] random: get_random_bytes called from start_kernel+0xa4/0x44c
with crng_init=0
[0.00] percpu: Embedded 16 pages/cpu @(ptrval) s36300 r8192 d21044
u65536
[0.00] Built 1 zonelists, mobility grouping on. Total pages: 522047
[0.00] Kernel command line: console=ttyO2,115200n8
root=PARTUUID=00028e44-02 rw rootfstype=ext4 rootwait
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 2062464K/2095100K available (8192K kernel code, 389K
rwdata, 3008K rodata, 1024K init, 280K bss, 32636K reserved, 0K cma-reserved,
1308668K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector : 0x - 0x1000 ( 4 kB)
[0.00] fixmap : 0xffc0 - 0xfff0 (3072 kB)
[0.00] vmalloc : 0xf080 - 0xff80 ( 240 MB)
[0.00] lowmem : 0xc000 - 0xf000 ( 768 MB)
[0.00]
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
Thanks, applied to the dma-mapping tree. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On 25 July 2018 at 13:31, Christoph Hellwig wrote: > Hi Robin, > > this series looks fine to me. Do you want me to pull this into the > dma-mapping tree? I'd like to see a few more ACKs from the ACPI/OF/ > IOMMU maintainers though. > This fixes the issue I reported with the on-SoC NIC of the Socionext SynQuacer, so assuming it works as advertised: Acked-by: Ard Biesheuvel for the series. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Wed, Jul 25, 2018 at 01:12:56PM +0100, Robin Murphy wrote: > On 25/07/18 12:31, Christoph Hellwig wrote: > >Hi Robin, > > > >this series looks fine to me. Do you want me to pull this into the > >dma-mapping tree? I'd like to see a few more ACKs from the ACPI/OF/ > >IOMMU maintainers though. > > Thanks, I was indeed assuming that the dma-mapping tree would be the best > route to keep this lot together, hence including patch #1 even though it's > not functionally related (having spoken to Will offline I think he's happy > for that one to go through you rather than via arm64 this time around, even > if #2-#7 don't make it). Yes, that's right. Here's my ack if it helps: Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On 25/07/18 12:31, Christoph Hellwig wrote: Hi Robin, this series looks fine to me. Do you want me to pull this into the dma-mapping tree? I'd like to see a few more ACKs from the ACPI/OF/ IOMMU maintainers though. Thanks, I was indeed assuming that the dma-mapping tree would be the best route to keep this lot together, hence including patch #1 even though it's not functionally related (having spoken to Will offline I think he's happy for that one to go through you rather than via arm64 this time around, even if #2-#7 don't make it). Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Wed, Jul 25, 2018 at 01:31:22PM +0200, Christoph Hellwig wrote: > Hi Robin, > > this series looks fine to me. Do you want me to pull this into the > dma-mapping tree? I'd like to see a few more ACKs from the ACPI/OF/ > IOMMU maintainers though. For the IOMMU parts: Acked-by: Joerg Roedel ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
Hi Robin, this series looks fine to me. Do you want me to pull this into the dma-mapping tree? I'd like to see a few more ACKs from the ACPI/OF/ IOMMU maintainers though. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
