* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-09 7:30 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-09 7:30 UTC (permalink / raw) To: Arnd Bergmann Cc: linux-arm-kernel, Will Deacon, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, Nikita Yushchenko It is possible that device is capable of 64-bit DMA addresses, and device driver tries to set wide DMA mask, but bridge or bus used to connect device to the system can't handle wide addresses. With swiotlb, memory above 4G still can be used by drivers for streaming DMA, but *dev->mask and dev->dma_coherent_mask must still keep values that hardware handles physically. This patch enforces that. Based on original version by Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> CC: Arnd Bergmann <arnd@arndb.de> --- Changes since v1: - fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - save mask, not size - remove doube empty line arch/arm64/Kconfig | 3 +++ arch/arm64/include/asm/device.h | 1 + arch/arm64/mm/dma-mapping.c | 51 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..afb2c08 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE config NEED_SG_DMA_LENGTH def_bool y +config ARCH_HAS_DMA_SET_COHERENT_MASK + def_bool y + config SMP def_bool y diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h index 243ef25..a57e7bb 100644 --- a/arch/arm64/include/asm/device.h +++ b/arch/arm64/include/asm/device.h @@ -22,6 +22,7 @@ struct dev_archdata { void *iommu; /* private IOMMU data */ #endif bool dma_coherent; + u64 parent_dma_mask; }; struct pdev_archdata { diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index e040827..5ab15ce 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -352,6 +352,30 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) return 1; } +static int __swiotlb_set_dma_mask(struct device *dev, u64 mask) +{ + /* device is not DMA capable */ + if (!dev->dma_mask) + return -EIO; + + /* mask is below swiotlb bounce buffer, so fail */ + if (!swiotlb_dma_supported(dev, mask)) + return -EIO; + + /* + * because of the swiotlb, we can return success for + * larger masks, but need to ensure that bounce buffers + * are used above parent_dma_mask, so set that as + * the effective mask. + */ + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + *dev->dma_mask = mask; + + return 0; +} + static struct dma_map_ops swiotlb_dma_ops = { .alloc = __dma_alloc, .free = __dma_free, @@ -367,8 +391,23 @@ static struct dma_map_ops swiotlb_dma_ops = { .sync_sg_for_device = __swiotlb_sync_sg_for_device, .dma_supported = __swiotlb_dma_supported, .mapping_error = swiotlb_dma_mapping_error, + .set_dma_mask = __swiotlb_set_dma_mask, }; +int dma_set_coherent_mask(struct device *dev, u64 mask) +{ + if (!dma_supported(dev, mask)) + return -EIO; + + if (get_dma_ops(dev) == &swiotlb_dma_ops && + mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + dev->coherent_dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_coherent_mask); + static int __init atomic_pool_init(void) { pgprot_t prot = __pgprot(PROT_NORMAL_NC); @@ -958,6 +997,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; + /* + * we don't yet support buses that have a non-zero mapping. + * Let's hope we won't need it + */ + WARN_ON(dma_base != 0); + + /* + * Whatever the parent bus can set. A device must not set + * a DMA mask larger than this. + */ + dev->archdata.parent_dma_mask = size - 1; + dev->archdata.dma_coherent = coherent; __iommu_setup_dma_ops(dev, dma_base, size, iommu); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-09 7:30 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-09 7:30 UTC (permalink / raw) To: linux-arm-kernel It is possible that device is capable of 64-bit DMA addresses, and device driver tries to set wide DMA mask, but bridge or bus used to connect device to the system can't handle wide addresses. With swiotlb, memory above 4G still can be used by drivers for streaming DMA, but *dev->mask and dev->dma_coherent_mask must still keep values that hardware handles physically. This patch enforces that. Based on original version by Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> CC: Arnd Bergmann <arnd@arndb.de> --- Changes since v1: - fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - save mask, not size - remove doube empty line arch/arm64/Kconfig | 3 +++ arch/arm64/include/asm/device.h | 1 + arch/arm64/mm/dma-mapping.c | 51 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..afb2c08 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE config NEED_SG_DMA_LENGTH def_bool y +config ARCH_HAS_DMA_SET_COHERENT_MASK + def_bool y + config SMP def_bool y diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h index 243ef25..a57e7bb 100644 --- a/arch/arm64/include/asm/device.h +++ b/arch/arm64/include/asm/device.h @@ -22,6 +22,7 @@ struct dev_archdata { void *iommu; /* private IOMMU data */ #endif bool dma_coherent; + u64 parent_dma_mask; }; struct pdev_archdata { diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index e040827..5ab15ce 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -352,6 +352,30 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) return 1; } +static int __swiotlb_set_dma_mask(struct device *dev, u64 mask) +{ + /* device is not DMA capable */ + if (!dev->dma_mask) + return -EIO; + + /* mask is below swiotlb bounce buffer, so fail */ + if (!swiotlb_dma_supported(dev, mask)) + return -EIO; + + /* + * because of the swiotlb, we can return success for + * larger masks, but need to ensure that bounce buffers + * are used above parent_dma_mask, so set that as + * the effective mask. + */ + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + *dev->dma_mask = mask; + + return 0; +} + static struct dma_map_ops swiotlb_dma_ops = { .alloc = __dma_alloc, .free = __dma_free, @@ -367,8 +391,23 @@ static struct dma_map_ops swiotlb_dma_ops = { .sync_sg_for_device = __swiotlb_sync_sg_for_device, .dma_supported = __swiotlb_dma_supported, .mapping_error = swiotlb_dma_mapping_error, + .set_dma_mask = __swiotlb_set_dma_mask, }; +int dma_set_coherent_mask(struct device *dev, u64 mask) +{ + if (!dma_supported(dev, mask)) + return -EIO; + + if (get_dma_ops(dev) == &swiotlb_dma_ops && + mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + dev->coherent_dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_coherent_mask); + static int __init atomic_pool_init(void) { pgprot_t prot = __pgprot(PROT_NORMAL_NC); @@ -958,6 +997,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; + /* + * we don't yet support buses that have a non-zero mapping. + * Let's hope we won't need it + */ + WARN_ON(dma_base != 0); + + /* + * Whatever the parent bus can set. A device must not set + * a DMA mask larger than this. + */ + dev->archdata.parent_dma_mask = size - 1; + dev->archdata.dma_coherent = coherent; __iommu_setup_dma_ops(dev, dma_base, size, iommu); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-09 7:30 ` Nikita Yushchenko @ 2017-01-10 11:51 ` Will Deacon -1 siblings, 0 replies; 75+ messages in thread From: Will Deacon @ 2017-01-10 11:51 UTC (permalink / raw) To: Nikita Yushchenko Cc: Arnd Bergmann, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, robin.murphy, fkan On Mon, Jan 09, 2017 at 10:30:02AM +0300, Nikita Yushchenko wrote: > It is possible that device is capable of 64-bit DMA addresses, and > device driver tries to set wide DMA mask, but bridge or bus used to > connect device to the system can't handle wide addresses. > > With swiotlb, memory above 4G still can be used by drivers for streaming > DMA, but *dev->mask and dev->dma_coherent_mask must still keep values > that hardware handles physically. > > This patch enforces that. Based on original version by > Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > CC: Arnd Bergmann <arnd@arndb.de> > --- > Changes since v1: > - fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > - save mask, not size > - remove doube empty line > > arch/arm64/Kconfig | 3 +++ > arch/arm64/include/asm/device.h | 1 + > arch/arm64/mm/dma-mapping.c | 51 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 55 insertions(+) I still don't think this patch is general enough. The problem you're seeing with swiotlb seems to be exactly the same problem reported by Feng Kan over at: http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com [read on; it was initially thought to be a hardware erratum, but it's actually the inability to restrict the DMA mask of the endpoint that's the problem] The point here is that an IOMMU doesn't solve your issue, and the IOMMU-backed DMA ops need the same treatment. In light of that, it really feels to me like the DMA masks should be restricted in of_dma_configure so that the parent mask is taken into account there, rather than hook into each set of DMA ops to intercept set_dma_mask. We'd still need to do something to stop dma_set_mask widening the mask if it was restricted by of_dma_configure, but I think Robin (cc'd) was playing with that. Will ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 11:51 ` Will Deacon 0 siblings, 0 replies; 75+ messages in thread From: Will Deacon @ 2017-01-10 11:51 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 09, 2017 at 10:30:02AM +0300, Nikita Yushchenko wrote: > It is possible that device is capable of 64-bit DMA addresses, and > device driver tries to set wide DMA mask, but bridge or bus used to > connect device to the system can't handle wide addresses. > > With swiotlb, memory above 4G still can be used by drivers for streaming > DMA, but *dev->mask and dev->dma_coherent_mask must still keep values > that hardware handles physically. > > This patch enforces that. Based on original version by > Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > CC: Arnd Bergmann <arnd@arndb.de> > --- > Changes since v1: > - fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > - save mask, not size > - remove doube empty line > > arch/arm64/Kconfig | 3 +++ > arch/arm64/include/asm/device.h | 1 + > arch/arm64/mm/dma-mapping.c | 51 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 55 insertions(+) I still don't think this patch is general enough. The problem you're seeing with swiotlb seems to be exactly the same problem reported by Feng Kan over at: http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g at mail.gmail.com [read on; it was initially thought to be a hardware erratum, but it's actually the inability to restrict the DMA mask of the endpoint that's the problem] The point here is that an IOMMU doesn't solve your issue, and the IOMMU-backed DMA ops need the same treatment. In light of that, it really feels to me like the DMA masks should be restricted in of_dma_configure so that the parent mask is taken into account there, rather than hook into each set of DMA ops to intercept set_dma_mask. We'd still need to do something to stop dma_set_mask widening the mask if it was restricted by of_dma_configure, but I think Robin (cc'd) was playing with that. Will ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 11:51 ` Will Deacon @ 2017-01-10 12:47 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-10 12:47 UTC (permalink / raw) To: Will Deacon Cc: Arnd Bergmann, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, robin.murphy, fkan, Christoph Hellwig Hi > The point here is that an IOMMU doesn't solve your issue, and the > IOMMU-backed DMA ops need the same treatment. In light of that, it really > feels to me like the DMA masks should be restricted in of_dma_configure > so that the parent mask is taken into account there, rather than hook > into each set of DMA ops to intercept set_dma_mask. We'd still need to > do something to stop dma_set_mask widening the mask if it was restricted > by of_dma_configure, but I think Robin (cc'd) was playing with that. What issue "IOMMU doesn't solve"? Issue I'm trying to address is - inconsistency within swiotlb dma_map_ops, where (1) any wide mask is silently accepted, but (2) then mask is used to decide if bounce buffers are needed or not. This inconsistency causes NVMe+R-Car cobmo not working (and breaking memory instead). I just can't think out what similar issue iommu can have. Do you mean that in iommu case, mask also must not be set to whatever wider than initial value? Why? What is the use of mask in iommu case? Is there any real case when iommu can't address all memory existing in the system? NVMe maintainer has just stated that they expect set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error out driver probe if that call fails. They claim that architecture must always be able to dma_map() whatever memory existing in the system - via iommu or swiotlb or whatever. Their direction is to remove bounce buffers from block and other layers. With this direction, semantics of dma mask becomes even more questionable. I'd say dma_mask is candidate for removal (or to move to swiotlb's or iommu's local area) Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 12:47 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-10 12:47 UTC (permalink / raw) To: linux-arm-kernel Hi > The point here is that an IOMMU doesn't solve your issue, and the > IOMMU-backed DMA ops need the same treatment. In light of that, it really > feels to me like the DMA masks should be restricted in of_dma_configure > so that the parent mask is taken into account there, rather than hook > into each set of DMA ops to intercept set_dma_mask. We'd still need to > do something to stop dma_set_mask widening the mask if it was restricted > by of_dma_configure, but I think Robin (cc'd) was playing with that. What issue "IOMMU doesn't solve"? Issue I'm trying to address is - inconsistency within swiotlb dma_map_ops, where (1) any wide mask is silently accepted, but (2) then mask is used to decide if bounce buffers are needed or not. This inconsistency causes NVMe+R-Car cobmo not working (and breaking memory instead). I just can't think out what similar issue iommu can have. Do you mean that in iommu case, mask also must not be set to whatever wider than initial value? Why? What is the use of mask in iommu case? Is there any real case when iommu can't address all memory existing in the system? NVMe maintainer has just stated that they expect set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error out driver probe if that call fails. They claim that architecture must always be able to dma_map() whatever memory existing in the system - via iommu or swiotlb or whatever. Their direction is to remove bounce buffers from block and other layers. With this direction, semantics of dma mask becomes even more questionable. I'd say dma_mask is candidate for removal (or to move to swiotlb's or iommu's local area) Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 12:47 ` Nikita Yushchenko @ 2017-01-10 13:12 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-10 13:12 UTC (permalink / raw) To: Nikita Yushchenko Cc: Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, robin.murphy, fkan, Christoph Hellwig On Tuesday, January 10, 2017 3:47:25 PM CET Nikita Yushchenko wrote: > > > The point here is that an IOMMU doesn't solve your issue, and the > > IOMMU-backed DMA ops need the same treatment. In light of that, it really > > feels to me like the DMA masks should be restricted in of_dma_configure > > so that the parent mask is taken into account there, rather than hook > > into each set of DMA ops to intercept set_dma_mask. of_dma_configure() sets up a 32-bit mask, which is assumed to always work in the kernel. We can't change it to a larger mask because that would break drivers that have to restrict devices to 32-bit. If the bus addressing is narrower than 32 bits however, the initial mask should probably be limited to whatever the bus supports, but that is not the problem we are trying to solve here. > > We'd still need to > > do something to stop dma_set_mask widening the mask if it was restricted > > by of_dma_configure, but I think Robin (cc'd) was playing with that. > > What issue "IOMMU doesn't solve"? > > Issue I'm trying to address is - inconsistency within swiotlb > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > mask is used to decide if bounce buffers are needed or not. This > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > instead). It's not just an inconsistency, it's a known bug that we really need to fix. > I just can't think out what similar issue iommu can have. > Do you mean that in iommu case, mask also must not be set to whatever > wider than initial value? Why? What is the use of mask in iommu case? Is > there any real case when iommu can't address all memory existing in the > system? I think the problem that Will is referring to is when the IOMMU has a virtual address space that is wider than the DMA mask of the device: In this case, dma_map_single() might return a dma_addr_t that is not reachable by the device. I'd consider that a separate bug that needs to be worked around in the IOMMU code. > NVMe maintainer has just stated that they expect > set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error > out driver probe if that call fails. They claim that architecture must > always be able to dma_map() whatever memory existing in the system - via > iommu or swiotlb or whatever. Their direction is to remove bounce > buffers from block and other layers. > > With this direction, semantics of dma mask becomes even more > questionable. I'd say dma_mask is candidate for removal (or to move to > swiotlb's or iommu's local area) Removing dma_mask is not realistic any time soon, there are too many things in the kernel that use it for one thing or another, so any changes here have to be done really carefully. We definitely need the mask to support architectures without swiotlb. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 13:12 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-10 13:12 UTC (permalink / raw) To: linux-arm-kernel On Tuesday, January 10, 2017 3:47:25 PM CET Nikita Yushchenko wrote: > > > The point here is that an IOMMU doesn't solve your issue, and the > > IOMMU-backed DMA ops need the same treatment. In light of that, it really > > feels to me like the DMA masks should be restricted in of_dma_configure > > so that the parent mask is taken into account there, rather than hook > > into each set of DMA ops to intercept set_dma_mask. of_dma_configure() sets up a 32-bit mask, which is assumed to always work in the kernel. We can't change it to a larger mask because that would break drivers that have to restrict devices to 32-bit. If the bus addressing is narrower than 32 bits however, the initial mask should probably be limited to whatever the bus supports, but that is not the problem we are trying to solve here. > > We'd still need to > > do something to stop dma_set_mask widening the mask if it was restricted > > by of_dma_configure, but I think Robin (cc'd) was playing with that. > > What issue "IOMMU doesn't solve"? > > Issue I'm trying to address is - inconsistency within swiotlb > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > mask is used to decide if bounce buffers are needed or not. This > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > instead). It's not just an inconsistency, it's a known bug that we really need to fix. > I just can't think out what similar issue iommu can have. > Do you mean that in iommu case, mask also must not be set to whatever > wider than initial value? Why? What is the use of mask in iommu case? Is > there any real case when iommu can't address all memory existing in the > system? I think the problem that Will is referring to is when the IOMMU has a virtual address space that is wider than the DMA mask of the device: In this case, dma_map_single() might return a dma_addr_t that is not reachable by the device. I'd consider that a separate bug that needs to be worked around in the IOMMU code. > NVMe maintainer has just stated that they expect > set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error > out driver probe if that call fails. They claim that architecture must > always be able to dma_map() whatever memory existing in the system - via > iommu or swiotlb or whatever. Their direction is to remove bounce > buffers from block and other layers. > > With this direction, semantics of dma mask becomes even more > questionable. I'd say dma_mask is candidate for removal (or to move to > swiotlb's or iommu's local area) Removing dma_mask is not realistic any time soon, there are too many things in the kernel that use it for one thing or another, so any changes here have to be done really carefully. We definitely need the mask to support architectures without swiotlb. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 12:47 ` Nikita Yushchenko @ 2017-01-10 13:25 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-10 13:25 UTC (permalink / raw) To: Nikita Yushchenko, Will Deacon Cc: Arnd Bergmann, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On 10/01/17 12:47, Nikita Yushchenko wrote: > Hi > >> The point here is that an IOMMU doesn't solve your issue, and the >> IOMMU-backed DMA ops need the same treatment. In light of that, it really >> feels to me like the DMA masks should be restricted in of_dma_configure >> so that the parent mask is taken into account there, rather than hook >> into each set of DMA ops to intercept set_dma_mask. We'd still need to >> do something to stop dma_set_mask widening the mask if it was restricted >> by of_dma_configure, but I think Robin (cc'd) was playing with that. > > What issue "IOMMU doesn't solve"? > > Issue I'm trying to address is - inconsistency within swiotlb > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > mask is used to decide if bounce buffers are needed or not. This > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > instead). The fundamental underlying problem is the "any wide mask is silently accepted" part, and that applies equally to IOMMU ops as well. > I just can't think out what similar issue iommu can have. > Do you mean that in iommu case, mask also must not be set to whatever > wider than initial value? Why? What is the use of mask in iommu case? Is > there any real case when iommu can't address all memory existing in the > system? There's a very subtle misunderstanding there - the DMA mask does not describe the memory a device can address, it describes the range of addresses the device is capable of generating. Yes, in the non-IOMMU case they are equivalent, but once you put an IOMMU in between, the problem is merely shifted from "what range of physical addresses can this device access" to "what range of IOVAs is valid to give to this device" - the fact that those IOVAs can map to any underlying physical address only obviates the need for any bouncing at the memory end; it doesn't remove the fact that the device has a hardware addressing limitation which needs to be accommodated. The thread Will linked to describes that equivalent version of your problem - the IOMMU gives the device 48-bit addresses which get erroneously truncated because it doesn't know that only 42 bits are actually wired up. That situation still requires the device's DMA mask to correctly describe its addressing capability just as yours does. > NVMe maintainer has just stated that they expect > set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error > out driver probe if that call fails. They claim that architecture must > always be able to dma_map() whatever memory existing in the system - via > iommu or swiotlb or whatever. Their direction is to remove bounce > buffers from block and other layers. I have to say I'm in full agreement with that - having subsystems do their own bouncing is generally redundant, although there are some edge-cases like MMC_BLOCK_BOUNCE (which is primarily about aligning and coalescing buffers than working around DMA limitations per se). > With this direction, semantics of dma mask becomes even more > questionable. I'd say dma_mask is candidate for removal (or to move to > swiotlb's or iommu's local area) We still need a way for drivers to communicate a device's probed addressing capability to SWIOTLB, so there's always going to have to be *some* sort of public interface. Personally, the change in semantics I'd like to see is to make dma_set_mask() only fail if DMA is entirely disallowed - in the normal case it would always succeed, but the DMA API implementation would be permitted to set a smaller mask than requested (this is effectively what the x86 IOMMU ops do already). The significant work that would require, though, is changing all the drivers currently using this sort of pattern: if (!dma_set_mask(dev, DMA_BIT_MASK(64)) /* put device into 64-bit mode */ else if (!dma_set_mask(dev, DMA_BIT_MASK(32)) /* put device into 32-bit mode */ else /* error */ to something like this: if (!dma_set_mask(dev, DMA_BIT_MASK(64)) /* error */ if (dma_get_mask(dev) > DMA_BIT_MASK(32)) /* put device into 64-bit mode */ else /* put device into 32-bit mode */ Which would be a pretty major job. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 13:25 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-10 13:25 UTC (permalink / raw) To: linux-arm-kernel On 10/01/17 12:47, Nikita Yushchenko wrote: > Hi > >> The point here is that an IOMMU doesn't solve your issue, and the >> IOMMU-backed DMA ops need the same treatment. In light of that, it really >> feels to me like the DMA masks should be restricted in of_dma_configure >> so that the parent mask is taken into account there, rather than hook >> into each set of DMA ops to intercept set_dma_mask. We'd still need to >> do something to stop dma_set_mask widening the mask if it was restricted >> by of_dma_configure, but I think Robin (cc'd) was playing with that. > > What issue "IOMMU doesn't solve"? > > Issue I'm trying to address is - inconsistency within swiotlb > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > mask is used to decide if bounce buffers are needed or not. This > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > instead). The fundamental underlying problem is the "any wide mask is silently accepted" part, and that applies equally to IOMMU ops as well. > I just can't think out what similar issue iommu can have. > Do you mean that in iommu case, mask also must not be set to whatever > wider than initial value? Why? What is the use of mask in iommu case? Is > there any real case when iommu can't address all memory existing in the > system? There's a very subtle misunderstanding there - the DMA mask does not describe the memory a device can address, it describes the range of addresses the device is capable of generating. Yes, in the non-IOMMU case they are equivalent, but once you put an IOMMU in between, the problem is merely shifted from "what range of physical addresses can this device access" to "what range of IOVAs is valid to give to this device" - the fact that those IOVAs can map to any underlying physical address only obviates the need for any bouncing at the memory end; it doesn't remove the fact that the device has a hardware addressing limitation which needs to be accommodated. The thread Will linked to describes that equivalent version of your problem - the IOMMU gives the device 48-bit addresses which get erroneously truncated because it doesn't know that only 42 bits are actually wired up. That situation still requires the device's DMA mask to correctly describe its addressing capability just as yours does. > NVMe maintainer has just stated that they expect > set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error > out driver probe if that call fails. They claim that architecture must > always be able to dma_map() whatever memory existing in the system - via > iommu or swiotlb or whatever. Their direction is to remove bounce > buffers from block and other layers. I have to say I'm in full agreement with that - having subsystems do their own bouncing is generally redundant, although there are some edge-cases like MMC_BLOCK_BOUNCE (which is primarily about aligning and coalescing buffers than working around DMA limitations per se). > With this direction, semantics of dma mask becomes even more > questionable. I'd say dma_mask is candidate for removal (or to move to > swiotlb's or iommu's local area) We still need a way for drivers to communicate a device's probed addressing capability to SWIOTLB, so there's always going to have to be *some* sort of public interface. Personally, the change in semantics I'd like to see is to make dma_set_mask() only fail if DMA is entirely disallowed - in the normal case it would always succeed, but the DMA API implementation would be permitted to set a smaller mask than requested (this is effectively what the x86 IOMMU ops do already). The significant work that would require, though, is changing all the drivers currently using this sort of pattern: if (!dma_set_mask(dev, DMA_BIT_MASK(64)) /* put device into 64-bit mode */ else if (!dma_set_mask(dev, DMA_BIT_MASK(32)) /* put device into 32-bit mode */ else /* error */ to something like this: if (!dma_set_mask(dev, DMA_BIT_MASK(64)) /* error */ if (dma_get_mask(dev) > DMA_BIT_MASK(32)) /* put device into 64-bit mode */ else /* put device into 32-bit mode */ Which would be a pretty major job. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 13:25 ` Robin Murphy @ 2017-01-10 13:42 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-10 13:42 UTC (permalink / raw) To: Robin Murphy Cc: Nikita Yushchenko, Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote: > On 10/01/17 12:47, Nikita Yushchenko wrote: > >> The point here is that an IOMMU doesn't solve your issue, and the > >> IOMMU-backed DMA ops need the same treatment. In light of that, it really > >> feels to me like the DMA masks should be restricted in of_dma_configure > >> so that the parent mask is taken into account there, rather than hook > >> into each set of DMA ops to intercept set_dma_mask. We'd still need to > >> do something to stop dma_set_mask widening the mask if it was restricted > >> by of_dma_configure, but I think Robin (cc'd) was playing with that. > > > > What issue "IOMMU doesn't solve"? > > > > Issue I'm trying to address is - inconsistency within swiotlb > > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > > mask is used to decide if bounce buffers are needed or not. This > > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > > instead). > > The fundamental underlying problem is the "any wide mask is silently > accepted" part, and that applies equally to IOMMU ops as well. It's a much rarer problem for the IOMMU case though, because it only impacts devices that are restricted to addressing of less than 32-bits. If you have an IOMMU enabled, the dma-mapping interface does not care if the device can do wider than 32 bit addressing, as it will never hand out IOVAs above 0xffffffff. > > I just can't think out what similar issue iommu can have. > > Do you mean that in iommu case, mask also must not be set to whatever > > wider than initial value? Why? What is the use of mask in iommu case? Is > > there any real case when iommu can't address all memory existing in the > > system? > > There's a very subtle misunderstanding there - the DMA mask does not > describe the memory a device can address, it describes the range of > addresses the device is capable of generating. Yes, in the non-IOMMU > case they are equivalent, but once you put an IOMMU in between, the > problem is merely shifted from "what range of physical addresses can > this device access" to "what range of IOVAs is valid to give to this > device" - the fact that those IOVAs can map to any underlying physical > address only obviates the need for any bouncing at the memory end; it > doesn't remove the fact that the device has a hardware addressing > limitation which needs to be accommodated. > > The thread Will linked to describes that equivalent version of your > problem - the IOMMU gives the device 48-bit addresses which get > erroneously truncated because it doesn't know that only 42 bits are > actually wired up. That situation still requires the device's DMA mask > to correctly describe its addressing capability just as yours does. That problem should only impact virtual machines which have a guest bus address space covering more than 42 bits of physical RAM, whereas the problem we have with swiotlb is for the dma-mapping interface. > > With this direction, semantics of dma mask becomes even more > > questionable. I'd say dma_mask is candidate for removal (or to move to > > swiotlb's or iommu's local area) > > We still need a way for drivers to communicate a device's probed > addressing capability to SWIOTLB, so there's always going to have to be > *some* sort of public interface. Personally, the change in semantics I'd > like to see is to make dma_set_mask() only fail if DMA is entirely > disallowed - in the normal case it would always succeed, but the DMA API > implementation would be permitted to set a smaller mask than requested > (this is effectively what the x86 IOMMU ops do already). With swiotlb enabled, it only needs to fail if the mask does not contain the swiotlb bounce buffer area, either because the start of RAM is outside of the mask, or the bounce area has been allocated at the end of ZONE_DMA and the mask is smaller than ZONE_DMA. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 13:42 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-10 13:42 UTC (permalink / raw) To: linux-arm-kernel On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote: > On 10/01/17 12:47, Nikita Yushchenko wrote: > >> The point here is that an IOMMU doesn't solve your issue, and the > >> IOMMU-backed DMA ops need the same treatment. In light of that, it really > >> feels to me like the DMA masks should be restricted in of_dma_configure > >> so that the parent mask is taken into account there, rather than hook > >> into each set of DMA ops to intercept set_dma_mask. We'd still need to > >> do something to stop dma_set_mask widening the mask if it was restricted > >> by of_dma_configure, but I think Robin (cc'd) was playing with that. > > > > What issue "IOMMU doesn't solve"? > > > > Issue I'm trying to address is - inconsistency within swiotlb > > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > > mask is used to decide if bounce buffers are needed or not. This > > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > > instead). > > The fundamental underlying problem is the "any wide mask is silently > accepted" part, and that applies equally to IOMMU ops as well. It's a much rarer problem for the IOMMU case though, because it only impacts devices that are restricted to addressing of less than 32-bits. If you have an IOMMU enabled, the dma-mapping interface does not care if the device can do wider than 32 bit addressing, as it will never hand out IOVAs above 0xffffffff. > > I just can't think out what similar issue iommu can have. > > Do you mean that in iommu case, mask also must not be set to whatever > > wider than initial value? Why? What is the use of mask in iommu case? Is > > there any real case when iommu can't address all memory existing in the > > system? > > There's a very subtle misunderstanding there - the DMA mask does not > describe the memory a device can address, it describes the range of > addresses the device is capable of generating. Yes, in the non-IOMMU > case they are equivalent, but once you put an IOMMU in between, the > problem is merely shifted from "what range of physical addresses can > this device access" to "what range of IOVAs is valid to give to this > device" - the fact that those IOVAs can map to any underlying physical > address only obviates the need for any bouncing at the memory end; it > doesn't remove the fact that the device has a hardware addressing > limitation which needs to be accommodated. > > The thread Will linked to describes that equivalent version of your > problem - the IOMMU gives the device 48-bit addresses which get > erroneously truncated because it doesn't know that only 42 bits are > actually wired up. That situation still requires the device's DMA mask > to correctly describe its addressing capability just as yours does. That problem should only impact virtual machines which have a guest bus address space covering more than 42 bits of physical RAM, whereas the problem we have with swiotlb is for the dma-mapping interface. > > With this direction, semantics of dma mask becomes even more > > questionable. I'd say dma_mask is candidate for removal (or to move to > > swiotlb's or iommu's local area) > > We still need a way for drivers to communicate a device's probed > addressing capability to SWIOTLB, so there's always going to have to be > *some* sort of public interface. Personally, the change in semantics I'd > like to see is to make dma_set_mask() only fail if DMA is entirely > disallowed - in the normal case it would always succeed, but the DMA API > implementation would be permitted to set a smaller mask than requested > (this is effectively what the x86 IOMMU ops do already). With swiotlb enabled, it only needs to fail if the mask does not contain the swiotlb bounce buffer area, either because the start of RAM is outside of the mask, or the bounce area has been allocated at the end of ZONE_DMA and the mask is smaller than ZONE_DMA. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 13:42 ` Arnd Bergmann @ 2017-01-10 14:16 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-10 14:16 UTC (permalink / raw) To: Arnd Bergmann Cc: Nikita Yushchenko, Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On 10/01/17 13:42, Arnd Bergmann wrote: > On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote: >> On 10/01/17 12:47, Nikita Yushchenko wrote: >>>> The point here is that an IOMMU doesn't solve your issue, and the >>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really >>>> feels to me like the DMA masks should be restricted in of_dma_configure >>>> so that the parent mask is taken into account there, rather than hook >>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to >>>> do something to stop dma_set_mask widening the mask if it was restricted >>>> by of_dma_configure, but I think Robin (cc'd) was playing with that. >>> >>> What issue "IOMMU doesn't solve"? >>> >>> Issue I'm trying to address is - inconsistency within swiotlb >>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then >>> mask is used to decide if bounce buffers are needed or not. This >>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory >>> instead). >> >> The fundamental underlying problem is the "any wide mask is silently >> accepted" part, and that applies equally to IOMMU ops as well. > > It's a much rarer problem for the IOMMU case though, because it only > impacts devices that are restricted to addressing of less than 32-bits. > > If you have an IOMMU enabled, the dma-mapping interface does not care > if the device can do wider than 32 bit addressing, as it will never > hand out IOVAs above 0xffffffff. I can assure you that it will - we constrain allocations to the intersection of the IOMMU domain aperture (normally the IOMMU's physical input address width) and the given device's DMA mask. If both of those are >32 bits then >32-bit IOVAs will fall out. For the arm64/common implementation I have prototyped a copy of the x86 optimisation which always first tries to get 32-bit IOVAs for PCI devices, but even then it can start returning higher addresses if the 32-bit space fills up. >>> I just can't think out what similar issue iommu can have. >>> Do you mean that in iommu case, mask also must not be set to whatever >>> wider than initial value? Why? What is the use of mask in iommu case? Is >>> there any real case when iommu can't address all memory existing in the >>> system? >> >> There's a very subtle misunderstanding there - the DMA mask does not >> describe the memory a device can address, it describes the range of >> addresses the device is capable of generating. Yes, in the non-IOMMU >> case they are equivalent, but once you put an IOMMU in between, the >> problem is merely shifted from "what range of physical addresses can >> this device access" to "what range of IOVAs is valid to give to this >> device" - the fact that those IOVAs can map to any underlying physical >> address only obviates the need for any bouncing at the memory end; it >> doesn't remove the fact that the device has a hardware addressing >> limitation which needs to be accommodated. >> >> The thread Will linked to describes that equivalent version of your >> problem - the IOMMU gives the device 48-bit addresses which get >> erroneously truncated because it doesn't know that only 42 bits are >> actually wired up. That situation still requires the device's DMA mask >> to correctly describe its addressing capability just as yours does. > > That problem should only impact virtual machines which have a guest > bus address space covering more than 42 bits of physical RAM, whereas > the problem we have with swiotlb is for the dma-mapping interface. As above, it impacts DMA API use for anything whose addressing capability is narrower than the IOMMU's reported input size and whose driver is able to blindly set a too-big DMA mask. It just happens to be the case that the stars line up on most systems, and for 32-bit devices who keep the default DMA mask. I actually have a third variation of this problem involving a PCI root complex which *could* drive full-width (40-bit) addresses, but won't, due to the way its PCI<->AXI interface is programmed. That would require even more complicated dma-ranges handling to describe the windows of valid physical addresses which it *will* pass, so I'm not pressing the issue - let's just get the basic DMA mask case fixed first. >>> With this direction, semantics of dma mask becomes even more >>> questionable. I'd say dma_mask is candidate for removal (or to move to >>> swiotlb's or iommu's local area) >> >> We still need a way for drivers to communicate a device's probed >> addressing capability to SWIOTLB, so there's always going to have to be >> *some* sort of public interface. Personally, the change in semantics I'd >> like to see is to make dma_set_mask() only fail if DMA is entirely >> disallowed - in the normal case it would always succeed, but the DMA API >> implementation would be permitted to set a smaller mask than requested >> (this is effectively what the x86 IOMMU ops do already). > > With swiotlb enabled, it only needs to fail if the mask does not contain > the swiotlb bounce buffer area, either because the start of RAM is outside > of the mask, or the bounce area has been allocated at the end of ZONE_DMA > and the mask is smaller than ZONE_DMA. Agreed, I'd managed to overlook that specific case, but I'd be inclined to consider "impossible" a subset of "disallowed" still :) Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 14:16 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-10 14:16 UTC (permalink / raw) To: linux-arm-kernel On 10/01/17 13:42, Arnd Bergmann wrote: > On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote: >> On 10/01/17 12:47, Nikita Yushchenko wrote: >>>> The point here is that an IOMMU doesn't solve your issue, and the >>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really >>>> feels to me like the DMA masks should be restricted in of_dma_configure >>>> so that the parent mask is taken into account there, rather than hook >>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to >>>> do something to stop dma_set_mask widening the mask if it was restricted >>>> by of_dma_configure, but I think Robin (cc'd) was playing with that. >>> >>> What issue "IOMMU doesn't solve"? >>> >>> Issue I'm trying to address is - inconsistency within swiotlb >>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then >>> mask is used to decide if bounce buffers are needed or not. This >>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory >>> instead). >> >> The fundamental underlying problem is the "any wide mask is silently >> accepted" part, and that applies equally to IOMMU ops as well. > > It's a much rarer problem for the IOMMU case though, because it only > impacts devices that are restricted to addressing of less than 32-bits. > > If you have an IOMMU enabled, the dma-mapping interface does not care > if the device can do wider than 32 bit addressing, as it will never > hand out IOVAs above 0xffffffff. I can assure you that it will - we constrain allocations to the intersection of the IOMMU domain aperture (normally the IOMMU's physical input address width) and the given device's DMA mask. If both of those are >32 bits then >32-bit IOVAs will fall out. For the arm64/common implementation I have prototyped a copy of the x86 optimisation which always first tries to get 32-bit IOVAs for PCI devices, but even then it can start returning higher addresses if the 32-bit space fills up. >>> I just can't think out what similar issue iommu can have. >>> Do you mean that in iommu case, mask also must not be set to whatever >>> wider than initial value? Why? What is the use of mask in iommu case? Is >>> there any real case when iommu can't address all memory existing in the >>> system? >> >> There's a very subtle misunderstanding there - the DMA mask does not >> describe the memory a device can address, it describes the range of >> addresses the device is capable of generating. Yes, in the non-IOMMU >> case they are equivalent, but once you put an IOMMU in between, the >> problem is merely shifted from "what range of physical addresses can >> this device access" to "what range of IOVAs is valid to give to this >> device" - the fact that those IOVAs can map to any underlying physical >> address only obviates the need for any bouncing at the memory end; it >> doesn't remove the fact that the device has a hardware addressing >> limitation which needs to be accommodated. >> >> The thread Will linked to describes that equivalent version of your >> problem - the IOMMU gives the device 48-bit addresses which get >> erroneously truncated because it doesn't know that only 42 bits are >> actually wired up. That situation still requires the device's DMA mask >> to correctly describe its addressing capability just as yours does. > > That problem should only impact virtual machines which have a guest > bus address space covering more than 42 bits of physical RAM, whereas > the problem we have with swiotlb is for the dma-mapping interface. As above, it impacts DMA API use for anything whose addressing capability is narrower than the IOMMU's reported input size and whose driver is able to blindly set a too-big DMA mask. It just happens to be the case that the stars line up on most systems, and for 32-bit devices who keep the default DMA mask. I actually have a third variation of this problem involving a PCI root complex which *could* drive full-width (40-bit) addresses, but won't, due to the way its PCI<->AXI interface is programmed. That would require even more complicated dma-ranges handling to describe the windows of valid physical addresses which it *will* pass, so I'm not pressing the issue - let's just get the basic DMA mask case fixed first. >>> With this direction, semantics of dma mask becomes even more >>> questionable. I'd say dma_mask is candidate for removal (or to move to >>> swiotlb's or iommu's local area) >> >> We still need a way for drivers to communicate a device's probed >> addressing capability to SWIOTLB, so there's always going to have to be >> *some* sort of public interface. Personally, the change in semantics I'd >> like to see is to make dma_set_mask() only fail if DMA is entirely >> disallowed - in the normal case it would always succeed, but the DMA API >> implementation would be permitted to set a smaller mask than requested >> (this is effectively what the x86 IOMMU ops do already). > > With swiotlb enabled, it only needs to fail if the mask does not contain > the swiotlb bounce buffer area, either because the start of RAM is outside > of the mask, or the bounce area has been allocated at the end of ZONE_DMA > and the mask is smaller than ZONE_DMA. Agreed, I'd managed to overlook that specific case, but I'd be inclined to consider "impossible" a subset of "disallowed" still :) Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 14:16 ` Robin Murphy @ 2017-01-10 15:06 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-10 15:06 UTC (permalink / raw) To: Robin Murphy Cc: Nikita Yushchenko, Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On Tuesday, January 10, 2017 2:16:57 PM CET Robin Murphy wrote: > On 10/01/17 13:42, Arnd Bergmann wrote: > > On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote: > >> On 10/01/17 12:47, Nikita Yushchenko wrote: > >>>> The point here is that an IOMMU doesn't solve your issue, and the > >>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really > >>>> feels to me like the DMA masks should be restricted in of_dma_configure > >>>> so that the parent mask is taken into account there, rather than hook > >>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to > >>>> do something to stop dma_set_mask widening the mask if it was restricted > >>>> by of_dma_configure, but I think Robin (cc'd) was playing with that. > >>> > >>> What issue "IOMMU doesn't solve"? > >>> > >>> Issue I'm trying to address is - inconsistency within swiotlb > >>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > >>> mask is used to decide if bounce buffers are needed or not. This > >>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > >>> instead). > >> > >> The fundamental underlying problem is the "any wide mask is silently > >> accepted" part, and that applies equally to IOMMU ops as well. > > > > It's a much rarer problem for the IOMMU case though, because it only > > impacts devices that are restricted to addressing of less than 32-bits. > > > > If you have an IOMMU enabled, the dma-mapping interface does not care > > if the device can do wider than 32 bit addressing, as it will never > > hand out IOVAs above 0xffffffff. > > I can assure you that it will - we constrain allocations to the > intersection of the IOMMU domain aperture (normally the IOMMU's physical > input address width) and the given device's DMA mask. If both of those > are >32 bits then >32-bit IOVAs will fall out. For the arm64/common > implementation I have prototyped a copy of the x86 optimisation which > always first tries to get 32-bit IOVAs for PCI devices, but even then it > can start returning higher addresses if the 32-bit space fills up. Ok, got it. I have to admit that most of my knowledge about the internals of IOMMUs is from PowerPC of a few years ago, which couldn't do this at all. I agree that we need to do the same thing on swiotlb and iommu then. > >> The thread Will linked to describes that equivalent version of your > >> problem - the IOMMU gives the device 48-bit addresses which get > >> erroneously truncated because it doesn't know that only 42 bits are > >> actually wired up. That situation still requires the device's DMA mask > >> to correctly describe its addressing capability just as yours does. > > > > That problem should only impact virtual machines which have a guest > > bus address space covering more than 42 bits of physical RAM, whereas > > the problem we have with swiotlb is for the dma-mapping interface. > > > I actually have a third variation of this problem involving a PCI root > complex which *could* drive full-width (40-bit) addresses, but won't, > due to the way its PCI<->AXI interface is programmed. That would require > even more complicated dma-ranges handling to describe the windows of > valid physical addresses which it *will* pass, so I'm not pressing the > issue - let's just get the basic DMA mask case fixed first. Can you describe this a little more? We should at least try to not make it harder to solve the next problem while solving this one, so I'd like to understand the exact limitation you are hitting there. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 15:06 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-10 15:06 UTC (permalink / raw) To: linux-arm-kernel On Tuesday, January 10, 2017 2:16:57 PM CET Robin Murphy wrote: > On 10/01/17 13:42, Arnd Bergmann wrote: > > On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote: > >> On 10/01/17 12:47, Nikita Yushchenko wrote: > >>>> The point here is that an IOMMU doesn't solve your issue, and the > >>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really > >>>> feels to me like the DMA masks should be restricted in of_dma_configure > >>>> so that the parent mask is taken into account there, rather than hook > >>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to > >>>> do something to stop dma_set_mask widening the mask if it was restricted > >>>> by of_dma_configure, but I think Robin (cc'd) was playing with that. > >>> > >>> What issue "IOMMU doesn't solve"? > >>> > >>> Issue I'm trying to address is - inconsistency within swiotlb > >>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then > >>> mask is used to decide if bounce buffers are needed or not. This > >>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory > >>> instead). > >> > >> The fundamental underlying problem is the "any wide mask is silently > >> accepted" part, and that applies equally to IOMMU ops as well. > > > > It's a much rarer problem for the IOMMU case though, because it only > > impacts devices that are restricted to addressing of less than 32-bits. > > > > If you have an IOMMU enabled, the dma-mapping interface does not care > > if the device can do wider than 32 bit addressing, as it will never > > hand out IOVAs above 0xffffffff. > > I can assure you that it will - we constrain allocations to the > intersection of the IOMMU domain aperture (normally the IOMMU's physical > input address width) and the given device's DMA mask. If both of those > are >32 bits then >32-bit IOVAs will fall out. For the arm64/common > implementation I have prototyped a copy of the x86 optimisation which > always first tries to get 32-bit IOVAs for PCI devices, but even then it > can start returning higher addresses if the 32-bit space fills up. Ok, got it. I have to admit that most of my knowledge about the internals of IOMMUs is from PowerPC of a few years ago, which couldn't do this at all. I agree that we need to do the same thing on swiotlb and iommu then. > >> The thread Will linked to describes that equivalent version of your > >> problem - the IOMMU gives the device 48-bit addresses which get > >> erroneously truncated because it doesn't know that only 42 bits are > >> actually wired up. That situation still requires the device's DMA mask > >> to correctly describe its addressing capability just as yours does. > > > > That problem should only impact virtual machines which have a guest > > bus address space covering more than 42 bits of physical RAM, whereas > > the problem we have with swiotlb is for the dma-mapping interface. > > > I actually have a third variation of this problem involving a PCI root > complex which *could* drive full-width (40-bit) addresses, but won't, > due to the way its PCI<->AXI interface is programmed. That would require > even more complicated dma-ranges handling to describe the windows of > valid physical addresses which it *will* pass, so I'm not pressing the > issue - let's just get the basic DMA mask case fixed first. Can you describe this a little more? We should at least try to not make it harder to solve the next problem while solving this one, so I'd like to understand the exact limitation you are hitting there. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 14:16 ` Robin Murphy @ 2017-01-11 12:37 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 12:37 UTC (permalink / raw) To: Robin Murphy, Arnd Bergmann Cc: Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig > I actually have a third variation of this problem involving a PCI root > complex which *could* drive full-width (40-bit) addresses, but won't, > due to the way its PCI<->AXI interface is programmed. That would require > even more complicated dma-ranges handling to describe the windows of > valid physical addresses which it *will* pass, so I'm not pressing the > issue - let's just get the basic DMA mask case fixed first. R-Car + NVMe is actually not "basic case". It has PCI<->AXI interface involved. PCI addresses are 64-bit and controller does handle 64-bit addresses there. Mapping between PCI addresses and AXI addresses is defined. But AXI is 32-bit. SoC has iommu that probably could be used between PCIe module and RAM. Although AFAIK nobody made that working yet. Board I work with has 4G of RAM, in 4 banks, located at different parts of wide address space, and only one of them is below 4G. But if iommu is capable of translating addresses such that 4 gigabyte banks map to first 4 gigabytes of address space, then all memory will become available for DMA from PCIe device. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-11 12:37 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 12:37 UTC (permalink / raw) To: linux-arm-kernel > I actually have a third variation of this problem involving a PCI root > complex which *could* drive full-width (40-bit) addresses, but won't, > due to the way its PCI<->AXI interface is programmed. That would require > even more complicated dma-ranges handling to describe the windows of > valid physical addresses which it *will* pass, so I'm not pressing the > issue - let's just get the basic DMA mask case fixed first. R-Car + NVMe is actually not "basic case". It has PCI<->AXI interface involved. PCI addresses are 64-bit and controller does handle 64-bit addresses there. Mapping between PCI addresses and AXI addresses is defined. But AXI is 32-bit. SoC has iommu that probably could be used between PCIe module and RAM. Although AFAIK nobody made that working yet. Board I work with has 4G of RAM, in 4 banks, located at different parts of wide address space, and only one of them is below 4G. But if iommu is capable of translating addresses such that 4 gigabyte banks map to first 4 gigabytes of address space, then all memory will become available for DMA from PCIe device. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-11 12:37 ` Nikita Yushchenko @ 2017-01-11 16:21 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-11 16:21 UTC (permalink / raw) To: Nikita Yushchenko Cc: Robin Murphy, Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On Wednesday, January 11, 2017 3:37:22 PM CET Nikita Yushchenko wrote: > > I actually have a third variation of this problem involving a PCI root > > complex which *could* drive full-width (40-bit) addresses, but won't, > > due to the way its PCI<->AXI interface is programmed. That would require > > even more complicated dma-ranges handling to describe the windows of > > valid physical addresses which it *will* pass, so I'm not pressing the > > issue - let's just get the basic DMA mask case fixed first. > > R-Car + NVMe is actually not "basic case". > > It has PCI<->AXI interface involved. > PCI addresses are 64-bit and controller does handle 64-bit addresses > there. Mapping between PCI addresses and AXI addresses is defined. But > AXI is 32-bit. > > SoC has iommu that probably could be used between PCIe module and RAM. > Although AFAIK nobody made that working yet. > > Board I work with has 4G of RAM, in 4 banks, located at different parts > of wide address space, and only one of them is below 4G. But if iommu is > capable of translating addresses such that 4 gigabyte banks map to first > 4 gigabytes of address space, then all memory will become available for > DMA from PCIe device. You can in theory handle this by defining your own platform specific dma_map_ops, as we used to do in the old days. Unfortunately, the modern way of using the generic IOVA allocation can't handle really it, so it's unclear if the work that would be necessary to support it (and the long term maintenance cost) outweigh the benefits. The more likely option here is to try harder to get the IOMMU working (or show that it's impossible but make sure the next chip gets it right). Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-11 16:21 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-11 16:21 UTC (permalink / raw) To: linux-arm-kernel On Wednesday, January 11, 2017 3:37:22 PM CET Nikita Yushchenko wrote: > > I actually have a third variation of this problem involving a PCI root > > complex which *could* drive full-width (40-bit) addresses, but won't, > > due to the way its PCI<->AXI interface is programmed. That would require > > even more complicated dma-ranges handling to describe the windows of > > valid physical addresses which it *will* pass, so I'm not pressing the > > issue - let's just get the basic DMA mask case fixed first. > > R-Car + NVMe is actually not "basic case". > > It has PCI<->AXI interface involved. > PCI addresses are 64-bit and controller does handle 64-bit addresses > there. Mapping between PCI addresses and AXI addresses is defined. But > AXI is 32-bit. > > SoC has iommu that probably could be used between PCIe module and RAM. > Although AFAIK nobody made that working yet. > > Board I work with has 4G of RAM, in 4 banks, located at different parts > of wide address space, and only one of them is below 4G. But if iommu is > capable of translating addresses such that 4 gigabyte banks map to first > 4 gigabytes of address space, then all memory will become available for > DMA from PCIe device. You can in theory handle this by defining your own platform specific dma_map_ops, as we used to do in the old days. Unfortunately, the modern way of using the generic IOVA allocation can't handle really it, so it's unclear if the work that would be necessary to support it (and the long term maintenance cost) outweigh the benefits. The more likely option here is to try harder to get the IOMMU working (or show that it's impossible but make sure the next chip gets it right). Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-11 12:37 ` Nikita Yushchenko @ 2017-01-11 18:28 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 18:28 UTC (permalink / raw) To: Nikita Yushchenko, Arnd Bergmann Cc: Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On 11/01/17 12:37, Nikita Yushchenko wrote: >> I actually have a third variation of this problem involving a PCI root >> complex which *could* drive full-width (40-bit) addresses, but won't, >> due to the way its PCI<->AXI interface is programmed. That would require >> even more complicated dma-ranges handling to describe the windows of >> valid physical addresses which it *will* pass, so I'm not pressing the >> issue - let's just get the basic DMA mask case fixed first. > > R-Car + NVMe is actually not "basic case". I meant "basic" in terms of what needs to be done in Linux - simply preventing device drivers from overwriting the DT-configured DMA mask will make everything work as well as well as it possibly can on R-Car, both with or without the IOMMU, since apparently all you need is to ensure a PCI device never gets given a DMA address above 4GB. The situation where PCI devices *can* DMA to all of physical memory, but can't use arbitrary addresses *outside* it - which only becomes a problem with an IOMMU - is considerably trickier. > It has PCI<->AXI interface involved. > PCI addresses are 64-bit and controller does handle 64-bit addresses > there. Mapping between PCI addresses and AXI addresses is defined. But > AXI is 32-bit. > > SoC has iommu that probably could be used between PCIe module and RAM. > Although AFAIK nobody made that working yet. > > Board I work with has 4G of RAM, in 4 banks, located at different parts > of wide address space, and only one of them is below 4G. But if iommu is > capable of translating addresses such that 4 gigabyte banks map to first > 4 gigabytes of address space, then all memory will become available for > DMA from PCIe device. The aforementioned situation on Juno is similar yet different - the PLDA XR3 root complex uses an address-based lookup table to translate outgoing PCI memory space transactions to AXI bus addresses with the appropriate attributes, in power-of-two-sized regions. The firmware configures 3 LUT entries - 2GB at 0x8000_0000 and 8GB at 0x8_8000_0000 with cache-coherent attributes to cover the DRAM areas, plus a small one with device attributes covering the GICv2m MSI frame. The issue is that there is no "no match" translation, so any transaction not within one of those regions never makes it out of the root complex at all. That's fine in normal operation, as there's nothing outside those regions in the physical memory map a PCI device should be accessing anyway, but turning on the SMMU is another matter - since the IOVA allocator runs top-down, a PCI device with a 64-bit DMA mask will do a dma_map or dma_alloc, get the physical page mapped to an IOVA up around FF_FFFF_F000 (the SMMU will constrain things to the system bus width of 40 bits), then try to access that address and get a termination straight back from the RC. Similarly, A KVM guest which wants to place its memory at arbitrary locations and expect device passthrough to work is going to have a bad time. I don't know if it's feasible to have the firmware set the LUT up differently, as that might lead to other problems when not using the SMMU, and/or just require far more than the 8 available LUT entries (assuming they have to be non-overlapping - I'm not 100% sure and documentation is sparse). Thus it seems appropriate to describe the currently valid PCI-AXI translations with dma-ranges, but then we'd have multiple entries - last time I looked Linux simply ignores all but the last one in that case - which can't be combined into a simple bitmask, so I'm not entirely sure where to go from there. Especially as so far it seems to be a problem exclusive to one not-widely-available ageing early-access development platform... It happens that limiting all PCI DMA masks to 32 bits would bodge around this problem thanks to the current IOVA allocator behaviour, but that's pretty yuck, and would force unnecessary bouncing for the non-SMMU case. My other hack to carve up IOVA domains to reserve all addresses not matching memblocks is hardly any more realistic, hence why the SMMU is in the Juno DT in a change-it-at-your-own-peril "disabled" state ;) Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-11 18:28 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 18:28 UTC (permalink / raw) To: linux-arm-kernel On 11/01/17 12:37, Nikita Yushchenko wrote: >> I actually have a third variation of this problem involving a PCI root >> complex which *could* drive full-width (40-bit) addresses, but won't, >> due to the way its PCI<->AXI interface is programmed. That would require >> even more complicated dma-ranges handling to describe the windows of >> valid physical addresses which it *will* pass, so I'm not pressing the >> issue - let's just get the basic DMA mask case fixed first. > > R-Car + NVMe is actually not "basic case". I meant "basic" in terms of what needs to be done in Linux - simply preventing device drivers from overwriting the DT-configured DMA mask will make everything work as well as well as it possibly can on R-Car, both with or without the IOMMU, since apparently all you need is to ensure a PCI device never gets given a DMA address above 4GB. The situation where PCI devices *can* DMA to all of physical memory, but can't use arbitrary addresses *outside* it - which only becomes a problem with an IOMMU - is considerably trickier. > It has PCI<->AXI interface involved. > PCI addresses are 64-bit and controller does handle 64-bit addresses > there. Mapping between PCI addresses and AXI addresses is defined. But > AXI is 32-bit. > > SoC has iommu that probably could be used between PCIe module and RAM. > Although AFAIK nobody made that working yet. > > Board I work with has 4G of RAM, in 4 banks, located at different parts > of wide address space, and only one of them is below 4G. But if iommu is > capable of translating addresses such that 4 gigabyte banks map to first > 4 gigabytes of address space, then all memory will become available for > DMA from PCIe device. The aforementioned situation on Juno is similar yet different - the PLDA XR3 root complex uses an address-based lookup table to translate outgoing PCI memory space transactions to AXI bus addresses with the appropriate attributes, in power-of-two-sized regions. The firmware configures 3 LUT entries - 2GB at 0x8000_0000 and 8GB at 0x8_8000_0000 with cache-coherent attributes to cover the DRAM areas, plus a small one with device attributes covering the GICv2m MSI frame. The issue is that there is no "no match" translation, so any transaction not within one of those regions never makes it out of the root complex at all. That's fine in normal operation, as there's nothing outside those regions in the physical memory map a PCI device should be accessing anyway, but turning on the SMMU is another matter - since the IOVA allocator runs top-down, a PCI device with a 64-bit DMA mask will do a dma_map or dma_alloc, get the physical page mapped to an IOVA up around FF_FFFF_F000 (the SMMU will constrain things to the system bus width of 40 bits), then try to access that address and get a termination straight back from the RC. Similarly, A KVM guest which wants to place its memory at arbitrary locations and expect device passthrough to work is going to have a bad time. I don't know if it's feasible to have the firmware set the LUT up differently, as that might lead to other problems when not using the SMMU, and/or just require far more than the 8 available LUT entries (assuming they have to be non-overlapping - I'm not 100% sure and documentation is sparse). Thus it seems appropriate to describe the currently valid PCI-AXI translations with dma-ranges, but then we'd have multiple entries - last time I looked Linux simply ignores all but the last one in that case - which can't be combined into a simple bitmask, so I'm not entirely sure where to go from there. Especially as so far it seems to be a problem exclusive to one not-widely-available ageing early-access development platform... It happens that limiting all PCI DMA masks to 32 bits would bodge around this problem thanks to the current IOVA allocator behaviour, but that's pretty yuck, and would force unnecessary bouncing for the non-SMMU case. My other hack to carve up IOVA domains to reserve all addresses not matching memblocks is hardly any more realistic, hence why the SMMU is in the Juno DT in a change-it-at-your-own-peril "disabled" state ;) Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 13:42 ` Arnd Bergmann @ 2017-01-10 14:59 ` Christoph Hellwig -1 siblings, 0 replies; 75+ messages in thread From: Christoph Hellwig @ 2017-01-10 14:59 UTC (permalink / raw) To: Arnd Bergmann Cc: Robin Murphy, Nikita Yushchenko, Will Deacon, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On Tue, Jan 10, 2017 at 02:42:23PM +0100, Arnd Bergmann wrote: > It's a much rarer problem for the IOMMU case though, because it only > impacts devices that are restricted to addressing of less than 32-bits. > > If you have an IOMMU enabled, the dma-mapping interface does not care > if the device can do wider than 32 bit addressing, as it will never > hand out IOVAs above 0xffffffff. That's absolutely not the case. IOMMUs can and do generate addresses larger than 32-bit. Also various platforms have modes where an IOMMU can be used when <= 32-bit addresses are used and bypassed if full 64-bit addressing is supported and I/O isolation is not explicitly requested. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 14:59 ` Christoph Hellwig 0 siblings, 0 replies; 75+ messages in thread From: Christoph Hellwig @ 2017-01-10 14:59 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 10, 2017 at 02:42:23PM +0100, Arnd Bergmann wrote: > It's a much rarer problem for the IOMMU case though, because it only > impacts devices that are restricted to addressing of less than 32-bits. > > If you have an IOMMU enabled, the dma-mapping interface does not care > if the device can do wider than 32 bit addressing, as it will never > hand out IOVAs above 0xffffffff. That's absolutely not the case. IOMMUs can and do generate addresses larger than 32-bit. Also various platforms have modes where an IOMMU can be used when <= 32-bit addresses are used and bypassed if full 64-bit addressing is supported and I/O isolation is not explicitly requested. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-10 13:25 ` Robin Murphy @ 2017-01-10 14:00 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-10 14:00 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, Nikita Yushchenko There are cases when device supports wide DMA addresses wider than device's connection supports. In this case driver sets DMA mask based on knowledge of device capabilities. That must succeed to allow drivers to initialize. However, swiotlb or iommu still need knowledge about actual device capabilities. To avoid breakage, actual mask must not be set wider than device connection allows. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> CC: Arnd Bergmann <arnd@arndb.de> CC: Robin Murphy <robin.murphy@arm.com> CC: Will Deacon <will.deacon@arm.com> --- arch/arm64/Kconfig | 3 +++ arch/arm64/include/asm/device.h | 1 + arch/arm64/include/asm/dma-mapping.h | 3 +++ arch/arm64/mm/dma-mapping.c | 43 ++++++++++++++++++++++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..afb2c08 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE config NEED_SG_DMA_LENGTH def_bool y +config ARCH_HAS_DMA_SET_COHERENT_MASK + def_bool y + config SMP def_bool y diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h index 243ef25..a57e7bb 100644 --- a/arch/arm64/include/asm/device.h +++ b/arch/arm64/include/asm/device.h @@ -22,6 +22,7 @@ struct dev_archdata { void *iommu; /* private IOMMU data */ #endif bool dma_coherent; + u64 parent_dma_mask; }; struct pdev_archdata { diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ccea82c..eab36d2 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, const struct iommu_ops *iommu, bool coherent); #define arch_setup_dma_ops arch_setup_dma_ops +#define HAVE_ARCH_DMA_SET_MASK 1 +extern int dma_set_mask(struct device *dev, u64 dma_mask); + #ifdef CONFIG_IOMMU_DMA void arch_teardown_dma_ops(struct device *dev); #define arch_teardown_dma_ops arch_teardown_dma_ops diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index e040827..7b1bb87 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size, __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); } +int dma_set_mask(struct device *dev, u64 dma_mask) +{ + const struct dma_map_ops *ops = get_dma_ops(dev); + + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (ops->set_dma_mask) + return ops->set_dma_mask(dev, mask); + + if (!dev->dma_mask || !dma_supported(dev, mask)) + return -EIO; + + *dev->dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_mask); + +int dma_set_coherent_mask(struct device *dev, u64 mask) +{ + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (!dma_supported(dev, mask)) + return -EIO; + + dev->coherent_dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_coherent_mask); + static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page, unsigned long offset, size_t size, enum dma_data_direction dir, @@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; + /* + * we don't yet support buses that have a non-zero mapping. + * Let's hope we won't need it + */ + WARN_ON(dma_base != 0); + + /* + * Whatever the parent bus can set. A device must not set + * a DMA mask larger than this. + */ + dev->archdata.parent_dma_mask = size - 1; + dev->archdata.dma_coherent = coherent; __iommu_setup_dma_ops(dev, dma_base, size, iommu); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-10 14:00 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-10 14:00 UTC (permalink / raw) To: linux-arm-kernel There are cases when device supports wide DMA addresses wider than device's connection supports. In this case driver sets DMA mask based on knowledge of device capabilities. That must succeed to allow drivers to initialize. However, swiotlb or iommu still need knowledge about actual device capabilities. To avoid breakage, actual mask must not be set wider than device connection allows. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> CC: Arnd Bergmann <arnd@arndb.de> CC: Robin Murphy <robin.murphy@arm.com> CC: Will Deacon <will.deacon@arm.com> --- arch/arm64/Kconfig | 3 +++ arch/arm64/include/asm/device.h | 1 + arch/arm64/include/asm/dma-mapping.h | 3 +++ arch/arm64/mm/dma-mapping.c | 43 ++++++++++++++++++++++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..afb2c08 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE config NEED_SG_DMA_LENGTH def_bool y +config ARCH_HAS_DMA_SET_COHERENT_MASK + def_bool y + config SMP def_bool y diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h index 243ef25..a57e7bb 100644 --- a/arch/arm64/include/asm/device.h +++ b/arch/arm64/include/asm/device.h @@ -22,6 +22,7 @@ struct dev_archdata { void *iommu; /* private IOMMU data */ #endif bool dma_coherent; + u64 parent_dma_mask; }; struct pdev_archdata { diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ccea82c..eab36d2 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, const struct iommu_ops *iommu, bool coherent); #define arch_setup_dma_ops arch_setup_dma_ops +#define HAVE_ARCH_DMA_SET_MASK 1 +extern int dma_set_mask(struct device *dev, u64 dma_mask); + #ifdef CONFIG_IOMMU_DMA void arch_teardown_dma_ops(struct device *dev); #define arch_teardown_dma_ops arch_teardown_dma_ops diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index e040827..7b1bb87 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size, __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); } +int dma_set_mask(struct device *dev, u64 dma_mask) +{ + const struct dma_map_ops *ops = get_dma_ops(dev); + + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (ops->set_dma_mask) + return ops->set_dma_mask(dev, mask); + + if (!dev->dma_mask || !dma_supported(dev, mask)) + return -EIO; + + *dev->dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_mask); + +int dma_set_coherent_mask(struct device *dev, u64 mask) +{ + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (!dma_supported(dev, mask)) + return -EIO; + + dev->coherent_dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_coherent_mask); + static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page, unsigned long offset, size_t size, enum dma_data_direction dir, @@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; + /* + * we don't yet support buses that have a non-zero mapping. + * Let's hope we won't need it + */ + WARN_ON(dma_base != 0); + + /* + * Whatever the parent bus can set. A device must not set + * a DMA mask larger than this. + */ + dev->archdata.parent_dma_mask = size - 1; + dev->archdata.dma_coherent = coherent; __iommu_setup_dma_ops(dev, dma_base, size, iommu); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-10 14:00 ` Nikita Yushchenko @ 2017-01-10 17:14 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-10 17:14 UTC (permalink / raw) To: Nikita Yushchenko, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan On 10/01/17 14:00, Nikita Yushchenko wrote: > There are cases when device supports wide DMA addresses wider than > device's connection supports. > > In this case driver sets DMA mask based on knowledge of device > capabilities. That must succeed to allow drivers to initialize. > > However, swiotlb or iommu still need knowledge about actual device > capabilities. To avoid breakage, actual mask must not be set wider than > device connection allows. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > CC: Arnd Bergmann <arnd@arndb.de> > CC: Robin Murphy <robin.murphy@arm.com> > CC: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/Kconfig | 3 +++ > arch/arm64/include/asm/device.h | 1 + > arch/arm64/include/asm/dma-mapping.h | 3 +++ > arch/arm64/mm/dma-mapping.c | 43 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 50 insertions(+) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 1117421..afb2c08 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE > config NEED_SG_DMA_LENGTH > def_bool y > > +config ARCH_HAS_DMA_SET_COHERENT_MASK > + def_bool y > + > config SMP > def_bool y > > diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h > index 243ef25..a57e7bb 100644 > --- a/arch/arm64/include/asm/device.h > +++ b/arch/arm64/include/asm/device.h > @@ -22,6 +22,7 @@ struct dev_archdata { > void *iommu; /* private IOMMU data */ > #endif > bool dma_coherent; > + u64 parent_dma_mask; > }; > > struct pdev_archdata { > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h > index ccea82c..eab36d2 100644 > --- a/arch/arm64/include/asm/dma-mapping.h > +++ b/arch/arm64/include/asm/dma-mapping.h > @@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > const struct iommu_ops *iommu, bool coherent); > #define arch_setup_dma_ops arch_setup_dma_ops > > +#define HAVE_ARCH_DMA_SET_MASK 1 > +extern int dma_set_mask(struct device *dev, u64 dma_mask); > + > #ifdef CONFIG_IOMMU_DMA > void arch_teardown_dma_ops(struct device *dev); > #define arch_teardown_dma_ops arch_teardown_dma_ops > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > index e040827..7b1bb87 100644 > --- a/arch/arm64/mm/dma-mapping.c > +++ b/arch/arm64/mm/dma-mapping.c > @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size, > __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); > } > > +int dma_set_mask(struct device *dev, u64 dma_mask) > +{ > + const struct dma_map_ops *ops = get_dma_ops(dev); > + > + if (mask > dev->archdata.parent_dma_mask) > + mask = dev->archdata.parent_dma_mask; > + > + if (ops->set_dma_mask) > + return ops->set_dma_mask(dev, mask); > + > + if (!dev->dma_mask || !dma_supported(dev, mask)) > + return -EIO; > + > + *dev->dma_mask = mask; > + return 0; > +} > +EXPORT_SYMBOL(dma_set_mask); > + > +int dma_set_coherent_mask(struct device *dev, u64 mask) > +{ > + if (mask > dev->archdata.parent_dma_mask) > + mask = dev->archdata.parent_dma_mask; > + > + if (!dma_supported(dev, mask)) > + return -EIO; > + > + dev->coherent_dma_mask = mask; > + return 0; > +} > +EXPORT_SYMBOL(dma_set_coherent_mask); > + > static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page, > unsigned long offset, size_t size, > enum dma_data_direction dir, > @@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > if (!dev->archdata.dma_ops) > dev->archdata.dma_ops = &swiotlb_dma_ops; > > + /* > + * we don't yet support buses that have a non-zero mapping. > + * Let's hope we won't need it > + */ > + WARN_ON(dma_base != 0); I believe we now accomodate the bus remap bits on BCM2837 as a DMA offset, so unfortunately I think this is no longer true. > + /* > + * Whatever the parent bus can set. A device must not set > + * a DMA mask larger than this. > + */ > + dev->archdata.parent_dma_mask = size - 1; This will effectively constrain *all* DMA masks to be 32-bit, since for 99% of devices we're going to see a size derived from the default mask passed in here. I worry that that's liable to lead to performance and stability regressions (now that the block layer can apparently generate sufficient readahead to ovflow a typical SWIOTLB bounce buffer[1]). Whilst DT users would be able to mitigate that by putting explicit "dma-ranges" properties on every device node, it's less clear what we'd do for ACPI. I reckon the easiest way forward would be to pass in some flag to arch_setup_dma_ops to indicate whether it's an explicitly-configured range or not - then simply initialising parent_dma_mask to ~0 for the default case *should* keep things working as before. Robin. [1]:https://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg26532.html > + > dev->archdata.dma_coherent = coherent; > __iommu_setup_dma_ops(dev, dma_base, size, iommu); > } > ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-10 17:14 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-10 17:14 UTC (permalink / raw) To: linux-arm-kernel On 10/01/17 14:00, Nikita Yushchenko wrote: > There are cases when device supports wide DMA addresses wider than > device's connection supports. > > In this case driver sets DMA mask based on knowledge of device > capabilities. That must succeed to allow drivers to initialize. > > However, swiotlb or iommu still need knowledge about actual device > capabilities. To avoid breakage, actual mask must not be set wider than > device connection allows. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > CC: Arnd Bergmann <arnd@arndb.de> > CC: Robin Murphy <robin.murphy@arm.com> > CC: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/Kconfig | 3 +++ > arch/arm64/include/asm/device.h | 1 + > arch/arm64/include/asm/dma-mapping.h | 3 +++ > arch/arm64/mm/dma-mapping.c | 43 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 50 insertions(+) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 1117421..afb2c08 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE > config NEED_SG_DMA_LENGTH > def_bool y > > +config ARCH_HAS_DMA_SET_COHERENT_MASK > + def_bool y > + > config SMP > def_bool y > > diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h > index 243ef25..a57e7bb 100644 > --- a/arch/arm64/include/asm/device.h > +++ b/arch/arm64/include/asm/device.h > @@ -22,6 +22,7 @@ struct dev_archdata { > void *iommu; /* private IOMMU data */ > #endif > bool dma_coherent; > + u64 parent_dma_mask; > }; > > struct pdev_archdata { > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h > index ccea82c..eab36d2 100644 > --- a/arch/arm64/include/asm/dma-mapping.h > +++ b/arch/arm64/include/asm/dma-mapping.h > @@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > const struct iommu_ops *iommu, bool coherent); > #define arch_setup_dma_ops arch_setup_dma_ops > > +#define HAVE_ARCH_DMA_SET_MASK 1 > +extern int dma_set_mask(struct device *dev, u64 dma_mask); > + > #ifdef CONFIG_IOMMU_DMA > void arch_teardown_dma_ops(struct device *dev); > #define arch_teardown_dma_ops arch_teardown_dma_ops > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > index e040827..7b1bb87 100644 > --- a/arch/arm64/mm/dma-mapping.c > +++ b/arch/arm64/mm/dma-mapping.c > @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size, > __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); > } > > +int dma_set_mask(struct device *dev, u64 dma_mask) > +{ > + const struct dma_map_ops *ops = get_dma_ops(dev); > + > + if (mask > dev->archdata.parent_dma_mask) > + mask = dev->archdata.parent_dma_mask; > + > + if (ops->set_dma_mask) > + return ops->set_dma_mask(dev, mask); > + > + if (!dev->dma_mask || !dma_supported(dev, mask)) > + return -EIO; > + > + *dev->dma_mask = mask; > + return 0; > +} > +EXPORT_SYMBOL(dma_set_mask); > + > +int dma_set_coherent_mask(struct device *dev, u64 mask) > +{ > + if (mask > dev->archdata.parent_dma_mask) > + mask = dev->archdata.parent_dma_mask; > + > + if (!dma_supported(dev, mask)) > + return -EIO; > + > + dev->coherent_dma_mask = mask; > + return 0; > +} > +EXPORT_SYMBOL(dma_set_coherent_mask); > + > static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page, > unsigned long offset, size_t size, > enum dma_data_direction dir, > @@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > if (!dev->archdata.dma_ops) > dev->archdata.dma_ops = &swiotlb_dma_ops; > > + /* > + * we don't yet support buses that have a non-zero mapping. > + * Let's hope we won't need it > + */ > + WARN_ON(dma_base != 0); I believe we now accomodate the bus remap bits on BCM2837 as a DMA offset, so unfortunately I think this is no longer true. > + /* > + * Whatever the parent bus can set. A device must not set > + * a DMA mask larger than this. > + */ > + dev->archdata.parent_dma_mask = size - 1; This will effectively constrain *all* DMA masks to be 32-bit, since for 99% of devices we're going to see a size derived from the default mask passed in here. I worry that that's liable to lead to performance and stability regressions (now that the block layer can apparently generate sufficient readahead to ovflow a typical SWIOTLB bounce buffer[1]). Whilst DT users would be able to mitigate that by putting explicit "dma-ranges" properties on every device node, it's less clear what we'd do for ACPI. I reckon the easiest way forward would be to pass in some flag to arch_setup_dma_ops to indicate whether it's an explicitly-configured range or not - then simply initialising parent_dma_mask to ~0 for the default case *should* keep things working as before. Robin. [1]:https://www.mail-archive.com/virtualization at lists.linux-foundation.org/msg26532.html > + > dev->archdata.dma_coherent = coherent; > __iommu_setup_dma_ops(dev, dma_base, size, iommu); > } > ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-10 17:14 ` Robin Murphy @ 2017-01-11 7:59 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 7:59 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan >> + /* >> + * we don't yet support buses that have a non-zero mapping. >> + * Let's hope we won't need it >> + */ >> + WARN_ON(dma_base != 0); > > I believe we now accomodate the bus remap bits on BCM2837 as a DMA > offset, so unfortunately I think this is no longer true. Arnd, this check is from you. Any updates? Perhaps this check can be just dropped? In swiotlb code, dma address (i.e. with offset already applied) is checked against mask. Not sure what 'dma_base' means in iommu case. >> + /* >> + * Whatever the parent bus can set. A device must not set >> + * a DMA mask larger than this. >> + */ >> + dev->archdata.parent_dma_mask = size - 1; > > This will effectively constrain *all* DMA masks to be 32-bit, since for > 99% of devices we're going to see a size derived from the default mask > passed in here. I worry that that's liable to lead to performance and > stability regressions That was exactly my concern when I first tried to address this issue. My first attempt was to alter very locally exact configuration where problem shows, while ensuring that everything else stays as is. See https://lkml.org/lkml/2016/12/29/218 But looks like people want a generic solution. > I reckon the easiest way forward would be to pass in some flag to > arch_setup_dma_ops to indicate whether it's an explicitly-configured > range or not - then simply initialising parent_dma_mask to ~0 for the > default case *should* keep things working as before. Currently only arm, arm64 and mips define arch_setup_dma_ops(). Mips version only checks 'coherent' argument, 'size' is used only by arm and arm64. Maybe move setting the default from caller to callee? I.e. pass size=0 if no explicit information exists, and let architecture handle that? ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 7:59 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 7:59 UTC (permalink / raw) To: linux-arm-kernel >> + /* >> + * we don't yet support buses that have a non-zero mapping. >> + * Let's hope we won't need it >> + */ >> + WARN_ON(dma_base != 0); > > I believe we now accomodate the bus remap bits on BCM2837 as a DMA > offset, so unfortunately I think this is no longer true. Arnd, this check is from you. Any updates? Perhaps this check can be just dropped? In swiotlb code, dma address (i.e. with offset already applied) is checked against mask. Not sure what 'dma_base' means in iommu case. >> + /* >> + * Whatever the parent bus can set. A device must not set >> + * a DMA mask larger than this. >> + */ >> + dev->archdata.parent_dma_mask = size - 1; > > This will effectively constrain *all* DMA masks to be 32-bit, since for > 99% of devices we're going to see a size derived from the default mask > passed in here. I worry that that's liable to lead to performance and > stability regressions That was exactly my concern when I first tried to address this issue. My first attempt was to alter very locally exact configuration where problem shows, while ensuring that everything else stays as is. See https://lkml.org/lkml/2016/12/29/218 But looks like people want a generic solution. > I reckon the easiest way forward would be to pass in some flag to > arch_setup_dma_ops to indicate whether it's an explicitly-configured > range or not - then simply initialising parent_dma_mask to ~0 for the > default case *should* keep things working as before. Currently only arm, arm64 and mips define arch_setup_dma_ops(). Mips version only checks 'coherent' argument, 'size' is used only by arm and arm64. Maybe move setting the default from caller to callee? I.e. pass size=0 if no explicit information exists, and let architecture handle that? ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 7:59 ` Nikita Yushchenko @ 2017-01-11 11:54 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 11:54 UTC (permalink / raw) To: Nikita Yushchenko, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan On 11/01/17 07:59, Nikita Yushchenko wrote: >>> + /* >>> + * we don't yet support buses that have a non-zero mapping. >>> + * Let's hope we won't need it >>> + */ >>> + WARN_ON(dma_base != 0); >> >> I believe we now accomodate the bus remap bits on BCM2837 as a DMA >> offset, so unfortunately I think this is no longer true. > > Arnd, this check is from you. Any updates? Perhaps this check can be > just dropped? > > In swiotlb code, dma address (i.e. with offset already applied) is > checked against mask. Not sure what 'dma_base' means in iommu case. > >>> + /* >>> + * Whatever the parent bus can set. A device must not set >>> + * a DMA mask larger than this. >>> + */ >>> + dev->archdata.parent_dma_mask = size - 1; >> >> This will effectively constrain *all* DMA masks to be 32-bit, since for >> 99% of devices we're going to see a size derived from the default mask >> passed in here. I worry that that's liable to lead to performance and >> stability regressions > > That was exactly my concern when I first tried to address this issue. My > first attempt was to alter very locally exact configuration where > problem shows, while ensuring that everything else stays as is. See > https://lkml.org/lkml/2016/12/29/218 > > But looks like people want a generic solution. > >> I reckon the easiest way forward would be to pass in some flag to >> arch_setup_dma_ops to indicate whether it's an explicitly-configured >> range or not - then simply initialising parent_dma_mask to ~0 for the >> default case *should* keep things working as before. > > Currently only arm, arm64 and mips define arch_setup_dma_ops(). > Mips version only checks 'coherent' argument, 'size' is used only by arm > and arm64. > > Maybe move setting the default from caller to callee? > I.e. pass size=0 if no explicit information exists, and let architecture > handle that? Yes, I think that ought to work, although the __iommu_setup_dma_ops() call will still want a real size reflecting the default mask, so something like: if (size == 0) { dev->archdata.parent_dma_mask = ~0; size = 1ULL << 32; } else { dev->archdata.parent_dma_mask = size - 1; } should probably do the trick. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 11:54 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 11:54 UTC (permalink / raw) To: linux-arm-kernel On 11/01/17 07:59, Nikita Yushchenko wrote: >>> + /* >>> + * we don't yet support buses that have a non-zero mapping. >>> + * Let's hope we won't need it >>> + */ >>> + WARN_ON(dma_base != 0); >> >> I believe we now accomodate the bus remap bits on BCM2837 as a DMA >> offset, so unfortunately I think this is no longer true. > > Arnd, this check is from you. Any updates? Perhaps this check can be > just dropped? > > In swiotlb code, dma address (i.e. with offset already applied) is > checked against mask. Not sure what 'dma_base' means in iommu case. > >>> + /* >>> + * Whatever the parent bus can set. A device must not set >>> + * a DMA mask larger than this. >>> + */ >>> + dev->archdata.parent_dma_mask = size - 1; >> >> This will effectively constrain *all* DMA masks to be 32-bit, since for >> 99% of devices we're going to see a size derived from the default mask >> passed in here. I worry that that's liable to lead to performance and >> stability regressions > > That was exactly my concern when I first tried to address this issue. My > first attempt was to alter very locally exact configuration where > problem shows, while ensuring that everything else stays as is. See > https://lkml.org/lkml/2016/12/29/218 > > But looks like people want a generic solution. > >> I reckon the easiest way forward would be to pass in some flag to >> arch_setup_dma_ops to indicate whether it's an explicitly-configured >> range or not - then simply initialising parent_dma_mask to ~0 for the >> default case *should* keep things working as before. > > Currently only arm, arm64 and mips define arch_setup_dma_ops(). > Mips version only checks 'coherent' argument, 'size' is used only by arm > and arm64. > > Maybe move setting the default from caller to callee? > I.e. pass size=0 if no explicit information exists, and let architecture > handle that? Yes, I think that ought to work, although the __iommu_setup_dma_ops() call will still want a real size reflecting the default mask, so something like: if (size == 0) { dev->archdata.parent_dma_mask = ~0; size = 1ULL << 32; } else { dev->archdata.parent_dma_mask = size - 1; } should probably do the trick. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 11:54 ` Robin Murphy @ 2017-01-11 13:41 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 13:41 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan > Yes, I think that ought to work, although the __iommu_setup_dma_ops() > call will still want a real size reflecting the default mask I see iommu_dma_ops do not define set_dma_mask. So what if setup was done for size reflecting one mask and then driver changes mask? Will things still operate correctly? ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 13:41 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 13:41 UTC (permalink / raw) To: linux-arm-kernel > Yes, I think that ought to work, although the __iommu_setup_dma_ops() > call will still want a real size reflecting the default mask I see iommu_dma_ops do not define set_dma_mask. So what if setup was done for size reflecting one mask and then driver changes mask? Will things still operate correctly? ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 13:41 ` Nikita Yushchenko @ 2017-01-11 14:50 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 14:50 UTC (permalink / raw) To: Nikita Yushchenko, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan On 11/01/17 13:41, Nikita Yushchenko wrote: >> Yes, I think that ought to work, although the __iommu_setup_dma_ops() >> call will still want a real size reflecting the default mask > > I see iommu_dma_ops do not define set_dma_mask. > > So what if setup was done for size reflecting one mask and then driver > changes mask? Will things still operate correctly? We've overridden dma_set_mask() at the function level, so it should always apply regardless. Besides, none of the arm64 ops implement .set_dma_mask anyway, so we could possibly drop the references to it altogether. Conversely, I suppose we could just implement said callback for swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not sure which approach is preferable - the latter seems arguably cleaner in isolation, but would also be less consistent with how the coherent mask has to be handled. Ho hum. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 14:50 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 14:50 UTC (permalink / raw) To: linux-arm-kernel On 11/01/17 13:41, Nikita Yushchenko wrote: >> Yes, I think that ought to work, although the __iommu_setup_dma_ops() >> call will still want a real size reflecting the default mask > > I see iommu_dma_ops do not define set_dma_mask. > > So what if setup was done for size reflecting one mask and then driver > changes mask? Will things still operate correctly? We've overridden dma_set_mask() at the function level, so it should always apply regardless. Besides, none of the arm64 ops implement .set_dma_mask anyway, so we could possibly drop the references to it altogether. Conversely, I suppose we could just implement said callback for swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not sure which approach is preferable - the latter seems arguably cleaner in isolation, but would also be less consistent with how the coherent mask has to be handled. Ho hum. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 14:50 ` Robin Murphy (?) @ 2017-01-11 16:03 ` Nikita Yushchenko 2017-01-11 16:50 ` Robin Murphy -1 siblings, 1 reply; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 16:03 UTC (permalink / raw) To: linux-arm-kernel 11.01.2017 17:50, Robin Murphy ?????: > On 11/01/17 13:41, Nikita Yushchenko wrote: >>> Yes, I think that ought to work, although the __iommu_setup_dma_ops() >>> call will still want a real size reflecting the default mask >> >> I see iommu_dma_ops do not define set_dma_mask. >> >> So what if setup was done for size reflecting one mask and then driver >> changes mask? Will things still operate correctly? > > We've overridden dma_set_mask() at the function level, so it should > always apply regardless. Besides, none of the arm64 ops implement > .set_dma_mask anyway, so we could possibly drop the references to it > altogether. > > Conversely, I suppose we could just implement said callback for > swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking > function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not > sure which approach is preferable - the latter seems arguably cleaner in > isolation, but would also be less consistent with how the coherent mask > has to be handled. Ho hum. I mean, before patch is applied. In the current mainline codebase, arm64 iommu does setup dependent on [default] dma_mask, but does not anyhow react on dma mask change. I don't know much details about arm64 iommu, but from distant view this combination looks incorrect: - if behavior of this hardware should depend on dma_mask of device, then it should handle mask change, - if behavior of this hardware should not depend on dma_mask of device, then what for to pass size to it's setup? ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 16:03 ` Nikita Yushchenko @ 2017-01-11 16:50 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 16:50 UTC (permalink / raw) To: Nikita Yushchenko, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan On 11/01/17 16:03, Nikita Yushchenko wrote: > > > 11.01.2017 17:50, Robin Murphy пишет: >> On 11/01/17 13:41, Nikita Yushchenko wrote: >>>> Yes, I think that ought to work, although the __iommu_setup_dma_ops() >>>> call will still want a real size reflecting the default mask >>> >>> I see iommu_dma_ops do not define set_dma_mask. >>> >>> So what if setup was done for size reflecting one mask and then driver >>> changes mask? Will things still operate correctly? >> >> We've overridden dma_set_mask() at the function level, so it should >> always apply regardless. Besides, none of the arm64 ops implement >> .set_dma_mask anyway, so we could possibly drop the references to it >> altogether. >> >> Conversely, I suppose we could just implement said callback for >> swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking >> function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not >> sure which approach is preferable - the latter seems arguably cleaner in >> isolation, but would also be less consistent with how the coherent mask >> has to be handled. Ho hum. > > I mean, before patch is applied. > > In the current mainline codebase, arm64 iommu does setup dependent on > [default] dma_mask, but does not anyhow react on dma mask change. > > I don't know much details about arm64 iommu, but from distant view this > combination looks incorrect: > - if behavior of this hardware should depend on dma_mask of device, then > it should handle mask change, > - if behavior of this hardware should not depend on dma_mask of device, > then what for to pass size to it's setup? Ah, right, I did indeed misunderstand. The IOMMU code is happy with the DMA masks changing, since it always checks the appropriate one at the point of IOVA allocation. The initial size given to __iommu_setup_dma_ops() really just sets up a caching threshold in the IOVA domain - if there is an explicitly-configured mask, then it's generally good to set the limit based on that, but otherwise the default 32-bit limit is fine even if the driver subsequently alters the device's mask(s) before making DMA API calls. Your patch won't actually change any behaviour in this regard, as long as it still passes a 4GB size by default. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 16:50 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-11 16:50 UTC (permalink / raw) To: linux-arm-kernel On 11/01/17 16:03, Nikita Yushchenko wrote: > > > 11.01.2017 17:50, Robin Murphy ?????: >> On 11/01/17 13:41, Nikita Yushchenko wrote: >>>> Yes, I think that ought to work, although the __iommu_setup_dma_ops() >>>> call will still want a real size reflecting the default mask >>> >>> I see iommu_dma_ops do not define set_dma_mask. >>> >>> So what if setup was done for size reflecting one mask and then driver >>> changes mask? Will things still operate correctly? >> >> We've overridden dma_set_mask() at the function level, so it should >> always apply regardless. Besides, none of the arm64 ops implement >> .set_dma_mask anyway, so we could possibly drop the references to it >> altogether. >> >> Conversely, I suppose we could just implement said callback for >> swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking >> function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not >> sure which approach is preferable - the latter seems arguably cleaner in >> isolation, but would also be less consistent with how the coherent mask >> has to be handled. Ho hum. > > I mean, before patch is applied. > > In the current mainline codebase, arm64 iommu does setup dependent on > [default] dma_mask, but does not anyhow react on dma mask change. > > I don't know much details about arm64 iommu, but from distant view this > combination looks incorrect: > - if behavior of this hardware should depend on dma_mask of device, then > it should handle mask change, > - if behavior of this hardware should not depend on dma_mask of device, > then what for to pass size to it's setup? Ah, right, I did indeed misunderstand. The IOMMU code is happy with the DMA masks changing, since it always checks the appropriate one at the point of IOVA allocation. The initial size given to __iommu_setup_dma_ops() really just sets up a caching threshold in the IOVA domain - if there is an explicitly-configured mask, then it's generally good to set the limit based on that, but otherwise the default 32-bit limit is fine even if the driver subsequently alters the device's mask(s) before making DMA API calls. Your patch won't actually change any behaviour in this regard, as long as it still passes a 4GB size by default. Robin. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports 2017-01-10 17:14 ` Robin Murphy @ 2017-01-11 18:31 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 18:31 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov, Nikita Yushchenko > I reckon the easiest way forward would be to pass in some flag to > arch_setup_dma_ops to indicate whether it's an explicitly-configured > range or not - then simply initialising parent_dma_mask to ~0 for the > default case *should* keep things working as before. Tried to do that. --- Nikita Yushchenko (2): dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() arm64: avoid increasing DMA masks above what hardware supports arch/arm/include/asm/dma-mapping.h | 1 + arch/arm/mm/dma-mapping.c | 3 +- arch/arm64/Kconfig | 3 ++ arch/arm64/include/asm/device.h | 1 + arch/arm64/include/asm/dma-mapping.h | 6 +++- arch/arm64/mm/dma-mapping.c | 43 +++++++++++++++++++++++++- arch/mips/include/asm/dma-mapping.h | 3 +- drivers/acpi/scan.c | 2 +- drivers/iommu/rockchip-iommu.c | 2 +- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +- drivers/of/device.c | 5 ++- drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 2 +- 12 files changed, 64 insertions(+), 9 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports @ 2017-01-11 18:31 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 18:31 UTC (permalink / raw) To: linux-arm-kernel > I reckon the easiest way forward would be to pass in some flag to > arch_setup_dma_ops to indicate whether it's an explicitly-configured > range or not - then simply initialising parent_dma_mask to ~0 for the > default case *should* keep things working as before. Tried to do that. --- Nikita Yushchenko (2): dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() arm64: avoid increasing DMA masks above what hardware supports arch/arm/include/asm/dma-mapping.h | 1 + arch/arm/mm/dma-mapping.c | 3 +- arch/arm64/Kconfig | 3 ++ arch/arm64/include/asm/device.h | 1 + arch/arm64/include/asm/dma-mapping.h | 6 +++- arch/arm64/mm/dma-mapping.c | 43 +++++++++++++++++++++++++- arch/mips/include/asm/dma-mapping.h | 3 +- drivers/acpi/scan.c | 2 +- drivers/iommu/rockchip-iommu.c | 2 +- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +- drivers/of/device.c | 5 ++- drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 2 +- 12 files changed, 64 insertions(+), 9 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-11 18:31 ` Nikita Yushchenko @ 2017-01-11 18:31 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 18:31 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov, Nikita Yushchenko There are cases when device is capable of wide DMA mask (and driver issues corresponding dma_set_mask() call), but bus device sits on can't support wide address. Example: NVMe device behind PCIe controller sitting on 32-bit SoC bus. To support such case, architecture needs information about such limitations. Such information can originate from dma-ranges property in device tree, and is passed to architecture via arch_setup_dma_ops() call. Problem is that in wide majority of cases, no dma range is defined. E.g. ACPI has no means to define it. Thus default range (usually full 32-bit range, i.e. 4G starting at zero address) is passed instead. If architecture enforce this range, all setups currently using wide DMA addresses without explicitly defining support for that via device tree will break. This is bad, especially for ACPI based platforms. To avoid that, this patch adds additional boolean argument to arch_setup_dma_ops() to show if range originates from authorative source and thus should be enforced, or is just a guess and should be handled as such. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> --- arch/arm/include/asm/dma-mapping.h | 1 + arch/arm/mm/dma-mapping.c | 3 ++- arch/arm64/include/asm/dma-mapping.h | 3 ++- arch/arm64/mm/dma-mapping.c | 3 ++- arch/mips/include/asm/dma-mapping.h | 3 ++- drivers/acpi/scan.c | 2 +- drivers/iommu/rockchip-iommu.c | 2 +- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +- drivers/of/device.c | 5 ++++- drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 2 +- 10 files changed, 17 insertions(+), 9 deletions(-) diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index bf02dbd..2a3863e 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -117,6 +117,7 @@ static inline unsigned long dma_max_pfn(struct device *dev) #define arch_setup_dma_ops arch_setup_dma_ops extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, + bool enforce_range, const struct iommu_ops *iommu, bool coherent); #define arch_teardown_dma_ops arch_teardown_dma_ops diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index ab77100..b8b11f8 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -2380,7 +2380,8 @@ static struct dma_map_ops *arm_get_dma_map_ops(bool coherent) } void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, - const struct iommu_ops *iommu, bool coherent) + bool enforce_range, const struct iommu_ops *iommu, + bool coherent) { struct dma_map_ops *dma_ops; diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ccea82c..ae1c23f 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -48,7 +48,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev) } void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, - const struct iommu_ops *iommu, bool coherent); + bool enforce_range, const struct iommu_ops *iommu, + bool coherent); #define arch_setup_dma_ops arch_setup_dma_ops #ifdef CONFIG_IOMMU_DMA diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index e040827..ea295f1 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -953,7 +953,8 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, #endif /* CONFIG_IOMMU_DMA */ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, - const struct iommu_ops *iommu, bool coherent) + bool enforce_range, const struct iommu_ops *iommu, + bool coherent) { if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h index 7aa71b9..6af4d87 100644 --- a/arch/mips/include/asm/dma-mapping.h +++ b/arch/mips/include/asm/dma-mapping.h @@ -34,7 +34,8 @@ extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size, #define arch_setup_dma_ops arch_setup_dma_ops static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, - u64 size, const struct iommu_ops *iommu, + u64 size, bool enforce_range, + const struct iommu_ops *iommu, bool coherent) { #ifdef CONFIG_DMA_PERDEV_COHERENT diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1926918..dea17a5 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1385,7 +1385,7 @@ void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) * Assume dma valid range starts at 0 and covers the whole * coherent_dma_mask. */ - arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu, + arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu, attr == DEV_DMA_COHERENT); } EXPORT_SYMBOL_GPL(acpi_dma_configure); diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 9afcbf7..0995ab3 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev) return -ENOMEM; /* Set dma_ops for dev, otherwise it would be dummy_dma_ops */ - arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false); + arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false); dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index c9b7ad6..19f70d8 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev) priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */ /* device used for DMA mapping */ - arch_setup_dma_ops(dev, 0, 0, NULL, false); + arch_setup_dma_ops(dev, 0, 0, false, NULL, false); err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40)); if (err) { dev_err(dev, "dma_coerce_mask_and_coherent() failed\n"); diff --git a/drivers/of/device.c b/drivers/of/device.c index fd5cfad..1cc2115 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) bool coherent; unsigned long offset; const struct iommu_ops *iommu; + bool enforce_range = false; /* * Set default coherent_dma_mask to 32 bit. Drivers are expected to @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) return; } dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); + + enforce_range = true; } dev->dma_pfn_offset = offset; @@ -147,7 +150,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) dev_dbg(dev, "device is%sbehind an iommu\n", iommu ? " " : " not "); - arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); + arch_setup_dma_ops(dev, dma_addr, size, enforce_range, iommu, coherent); } EXPORT_SYMBOL_GPL(of_dma_configure); diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c index 5ac373c..480b644 100644 --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, /* Objects are coherent, unless 'no shareability' flag set. */ if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); /* * The device-specific probe callback will get invoked by device_add() -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-11 18:31 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 18:31 UTC (permalink / raw) To: linux-arm-kernel There are cases when device is capable of wide DMA mask (and driver issues corresponding dma_set_mask() call), but bus device sits on can't support wide address. Example: NVMe device behind PCIe controller sitting on 32-bit SoC bus. To support such case, architecture needs information about such limitations. Such information can originate from dma-ranges property in device tree, and is passed to architecture via arch_setup_dma_ops() call. Problem is that in wide majority of cases, no dma range is defined. E.g. ACPI has no means to define it. Thus default range (usually full 32-bit range, i.e. 4G starting at zero address) is passed instead. If architecture enforce this range, all setups currently using wide DMA addresses without explicitly defining support for that via device tree will break. This is bad, especially for ACPI based platforms. To avoid that, this patch adds additional boolean argument to arch_setup_dma_ops() to show if range originates from authorative source and thus should be enforced, or is just a guess and should be handled as such. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> --- arch/arm/include/asm/dma-mapping.h | 1 + arch/arm/mm/dma-mapping.c | 3 ++- arch/arm64/include/asm/dma-mapping.h | 3 ++- arch/arm64/mm/dma-mapping.c | 3 ++- arch/mips/include/asm/dma-mapping.h | 3 ++- drivers/acpi/scan.c | 2 +- drivers/iommu/rockchip-iommu.c | 2 +- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +- drivers/of/device.c | 5 ++++- drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 2 +- 10 files changed, 17 insertions(+), 9 deletions(-) diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index bf02dbd..2a3863e 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -117,6 +117,7 @@ static inline unsigned long dma_max_pfn(struct device *dev) #define arch_setup_dma_ops arch_setup_dma_ops extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, + bool enforce_range, const struct iommu_ops *iommu, bool coherent); #define arch_teardown_dma_ops arch_teardown_dma_ops diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index ab77100..b8b11f8 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -2380,7 +2380,8 @@ static struct dma_map_ops *arm_get_dma_map_ops(bool coherent) } void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, - const struct iommu_ops *iommu, bool coherent) + bool enforce_range, const struct iommu_ops *iommu, + bool coherent) { struct dma_map_ops *dma_ops; diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ccea82c..ae1c23f 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -48,7 +48,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev) } void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, - const struct iommu_ops *iommu, bool coherent); + bool enforce_range, const struct iommu_ops *iommu, + bool coherent); #define arch_setup_dma_ops arch_setup_dma_ops #ifdef CONFIG_IOMMU_DMA diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index e040827..ea295f1 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -953,7 +953,8 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, #endif /* CONFIG_IOMMU_DMA */ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, - const struct iommu_ops *iommu, bool coherent) + bool enforce_range, const struct iommu_ops *iommu, + bool coherent) { if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h index 7aa71b9..6af4d87 100644 --- a/arch/mips/include/asm/dma-mapping.h +++ b/arch/mips/include/asm/dma-mapping.h @@ -34,7 +34,8 @@ extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size, #define arch_setup_dma_ops arch_setup_dma_ops static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, - u64 size, const struct iommu_ops *iommu, + u64 size, bool enforce_range, + const struct iommu_ops *iommu, bool coherent) { #ifdef CONFIG_DMA_PERDEV_COHERENT diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1926918..dea17a5 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1385,7 +1385,7 @@ void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) * Assume dma valid range starts at 0 and covers the whole * coherent_dma_mask. */ - arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu, + arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu, attr == DEV_DMA_COHERENT); } EXPORT_SYMBOL_GPL(acpi_dma_configure); diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 9afcbf7..0995ab3 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev) return -ENOMEM; /* Set dma_ops for dev, otherwise it would be dummy_dma_ops */ - arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false); + arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false); dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index c9b7ad6..19f70d8 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev) priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */ /* device used for DMA mapping */ - arch_setup_dma_ops(dev, 0, 0, NULL, false); + arch_setup_dma_ops(dev, 0, 0, false, NULL, false); err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40)); if (err) { dev_err(dev, "dma_coerce_mask_and_coherent() failed\n"); diff --git a/drivers/of/device.c b/drivers/of/device.c index fd5cfad..1cc2115 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) bool coherent; unsigned long offset; const struct iommu_ops *iommu; + bool enforce_range = false; /* * Set default coherent_dma_mask to 32 bit. Drivers are expected to @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) return; } dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); + + enforce_range = true; } dev->dma_pfn_offset = offset; @@ -147,7 +150,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) dev_dbg(dev, "device is%sbehind an iommu\n", iommu ? " " : " not "); - arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); + arch_setup_dma_ops(dev, dma_addr, size, enforce_range, iommu, coherent); } EXPORT_SYMBOL_GPL(of_dma_configure); diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c index 5ac373c..480b644 100644 --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, /* Objects are coherent, unless 'no shareability' flag set. */ if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); /* * The device-specific probe callback will get invoked by device_add() -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-11 18:31 ` Nikita Yushchenko @ 2017-01-11 21:08 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-11 21:08 UTC (permalink / raw) To: Nikita Yushchenko Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov On Wednesday, January 11, 2017 9:31:51 PM CET Nikita Yushchenko wrote: > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 9afcbf7..0995ab3 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev) > return -ENOMEM; > > /* Set dma_ops for dev, otherwise it would be dummy_dma_ops */ > - arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false); > + arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false); > > dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index c9b7ad6..19f70d8 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev) > priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */ > > /* device used for DMA mapping */ > - arch_setup_dma_ops(dev, 0, 0, NULL, false); > + arch_setup_dma_ops(dev, 0, 0, false, NULL, false); > err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40)); > if (err) { > dev_err(dev, "dma_coerce_mask_and_coherent() failed\n"); > diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > index 5ac373c..480b644 100644 > --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, > > /* Objects are coherent, unless 'no shareability' flag set. */ > if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) > - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); > + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); > > /* > * The device-specific probe callback will get invoked by device_add() Why are these actually calling arch_setup_dma_ops() here in the first place? Are these all devices that are DMA masters without an OF node? > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad..1cc2115 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > + bool enforce_range = false; > > /* > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) > return; > } > dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); > + > + enforce_range = true; > } > > dev->dma_pfn_offset = offset; Hmm, I think when the dma-ranges are missing, we should either enforce a 32-bit mask, or disallow DMA completely. It's probably too late for the latter, I wish we had done this earlier in order to force everyone on ARM64 to have a valid dma-ranges property for any DMA master. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-11 21:08 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-11 21:08 UTC (permalink / raw) To: linux-arm-kernel On Wednesday, January 11, 2017 9:31:51 PM CET Nikita Yushchenko wrote: > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 9afcbf7..0995ab3 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev) > return -ENOMEM; > > /* Set dma_ops for dev, otherwise it would be dummy_dma_ops */ > - arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false); > + arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false); > > dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index c9b7ad6..19f70d8 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev) > priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */ > > /* device used for DMA mapping */ > - arch_setup_dma_ops(dev, 0, 0, NULL, false); > + arch_setup_dma_ops(dev, 0, 0, false, NULL, false); > err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40)); > if (err) { > dev_err(dev, "dma_coerce_mask_and_coherent() failed\n"); > diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > index 5ac373c..480b644 100644 > --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, > > /* Objects are coherent, unless 'no shareability' flag set. */ > if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) > - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); > + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); > > /* > * The device-specific probe callback will get invoked by device_add() Why are these actually calling arch_setup_dma_ops() here in the first place? Are these all devices that are DMA masters without an OF node? > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad..1cc2115 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > + bool enforce_range = false; > > /* > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) > return; > } > dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); > + > + enforce_range = true; > } > > dev->dma_pfn_offset = offset; Hmm, I think when the dma-ranges are missing, we should either enforce a 32-bit mask, or disallow DMA completely. It's probably too late for the latter, I wish we had done this earlier in order to force everyone on ARM64 to have a valid dma-ranges property for any DMA master. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-11 21:08 ` Arnd Bergmann @ 2017-01-12 5:52 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 5:52 UTC (permalink / raw) To: Arnd Bergmann Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> index 5ac373c..480b644 100644 >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >> >> /* Objects are coherent, unless 'no shareability' flag set. */ >> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) >> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); >> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); >> >> /* >> * The device-specific probe callback will get invoked by device_add() > > Why are these actually calling arch_setup_dma_ops() here in the first > place? Are these all devices that are DMA masters without an OF node? I don't know, but that's a different topic. This patch just adds argument and sets it to false everywhere but in the location when range should be definitely enforced. >> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) >> return; >> } >> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); >> + >> + enforce_range = true; >> } >> >> dev->dma_pfn_offset = offset; > > Hmm, I think when the dma-ranges are missing, we should either enforce > a 32-bit mask, or disallow DMA completely. It's probably too late for > the latter, I wish we had done this earlier in order to force everyone > on ARM64 to have a valid dma-ranges property for any DMA master. This can be done over time. However the very idea of this version of patch is - keep working pieces as-is, thus for now setting enforce_range to false in case of no defined dma-ranges is intentional. What I should re-check is - does rcar dtsi set dma-ranges, and add it if it does not. Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 5:52 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 5:52 UTC (permalink / raw) To: linux-arm-kernel >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> index 5ac373c..480b644 100644 >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >> >> /* Objects are coherent, unless 'no shareability' flag set. */ >> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) >> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); >> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); >> >> /* >> * The device-specific probe callback will get invoked by device_add() > > Why are these actually calling arch_setup_dma_ops() here in the first > place? Are these all devices that are DMA masters without an OF node? I don't know, but that's a different topic. This patch just adds argument and sets it to false everywhere but in the location when range should be definitely enforced. >> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) >> return; >> } >> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); >> + >> + enforce_range = true; >> } >> >> dev->dma_pfn_offset = offset; > > Hmm, I think when the dma-ranges are missing, we should either enforce > a 32-bit mask, or disallow DMA completely. It's probably too late for > the latter, I wish we had done this earlier in order to force everyone > on ARM64 to have a valid dma-ranges property for any DMA master. This can be done over time. However the very idea of this version of patch is - keep working pieces as-is, thus for now setting enforce_range to false in case of no defined dma-ranges is intentional. What I should re-check is - does rcar dtsi set dma-ranges, and add it if it does not. Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-12 5:52 ` Nikita Yushchenko @ 2017-01-12 6:33 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 6:33 UTC (permalink / raw) To: Arnd Bergmann Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov 12.01.2017 08:52, Nikita Yushchenko wrote: >>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>> index 5ac373c..480b644 100644 >>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >>> >>> /* Objects are coherent, unless 'no shareability' flag set. */ >>> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) >>> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); >>> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); >>> >>> /* >>> * The device-specific probe callback will get invoked by device_add() >> >> Why are these actually calling arch_setup_dma_ops() here in the first >> place? Are these all devices that are DMA masters without an OF node? > > I don't know, but that's a different topic. This patch just adds > argument and sets it to false everywhere but in the location when range > should be definitely enforced. > >>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) >>> return; >>> } >>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); >>> + >>> + enforce_range = true; >>> } >>> >>> dev->dma_pfn_offset = offset; >> >> Hmm, I think when the dma-ranges are missing, we should either enforce >> a 32-bit mask, or disallow DMA completely. It's probably too late for >> the latter, I wish we had done this earlier in order to force everyone >> on ARM64 to have a valid dma-ranges property for any DMA master. > > This can be done over time. > > However the very idea of this version of patch is - keep working pieces > as-is, thus for now setting enforce_range to false in case of no defined > dma-ranges is intentional. What we can do is - check bus width (as it is defined in DT) and set enforce_range to true if bus is 32-bit > What I should re-check is - does rcar dtsi set dma-ranges, and add it if > it does not. It does not, will have to add. In DT bus is defined as 64-bit. But looks like physically it is 32-bit. Maybe DT needs fixing. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 6:33 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 6:33 UTC (permalink / raw) To: linux-arm-kernel 12.01.2017 08:52, Nikita Yushchenko wrote: >>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>> index 5ac373c..480b644 100644 >>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >>> >>> /* Objects are coherent, unless 'no shareability' flag set. */ >>> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) >>> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); >>> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); >>> >>> /* >>> * The device-specific probe callback will get invoked by device_add() >> >> Why are these actually calling arch_setup_dma_ops() here in the first >> place? Are these all devices that are DMA masters without an OF node? > > I don't know, but that's a different topic. This patch just adds > argument and sets it to false everywhere but in the location when range > should be definitely enforced. > >>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np) >>> return; >>> } >>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); >>> + >>> + enforce_range = true; >>> } >>> >>> dev->dma_pfn_offset = offset; >> >> Hmm, I think when the dma-ranges are missing, we should either enforce >> a 32-bit mask, or disallow DMA completely. It's probably too late for >> the latter, I wish we had done this earlier in order to force everyone >> on ARM64 to have a valid dma-ranges property for any DMA master. > > This can be done over time. > > However the very idea of this version of patch is - keep working pieces > as-is, thus for now setting enforce_range to false in case of no defined > dma-ranges is intentional. What we can do is - check bus width (as it is defined in DT) and set enforce_range to true if bus is 32-bit > What I should re-check is - does rcar dtsi set dma-ranges, and add it if > it does not. It does not, will have to add. In DT bus is defined as 64-bit. But looks like physically it is 32-bit. Maybe DT needs fixing. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-12 6:33 ` Nikita Yushchenko @ 2017-01-12 13:28 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-12 13:28 UTC (permalink / raw) To: Nikita Yushchenko Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov On Thursday, January 12, 2017 9:33:32 AM CET Nikita Yushchenko wrote: > >> Hmm, I think when the dma-ranges are missing, we should either enforce > >> a 32-bit mask, or disallow DMA completely. It's probably too late for > >> the latter, I wish we had done this earlier in order to force everyone > >> on ARM64 to have a valid dma-ranges property for any DMA master. > > > > This can be done over time. > > > > However the very idea of this version of patch is - keep working pieces > > as-is, thus for now setting enforce_range to false in case of no defined > > dma-ranges is intentional. > > What we can do is - check bus width (as it is defined in DT) and set > enforce_range to true if bus is 32-bit > > > What I should re-check is - does rcar dtsi set dma-ranges, and add it if > > it does not. > > It does not, will have to add. > > In DT bus is defined as 64-bit. But looks like physically it is 32-bit. > Maybe DT needs fixing. I think we always assumed that the lack of a dma-ranges property implied a 32-bit width, as that is the safe fallback as well as the most common case. AFAICT, this means you are actually fine on rcar, and all other platforms will keep working as we enforce it, but might get slowed down if they relied on the unintended behavior of allowing 64-bit DMA. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 13:28 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-12 13:28 UTC (permalink / raw) To: linux-arm-kernel On Thursday, January 12, 2017 9:33:32 AM CET Nikita Yushchenko wrote: > >> Hmm, I think when the dma-ranges are missing, we should either enforce > >> a 32-bit mask, or disallow DMA completely. It's probably too late for > >> the latter, I wish we had done this earlier in order to force everyone > >> on ARM64 to have a valid dma-ranges property for any DMA master. > > > > This can be done over time. > > > > However the very idea of this version of patch is - keep working pieces > > as-is, thus for now setting enforce_range to false in case of no defined > > dma-ranges is intentional. > > What we can do is - check bus width (as it is defined in DT) and set > enforce_range to true if bus is 32-bit > > > What I should re-check is - does rcar dtsi set dma-ranges, and add it if > > it does not. > > It does not, will have to add. > > In DT bus is defined as 64-bit. But looks like physically it is 32-bit. > Maybe DT needs fixing. I think we always assumed that the lack of a dma-ranges property implied a 32-bit width, as that is the safe fallback as well as the most common case. AFAICT, this means you are actually fine on rcar, and all other platforms will keep working as we enforce it, but might get slowed down if they relied on the unintended behavior of allowing 64-bit DMA. Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-12 13:28 ` Arnd Bergmann @ 2017-01-12 13:39 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 13:39 UTC (permalink / raw) To: Arnd Bergmann Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov >>>> Hmm, I think when the dma-ranges are missing, we should either enforce >>>> a 32-bit mask, or disallow DMA completely. It's probably too late for >>>> the latter, I wish we had done this earlier in order to force everyone >>>> on ARM64 to have a valid dma-ranges property for any DMA master. >>> >>> This can be done over time. >>> >>> However the very idea of this version of patch is - keep working pieces >>> as-is, thus for now setting enforce_range to false in case of no defined >>> dma-ranges is intentional. >> >> What we can do is - check bus width (as it is defined in DT) and set >> enforce_range to true if bus is 32-bit >> >>> What I should re-check is - does rcar dtsi set dma-ranges, and add it if >>> it does not. >> >> It does not, will have to add. >> >> In DT bus is defined as 64-bit. But looks like physically it is 32-bit. >> Maybe DT needs fixing. > > I think we always assumed that the lack of a dma-ranges property > implied a 32-bit width, as that is the safe fallback as well as the > most common case. Yes we assumed that, but that was combined with blindly accepting wider dma_mask per driver's request. Later is being changed. > AFAICT, this means you are actually fine on rcar, and all other > platforms will keep working as we enforce it, but might get slowed > down if they relied on the unintended behavior of allowing 64-bit > DMA. Yesterday Robin raised issue that a change starting to enforce default dma_mask will break existing setups - i.e. those that depend in 64bit DMA being implicitly supported without manually declaring such support. In reply to that, I suggested this version of patchset that should keep existing behavior by default. I'm fine with both approaches regarding behavior on hw that I don't have - but I'm not in position to make any decisions on that. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 13:39 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 13:39 UTC (permalink / raw) To: linux-arm-kernel >>>> Hmm, I think when the dma-ranges are missing, we should either enforce >>>> a 32-bit mask, or disallow DMA completely. It's probably too late for >>>> the latter, I wish we had done this earlier in order to force everyone >>>> on ARM64 to have a valid dma-ranges property for any DMA master. >>> >>> This can be done over time. >>> >>> However the very idea of this version of patch is - keep working pieces >>> as-is, thus for now setting enforce_range to false in case of no defined >>> dma-ranges is intentional. >> >> What we can do is - check bus width (as it is defined in DT) and set >> enforce_range to true if bus is 32-bit >> >>> What I should re-check is - does rcar dtsi set dma-ranges, and add it if >>> it does not. >> >> It does not, will have to add. >> >> In DT bus is defined as 64-bit. But looks like physically it is 32-bit. >> Maybe DT needs fixing. > > I think we always assumed that the lack of a dma-ranges property > implied a 32-bit width, as that is the safe fallback as well as the > most common case. Yes we assumed that, but that was combined with blindly accepting wider dma_mask per driver's request. Later is being changed. > AFAICT, this means you are actually fine on rcar, and all other > platforms will keep working as we enforce it, but might get slowed > down if they relied on the unintended behavior of allowing 64-bit > DMA. Yesterday Robin raised issue that a change starting to enforce default dma_mask will break existing setups - i.e. those that depend in 64bit DMA being implicitly supported without manually declaring such support. In reply to that, I suggested this version of patchset that should keep existing behavior by default. I'm fine with both approaches regarding behavior on hw that I don't have - but I'm not in position to make any decisions on that. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-12 5:52 ` Nikita Yushchenko @ 2017-01-12 12:16 ` Will Deacon -1 siblings, 0 replies; 75+ messages in thread From: Will Deacon @ 2017-01-12 12:16 UTC (permalink / raw) To: Nikita Yushchenko Cc: Arnd Bergmann, Robin Murphy, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote: > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > >> index 5ac373c..480b644 100644 > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, > >> > >> /* Objects are coherent, unless 'no shareability' flag set. */ > >> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) > >> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); > >> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); > >> > >> /* > >> * The device-specific probe callback will get invoked by device_add() > > > > Why are these actually calling arch_setup_dma_ops() here in the first > > place? Are these all devices that are DMA masters without an OF node? > > I don't know, but that's a different topic. This patch just adds > argument and sets it to false everywhere but in the location when range > should be definitely enforced. I also wouldn't lose any sleep over a staging driver. Will ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 12:16 ` Will Deacon 0 siblings, 0 replies; 75+ messages in thread From: Will Deacon @ 2017-01-12 12:16 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote: > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > >> index 5ac373c..480b644 100644 > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, > >> > >> /* Objects are coherent, unless 'no shareability' flag set. */ > >> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) > >> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); > >> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); > >> > >> /* > >> * The device-specific probe callback will get invoked by device_add() > > > > Why are these actually calling arch_setup_dma_ops() here in the first > > place? Are these all devices that are DMA masters without an OF node? > > I don't know, but that's a different topic. This patch just adds > argument and sets it to false everywhere but in the location when range > should be definitely enforced. I also wouldn't lose any sleep over a staging driver. Will ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-12 12:16 ` Will Deacon @ 2017-01-12 13:25 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-12 13:25 UTC (permalink / raw) To: Will Deacon Cc: Nikita Yushchenko, Robin Murphy, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote: > On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote: > > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > > >> index 5ac373c..480b644 100644 > > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, > > >> > > >> /* Objects are coherent, unless 'no shareability' flag set. */ > > >> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) > > >> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); > > >> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); > > >> > > >> /* > > >> * The device-specific probe callback will get invoked by device_add() > > > > > > Why are these actually calling arch_setup_dma_ops() here in the first > > > place? Are these all devices that are DMA masters without an OF node? > > > > I don't know, but that's a different topic. This patch just adds > > argument and sets it to false everywhere but in the location when range > > should be definitely enforced. > > I also wouldn't lose any sleep over a staging driver. I think this is in the process of being moved out of staging, and my question was about the other two as well: drivers/net/ethernet/freescale/dpaa/dpaa_eth.c drivers/iommu/rockchip-iommu.c Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 13:25 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-12 13:25 UTC (permalink / raw) To: linux-arm-kernel On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote: > On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote: > > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > > >> index 5ac373c..480b644 100644 > > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c > > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, > > >> > > >> /* Objects are coherent, unless 'no shareability' flag set. */ > > >> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) > > >> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); > > >> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); > > >> > > >> /* > > >> * The device-specific probe callback will get invoked by device_add() > > > > > > Why are these actually calling arch_setup_dma_ops() here in the first > > > place? Are these all devices that are DMA masters without an OF node? > > > > I don't know, but that's a different topic. This patch just adds > > argument and sets it to false everywhere but in the location when range > > should be definitely enforced. > > I also wouldn't lose any sleep over a staging driver. I think this is in the process of being moved out of staging, and my question was about the other two as well: drivers/net/ethernet/freescale/dpaa/dpaa_eth.c drivers/iommu/rockchip-iommu.c Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-12 13:25 ` Arnd Bergmann @ 2017-01-12 13:43 ` Robin Murphy -1 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-12 13:43 UTC (permalink / raw) To: Arnd Bergmann Cc: Will Deacon, Nikita Yushchenko, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov On 12/01/17 13:25, Arnd Bergmann wrote: > On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote: >> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote: >>>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>>>> index 5ac373c..480b644 100644 >>>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >>>>> >>>>> /* Objects are coherent, unless 'no shareability' flag set. */ >>>>> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) >>>>> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); >>>>> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); >>>>> >>>>> /* >>>>> * The device-specific probe callback will get invoked by device_add() >>>> >>>> Why are these actually calling arch_setup_dma_ops() here in the first >>>> place? Are these all devices that are DMA masters without an OF node? >>> >>> I don't know, but that's a different topic. This patch just adds >>> argument and sets it to false everywhere but in the location when range >>> should be definitely enforced. >> >> I also wouldn't lose any sleep over a staging driver. > > I think this is in the process of being moved out of staging, and > my question was about the other two as well: The fsl-mc is actually a sort-of-bus-controller probing and configuring its (directly DMA-capable) child devices, so is in fact legitimate here. > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c That one is completely bogus, and should just go away. > drivers/iommu/rockchip-iommu.c That one is part of some ugly trickery involving creating a fake device to represent multiple separate IOMMU devices. The driver could probably be reworked to not need it (the Exynos IOMMU handles a similar situation without such tricks), but it's non-trivial. Robin. > > Arnd > ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-12 13:43 ` Robin Murphy 0 siblings, 0 replies; 75+ messages in thread From: Robin Murphy @ 2017-01-12 13:43 UTC (permalink / raw) To: linux-arm-kernel On 12/01/17 13:25, Arnd Bergmann wrote: > On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote: >> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote: >>>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>>>> index 5ac373c..480b644 100644 >>>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >>>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >>>>> >>>>> /* Objects are coherent, unless 'no shareability' flag set. */ >>>>> if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY)) >>>>> - arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true); >>>>> + arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true); >>>>> >>>>> /* >>>>> * The device-specific probe callback will get invoked by device_add() >>>> >>>> Why are these actually calling arch_setup_dma_ops() here in the first >>>> place? Are these all devices that are DMA masters without an OF node? >>> >>> I don't know, but that's a different topic. This patch just adds >>> argument and sets it to false everywhere but in the location when range >>> should be definitely enforced. >> >> I also wouldn't lose any sleep over a staging driver. > > I think this is in the process of being moved out of staging, and > my question was about the other two as well: The fsl-mc is actually a sort-of-bus-controller probing and configuring its (directly DMA-capable) child devices, so is in fact legitimate here. > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c That one is completely bogus, and should just go away. > drivers/iommu/rockchip-iommu.c That one is part of some ugly trickery involving creating a fake device to represent multiple separate IOMMU devices. The driver could probably be reworked to not need it (the Exynos IOMMU handles a similar situation without such tricks), but it's non-trivial. Robin. > > Arnd > ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() 2017-01-11 18:31 ` Nikita Yushchenko @ 2017-01-13 10:40 ` kbuild test robot -1 siblings, 0 replies; 75+ messages in thread From: kbuild test robot @ 2017-01-13 10:40 UTC (permalink / raw) To: Nikita Yushchenko Cc: kbuild-all, Robin Murphy, Will Deacon, Arnd Bergmann, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov, Nikita Yushchenko [-- Attachment #1: Type: text/plain, Size: 1671 bytes --] Hi Nikita, [auto build test ERROR on linus/master] [also build test ERROR on v4.10-rc3 next-20170112] [cannot apply to arm64/for-next/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733 config: x86_64-randconfig-u0-01131618 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/acpi/scan.c: In function 'acpi_dma_configure': >> drivers/acpi/scan.c:1388:2: error: too many arguments to function 'arch_setup_dma_ops' arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu, ^~~~~~~~~~~~~~~~~~ In file included from drivers/acpi/scan.c:15:0: include/linux/dma-mapping.h:611:20: note: declared here static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, ^~~~~~~~~~~~~~~~~~ vim +/arch_setup_dma_ops +1388 drivers/acpi/scan.c 1382 iommu = iort_iommu_configure(dev); 1383 1384 /* 1385 * Assume dma valid range starts at 0 and covers the whole 1386 * coherent_dma_mask. 1387 */ > 1388 arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu, 1389 attr == DEV_DMA_COHERENT); 1390 } 1391 EXPORT_SYMBOL_GPL(acpi_dma_configure); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 25665 bytes --] ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() @ 2017-01-13 10:40 ` kbuild test robot 0 siblings, 0 replies; 75+ messages in thread From: kbuild test robot @ 2017-01-13 10:40 UTC (permalink / raw) To: linux-arm-kernel Hi Nikita, [auto build test ERROR on linus/master] [also build test ERROR on v4.10-rc3 next-20170112] [cannot apply to arm64/for-next/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733 config: x86_64-randconfig-u0-01131618 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/acpi/scan.c: In function 'acpi_dma_configure': >> drivers/acpi/scan.c:1388:2: error: too many arguments to function 'arch_setup_dma_ops' arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu, ^~~~~~~~~~~~~~~~~~ In file included from drivers/acpi/scan.c:15:0: include/linux/dma-mapping.h:611:20: note: declared here static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, ^~~~~~~~~~~~~~~~~~ vim +/arch_setup_dma_ops +1388 drivers/acpi/scan.c 1382 iommu = iort_iommu_configure(dev); 1383 1384 /* 1385 * Assume dma valid range starts at 0 and covers the whole 1386 * coherent_dma_mask. 1387 */ > 1388 arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu, 1389 attr == DEV_DMA_COHERENT); 1390 } 1391 EXPORT_SYMBOL_GPL(acpi_dma_configure); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/gzip Size: 25665 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170113/7e91dc43/attachment-0001.gz> ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 18:31 ` Nikita Yushchenko @ 2017-01-11 18:31 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 18:31 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Arnd Bergmann Cc: linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov, Nikita Yushchenko There are cases when device supports wide DMA addresses wider than device's connection supports. In this case driver sets DMA mask based on knowledge of device capabilities. That must succeed to allow drivers to initialize. However, swiotlb or iommu still need knowledge about actual device capabilities. To avoid breakage, actual mask must not be set wider than device connection allows. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> CC: Arnd Bergmann <arnd@arndb.de> CC: Robin Murphy <robin.murphy@arm.com> CC: Will Deacon <will.deacon@arm.com> --- arch/arm64/Kconfig | 3 +++ arch/arm64/include/asm/device.h | 1 + arch/arm64/include/asm/dma-mapping.h | 3 +++ arch/arm64/mm/dma-mapping.c | 40 ++++++++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..afb2c08 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE config NEED_SG_DMA_LENGTH def_bool y +config ARCH_HAS_DMA_SET_COHERENT_MASK + def_bool y + config SMP def_bool y diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h index 243ef25..a57e7bb 100644 --- a/arch/arm64/include/asm/device.h +++ b/arch/arm64/include/asm/device.h @@ -22,6 +22,7 @@ struct dev_archdata { void *iommu; /* private IOMMU data */ #endif bool dma_coherent; + u64 parent_dma_mask; }; struct pdev_archdata { diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ae1c23f..46b53e6 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -52,6 +52,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, bool coherent); #define arch_setup_dma_ops arch_setup_dma_ops +#define HAVE_ARCH_DMA_SET_MASK 1 +extern int dma_set_mask(struct device *dev, u64 dma_mask); + #ifdef CONFIG_IOMMU_DMA void arch_teardown_dma_ops(struct device *dev); #define arch_teardown_dma_ops arch_teardown_dma_ops diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index ea295f1..31b96fd 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size, __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); } +int dma_set_mask(struct device *dev, u64 dma_mask) +{ + const struct dma_map_ops *ops = get_dma_ops(dev); + + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (ops->set_dma_mask) + return ops->set_dma_mask(dev, mask); + + if (!dev->dma_mask || !dma_supported(dev, mask)) + return -EIO; + + *dev->dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_mask); + +int dma_set_coherent_mask(struct device *dev, u64 mask) +{ + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (!dma_supported(dev, mask)) + return -EIO; + + dev->coherent_dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_coherent_mask); + static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page, unsigned long offset, size_t size, enum dma_data_direction dir, @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; + /* + * Whatever the parent bus can set. A device must not set + * a DMA mask larger than this. + */ + if (enforce_range) + dev->archdata.parent_dma_mask = size - 1; + else + dev->archdata.parent_dma_mask = DMA_BIT_MASK(64); + dev->archdata.dma_coherent = coherent; __iommu_setup_dma_ops(dev, dma_base, size, iommu); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 18:31 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-11 18:31 UTC (permalink / raw) To: linux-arm-kernel There are cases when device supports wide DMA addresses wider than device's connection supports. In this case driver sets DMA mask based on knowledge of device capabilities. That must succeed to allow drivers to initialize. However, swiotlb or iommu still need knowledge about actual device capabilities. To avoid breakage, actual mask must not be set wider than device connection allows. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> CC: Arnd Bergmann <arnd@arndb.de> CC: Robin Murphy <robin.murphy@arm.com> CC: Will Deacon <will.deacon@arm.com> --- arch/arm64/Kconfig | 3 +++ arch/arm64/include/asm/device.h | 1 + arch/arm64/include/asm/dma-mapping.h | 3 +++ arch/arm64/mm/dma-mapping.c | 40 ++++++++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..afb2c08 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE config NEED_SG_DMA_LENGTH def_bool y +config ARCH_HAS_DMA_SET_COHERENT_MASK + def_bool y + config SMP def_bool y diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h index 243ef25..a57e7bb 100644 --- a/arch/arm64/include/asm/device.h +++ b/arch/arm64/include/asm/device.h @@ -22,6 +22,7 @@ struct dev_archdata { void *iommu; /* private IOMMU data */ #endif bool dma_coherent; + u64 parent_dma_mask; }; struct pdev_archdata { diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ae1c23f..46b53e6 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -52,6 +52,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, bool coherent); #define arch_setup_dma_ops arch_setup_dma_ops +#define HAVE_ARCH_DMA_SET_MASK 1 +extern int dma_set_mask(struct device *dev, u64 dma_mask); + #ifdef CONFIG_IOMMU_DMA void arch_teardown_dma_ops(struct device *dev); #define arch_teardown_dma_ops arch_teardown_dma_ops diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index ea295f1..31b96fd 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size, __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); } +int dma_set_mask(struct device *dev, u64 dma_mask) +{ + const struct dma_map_ops *ops = get_dma_ops(dev); + + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (ops->set_dma_mask) + return ops->set_dma_mask(dev, mask); + + if (!dev->dma_mask || !dma_supported(dev, mask)) + return -EIO; + + *dev->dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_mask); + +int dma_set_coherent_mask(struct device *dev, u64 mask) +{ + if (mask > dev->archdata.parent_dma_mask) + mask = dev->archdata.parent_dma_mask; + + if (!dma_supported(dev, mask)) + return -EIO; + + dev->coherent_dma_mask = mask; + return 0; +} +EXPORT_SYMBOL(dma_set_coherent_mask); + static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page, unsigned long offset, size_t size, enum dma_data_direction dir, @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!dev->archdata.dma_ops) dev->archdata.dma_ops = &swiotlb_dma_ops; + /* + * Whatever the parent bus can set. A device must not set + * a DMA mask larger than this. + */ + if (enforce_range) + dev->archdata.parent_dma_mask = size - 1; + else + dev->archdata.parent_dma_mask = DMA_BIT_MASK(64); + dev->archdata.dma_coherent = coherent; __iommu_setup_dma_ops(dev, dma_base, size, iommu); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 18:31 ` Nikita Yushchenko @ 2017-01-11 21:11 ` Arnd Bergmann -1 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-11 21:11 UTC (permalink / raw) To: Nikita Yushchenko Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov On Wednesday, January 11, 2017 9:31:52 PM CET Nikita Yushchenko wrote: > @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > if (!dev->archdata.dma_ops) > dev->archdata.dma_ops = &swiotlb_dma_ops; > > + /* > + * Whatever the parent bus can set. A device must not set > + * a DMA mask larger than this. > + */ > + if (enforce_range) > + dev->archdata.parent_dma_mask = size - 1; > + else > + dev->archdata.parent_dma_mask = DMA_BIT_MASK(64); > + > dev->archdata.dma_coherent = coherent; > __iommu_setup_dma_ops(dev, dma_base, size, iommu); > Could we just pass the mask instead of the size here? Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-11 21:11 ` Arnd Bergmann 0 siblings, 0 replies; 75+ messages in thread From: Arnd Bergmann @ 2017-01-11 21:11 UTC (permalink / raw) To: linux-arm-kernel On Wednesday, January 11, 2017 9:31:52 PM CET Nikita Yushchenko wrote: > @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > if (!dev->archdata.dma_ops) > dev->archdata.dma_ops = &swiotlb_dma_ops; > > + /* > + * Whatever the parent bus can set. A device must not set > + * a DMA mask larger than this. > + */ > + if (enforce_range) > + dev->archdata.parent_dma_mask = size - 1; > + else > + dev->archdata.parent_dma_mask = DMA_BIT_MASK(64); > + > dev->archdata.dma_coherent = coherent; > __iommu_setup_dma_ops(dev, dma_base, size, iommu); > Could we just pass the mask instead of the size here? Arnd ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 21:11 ` Arnd Bergmann @ 2017-01-12 5:53 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 5:53 UTC (permalink / raw) To: Arnd Bergmann Cc: Robin Murphy, Will Deacon, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov >> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, >> if (!dev->archdata.dma_ops) >> dev->archdata.dma_ops = &swiotlb_dma_ops; >> >> + /* >> + * Whatever the parent bus can set. A device must not set >> + * a DMA mask larger than this. >> + */ >> + if (enforce_range) >> + dev->archdata.parent_dma_mask = size - 1; >> + else >> + dev->archdata.parent_dma_mask = DMA_BIT_MASK(64); >> + >> dev->archdata.dma_coherent = coherent; >> __iommu_setup_dma_ops(dev, dma_base, size, iommu); >> > > Could we just pass the mask instead of the size here? We don't want to change API now. Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-12 5:53 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-12 5:53 UTC (permalink / raw) To: linux-arm-kernel >> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, >> if (!dev->archdata.dma_ops) >> dev->archdata.dma_ops = &swiotlb_dma_ops; >> >> + /* >> + * Whatever the parent bus can set. A device must not set >> + * a DMA mask larger than this. >> + */ >> + if (enforce_range) >> + dev->archdata.parent_dma_mask = size - 1; >> + else >> + dev->archdata.parent_dma_mask = DMA_BIT_MASK(64); >> + >> dev->archdata.dma_coherent = coherent; >> __iommu_setup_dma_ops(dev, dma_base, size, iommu); >> > > Could we just pass the mask instead of the size here? We don't want to change API now. Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports 2017-01-11 18:31 ` Nikita Yushchenko @ 2017-01-13 10:16 ` kbuild test robot -1 siblings, 0 replies; 75+ messages in thread From: kbuild test robot @ 2017-01-13 10:16 UTC (permalink / raw) To: Nikita Yushchenko Cc: kbuild-all, Robin Murphy, Will Deacon, Arnd Bergmann, linux-arm-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, fkan, linux-kernel, Artemi Ivanov, Nikita Yushchenko [-- Attachment #1: Type: text/plain, Size: 1630 bytes --] Hi Nikita, [auto build test ERROR on linus/master] [also build test ERROR on v4.10-rc3 next-20170112] [cannot apply to arm64/for-next/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733 config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64 All errors (new ones prefixed by >>): arch/arm64/mm/dma-mapping.c: In function 'dma_set_mask': >> arch/arm64/mm/dma-mapping.c:210:6: error: 'mask' undeclared (first use in this function) if (mask > dev->archdata.parent_dma_mask) ^~~~ arch/arm64/mm/dma-mapping.c:210:6: note: each undeclared identifier is reported only once for each function it appears in vim +/mask +210 arch/arm64/mm/dma-mapping.c 204 } 205 206 int dma_set_mask(struct device *dev, u64 dma_mask) 207 { 208 const struct dma_map_ops *ops = get_dma_ops(dev); 209 > 210 if (mask > dev->archdata.parent_dma_mask) 211 mask = dev->archdata.parent_dma_mask; 212 213 if (ops->set_dma_mask) --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 33713 bytes --] ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports @ 2017-01-13 10:16 ` kbuild test robot 0 siblings, 0 replies; 75+ messages in thread From: kbuild test robot @ 2017-01-13 10:16 UTC (permalink / raw) To: linux-arm-kernel Hi Nikita, [auto build test ERROR on linus/master] [also build test ERROR on v4.10-rc3 next-20170112] [cannot apply to arm64/for-next/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733 config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64 All errors (new ones prefixed by >>): arch/arm64/mm/dma-mapping.c: In function 'dma_set_mask': >> arch/arm64/mm/dma-mapping.c:210:6: error: 'mask' undeclared (first use in this function) if (mask > dev->archdata.parent_dma_mask) ^~~~ arch/arm64/mm/dma-mapping.c:210:6: note: each undeclared identifier is reported only once for each function it appears in vim +/mask +210 arch/arm64/mm/dma-mapping.c 204 } 205 206 int dma_set_mask(struct device *dev, u64 dma_mask) 207 { 208 const struct dma_map_ops *ops = get_dma_ops(dev); 209 > 210 if (mask > dev->archdata.parent_dma_mask) 211 mask = dev->archdata.parent_dma_mask; 212 213 if (ops->set_dma_mask) --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/gzip Size: 33713 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170113/4aebcaaa/attachment-0001.gz> ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 13:25 ` Robin Murphy @ 2017-01-10 14:01 ` Nikita Yushchenko -1 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-10 14:01 UTC (permalink / raw) To: Robin Murphy, Will Deacon Cc: Arnd Bergmann, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan >> What issue "IOMMU doesn't solve"? >> >> Issue I'm trying to address is - inconsistency within swiotlb >> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then >> mask is used to decide if bounce buffers are needed or not. This >> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory >> instead). > > The fundamental underlying problem is the "any wide mask is silently > accepted" part, and that applies equally to IOMMU ops as well. Is just posted version better? It should cover iommu case as well. Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 14:01 ` Nikita Yushchenko 0 siblings, 0 replies; 75+ messages in thread From: Nikita Yushchenko @ 2017-01-10 14:01 UTC (permalink / raw) To: linux-arm-kernel >> What issue "IOMMU doesn't solve"? >> >> Issue I'm trying to address is - inconsistency within swiotlb >> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then >> mask is used to decide if bounce buffers are needed or not. This >> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory >> instead). > > The fundamental underlying problem is the "any wide mask is silently > accepted" part, and that applies equally to IOMMU ops as well. Is just posted version better? It should cover iommu case as well. Nikita ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 13:25 ` Robin Murphy @ 2017-01-10 14:57 ` Christoph Hellwig -1 siblings, 0 replies; 75+ messages in thread From: Christoph Hellwig @ 2017-01-10 14:57 UTC (permalink / raw) To: Robin Murphy Cc: Nikita Yushchenko, Will Deacon, Arnd Bergmann, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, fkan, Christoph Hellwig On Tue, Jan 10, 2017 at 01:25:12PM +0000, Robin Murphy wrote: > We still need a way for drivers to communicate a device's probed > addressing capability to SWIOTLB, so there's always going to have to be > *some* sort of public interface. Personally, the change in semantics I'd > like to see is to make dma_set_mask() only fail if DMA is entirely > disallowed - in the normal case it would always succeed, but the DMA API > implementation would be permitted to set a smaller mask than requested > (this is effectively what the x86 IOMMU ops do already). Yes, this sounds reasonable. > The significant > work that would require, though, is changing all the drivers currently > using this sort of pattern: > > if (!dma_set_mask(dev, DMA_BIT_MASK(64)) > /* put device into 64-bit mode */ > else if (!dma_set_mask(dev, DMA_BIT_MASK(32)) > /* put device into 32-bit mode */ > else > /* error */ While we have this pattern in a lot of places it's already rather pointless on most architectures as the first dma_set_mask call won't ever fail for the most common dma_ops implementations. > to something like this: > > if (!dma_set_mask(dev, DMA_BIT_MASK(64)) > /* error */ > if (dma_get_mask(dev) > DMA_BIT_MASK(32)) > /* put device into 64-bit mode */ > else > /* put device into 32-bit mode */ > > Which would be a pretty major job. I don't think it's too bad. Also for many modern devices there is no need to put the device into a specific mode. It's mostly a historic issue from the PCI/PCI-X days with the less efficient DAC addressing scheme. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 14:57 ` Christoph Hellwig 0 siblings, 0 replies; 75+ messages in thread From: Christoph Hellwig @ 2017-01-10 14:57 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 10, 2017 at 01:25:12PM +0000, Robin Murphy wrote: > We still need a way for drivers to communicate a device's probed > addressing capability to SWIOTLB, so there's always going to have to be > *some* sort of public interface. Personally, the change in semantics I'd > like to see is to make dma_set_mask() only fail if DMA is entirely > disallowed - in the normal case it would always succeed, but the DMA API > implementation would be permitted to set a smaller mask than requested > (this is effectively what the x86 IOMMU ops do already). Yes, this sounds reasonable. > The significant > work that would require, though, is changing all the drivers currently > using this sort of pattern: > > if (!dma_set_mask(dev, DMA_BIT_MASK(64)) > /* put device into 64-bit mode */ > else if (!dma_set_mask(dev, DMA_BIT_MASK(32)) > /* put device into 32-bit mode */ > else > /* error */ While we have this pattern in a lot of places it's already rather pointless on most architectures as the first dma_set_mask call won't ever fail for the most common dma_ops implementations. > to something like this: > > if (!dma_set_mask(dev, DMA_BIT_MASK(64)) > /* error */ > if (dma_get_mask(dev) > DMA_BIT_MASK(32)) > /* put device into 64-bit mode */ > else > /* put device into 32-bit mode */ > > Which would be a pretty major job. I don't think it's too bad. Also for many modern devices there is no need to put the device into a specific mode. It's mostly a historic issue from the PCI/PCI-X days with the less efficient DAC addressing scheme. ^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle 2017-01-10 12:47 ` Nikita Yushchenko @ 2017-01-10 14:51 ` Christoph Hellwig -1 siblings, 0 replies; 75+ messages in thread From: Christoph Hellwig @ 2017-01-10 14:51 UTC (permalink / raw) To: Nikita Yushchenko Cc: Will Deacon, Arnd Bergmann, linux-arm-kernel, Catalin Marinas, linux-kernel, linux-renesas-soc, Simon Horman, Bjorn Helgaas, artemi.ivanov, robin.murphy, fkan, Christoph Hellwig On Tue, Jan 10, 2017 at 03:47:25PM +0300, Nikita Yushchenko wrote: > With this direction, semantics of dma mask becomes even more > questionable. I'd say dma_mask is candidate for removal (or to move to > swiotlb's or iommu's local area) We need the dma mask so that the device can advertise what addresses the device supports. Many old devices only support 32-bit DMA addressing, and some less common ones just 24-bit or other weird ones. ^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH v2] arm64: do not set dma masks that device connection can't handle @ 2017-01-10 14:51 ` Christoph Hellwig 0 siblings, 0 replies; 75+ messages in thread From: Christoph Hellwig @ 2017-01-10 14:51 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 10, 2017 at 03:47:25PM +0300, Nikita Yushchenko wrote: > With this direction, semantics of dma mask becomes even more > questionable. I'd say dma_mask is candidate for removal (or to move to > swiotlb's or iommu's local area) We need the dma mask so that the device can advertise what addresses the device supports. Many old devices only support 32-bit DMA addressing, and some less common ones just 24-bit or other weird ones. ^ permalink raw reply [flat|nested] 75+ messages in thread
end of thread, other threads:[~2017-01-13 10:41 UTC | newest] Thread overview: 75+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-09 7:30 [PATCH v2] arm64: do not set dma masks that device connection can't handle Nikita Yushchenko 2017-01-09 7:30 ` Nikita Yushchenko 2017-01-10 11:51 ` Will Deacon 2017-01-10 11:51 ` Will Deacon 2017-01-10 12:47 ` Nikita Yushchenko 2017-01-10 12:47 ` Nikita Yushchenko 2017-01-10 13:12 ` Arnd Bergmann 2017-01-10 13:12 ` Arnd Bergmann 2017-01-10 13:25 ` Robin Murphy 2017-01-10 13:25 ` Robin Murphy 2017-01-10 13:42 ` Arnd Bergmann 2017-01-10 13:42 ` Arnd Bergmann 2017-01-10 14:16 ` Robin Murphy 2017-01-10 14:16 ` Robin Murphy 2017-01-10 15:06 ` Arnd Bergmann 2017-01-10 15:06 ` Arnd Bergmann 2017-01-11 12:37 ` Nikita Yushchenko 2017-01-11 12:37 ` Nikita Yushchenko 2017-01-11 16:21 ` Arnd Bergmann 2017-01-11 16:21 ` Arnd Bergmann 2017-01-11 18:28 ` Robin Murphy 2017-01-11 18:28 ` Robin Murphy 2017-01-10 14:59 ` Christoph Hellwig 2017-01-10 14:59 ` Christoph Hellwig 2017-01-10 14:00 ` [PATCH] arm64: avoid increasing DMA masks above what hardware supports Nikita Yushchenko 2017-01-10 14:00 ` Nikita Yushchenko 2017-01-10 17:14 ` Robin Murphy 2017-01-10 17:14 ` Robin Murphy 2017-01-11 7:59 ` Nikita Yushchenko 2017-01-11 7:59 ` Nikita Yushchenko 2017-01-11 11:54 ` Robin Murphy 2017-01-11 11:54 ` Robin Murphy 2017-01-11 13:41 ` Nikita Yushchenko 2017-01-11 13:41 ` Nikita Yushchenko 2017-01-11 14:50 ` Robin Murphy 2017-01-11 14:50 ` Robin Murphy 2017-01-11 16:03 ` Nikita Yushchenko 2017-01-11 16:50 ` Robin Murphy 2017-01-11 16:50 ` Robin Murphy 2017-01-11 18:31 ` [PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports Nikita Yushchenko 2017-01-11 18:31 ` Nikita Yushchenko 2017-01-11 18:31 ` [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops() Nikita Yushchenko 2017-01-11 18:31 ` Nikita Yushchenko 2017-01-11 21:08 ` Arnd Bergmann 2017-01-11 21:08 ` Arnd Bergmann 2017-01-12 5:52 ` Nikita Yushchenko 2017-01-12 5:52 ` Nikita Yushchenko 2017-01-12 6:33 ` Nikita Yushchenko 2017-01-12 6:33 ` Nikita Yushchenko 2017-01-12 13:28 ` Arnd Bergmann 2017-01-12 13:28 ` Arnd Bergmann 2017-01-12 13:39 ` Nikita Yushchenko 2017-01-12 13:39 ` Nikita Yushchenko 2017-01-12 12:16 ` Will Deacon 2017-01-12 12:16 ` Will Deacon 2017-01-12 13:25 ` Arnd Bergmann 2017-01-12 13:25 ` Arnd Bergmann 2017-01-12 13:43 ` Robin Murphy 2017-01-12 13:43 ` Robin Murphy 2017-01-13 10:40 ` kbuild test robot 2017-01-13 10:40 ` kbuild test robot 2017-01-11 18:31 ` [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports Nikita Yushchenko 2017-01-11 18:31 ` Nikita Yushchenko 2017-01-11 21:11 ` Arnd Bergmann 2017-01-11 21:11 ` Arnd Bergmann 2017-01-12 5:53 ` Nikita Yushchenko 2017-01-12 5:53 ` Nikita Yushchenko 2017-01-13 10:16 ` kbuild test robot 2017-01-13 10:16 ` kbuild test robot 2017-01-10 14:01 ` [PATCH v2] arm64: do not set dma masks that device connection can't handle Nikita Yushchenko 2017-01-10 14:01 ` Nikita Yushchenko 2017-01-10 14:57 ` Christoph Hellwig 2017-01-10 14:57 ` Christoph Hellwig 2017-01-10 14:51 ` Christoph Hellwig 2017-01-10 14:51 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.