Re: [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem
Hello, On 2014-12-02 10:59, Sjoerd Simons wrote: Hey Marek, Inki, On Wed, 2014-11-19 at 12:15 +0100, Marek Szyprowski wrote: Hello Everyone, This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest automatic DMA configuration for IOMMU masters patches from Will Deacon. Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset? I hope to add Exynos5 SYSMMU patches to the next iteration of my patchset, but I doubt it will get into v3.19-rc1. For some background to that question, We (re-)discovered yesterday that the out-of-tree exynos-reference kernel iommu patches are required to get HDMI out working on exynos 5 boards. The current situation in mainline is rather broken, HDMI output without CONFIG_DRM_EXYNOS_IOMMU results in just displaying stripes[0]. While turning on CONFIG_DRM_EXYNOS_IOMMU causes a kernel oops at boot We have observed similar issues with Exynos4 based boards, when LCD0 power domain was turned off and only TV power domain has been powered on. Please check the power domain configuration. Maybe in case of Exynos5 the same issue is caused by the interaction between DISP1 and GSCL domains. Best regards -- Marek Szyprowski, PhD Samsung RD Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 2/4] iommu: add ARM LPAE page table allocator
Hi Will, Please find my comments inline. Search for varun -Original Message- From: Will Deacon [mailto:will.dea...@arm.com] Sent: Thursday, November 27, 2014 5:21 PM To: linux-arm-ker...@lists.infradead.org; iommu@lists.linux-foundation.org Cc: prem.malla...@broadcom.com; robin.mur...@arm.com; lau...@codeaurora.org; mitch...@codeaurora.org; laurent.pinch...@ideasonboard.com; j...@8bytes.org; Sethi Varun-B16395; m.szyprow...@samsung.com; Will Deacon Subject: [PATCH 2/4] iommu: add ARM LPAE page table allocator A number of IOMMUs found in ARM SoCs can walk architecture-compatible page tables. This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 4-levels of walk to cover a 48-bit address space. Signed-off-by: Will Deacon will.dea...@arm.com --- MAINTAINERS| 1 + drivers/iommu/Kconfig | 9 + drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable-arm.c | 735 + drivers/iommu/io-pgtable.c | 7 + drivers/iommu/io-pgtable.h | 12 + 6 files changed, 765 insertions(+) create mode 100644 drivers/iommu/io-pgtable-arm.c diff --git a/MAINTAINERS b/MAINTAINERS index 0ff630de8a6d..d3ca31b7c960 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1562,6 +1562,7 @@ M:Will Deacon will.dea...@arm.com L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) S: Maintained F: drivers/iommu/arm-smmu.c +F: drivers/iommu/io-pgtable-arm.c ARM64 PORT (AARCH64 ARCHITECTURE) M: Catalin Marinas catalin.mari...@arm.com diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 0f10554e7114..e1742a0146f8 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -19,6 +19,15 @@ menu Generic IOMMU Pagetable Support config IOMMU_IO_PGTABLE bool +config IOMMU_IO_PGTABLE_LPAE + bool ARMv7/v8 Long Descriptor Format + select IOMMU_IO_PGTABLE + help + Enable support for the ARM long descriptor pagetable format. + This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page + sizes at both stage-1 and stage-2, as well as address spaces + up to 48-bits in size. + endmenu config OF_IOMMU diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index aff244c78181..269cdd82b672 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o +obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c new file mode 100644 index ..9dbaa2e48424 --- /dev/null +++ b/drivers/iommu/io-pgtable-arm.c @@ -0,0 +1,735 @@ +/* + * CPU-agnostic ARM page table allocator. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + * + * Copyright (C) 2014 ARM Limited + * + * Author: Will Deacon will.dea...@arm.com */ + +#define pr_fmt(fmt)arm-lpae io-pgtable: fmt + +#include linux/iommu.h +#include linux/kernel.h +#include linux/sizes.h +#include linux/slab.h +#include linux/types.h + +#include io-pgtable.h + +#define ARM_LPAE_MAX_ADDR_BITS 48 +#define ARM_LPAE_S2_MAX_CONCAT_PAGES 16 +#define ARM_LPAE_MAX_LEVELS4 + +/* Struct accessors */ +#define io_pgtable_to_data(x) \ + container_of((x), struct arm_lpae_io_pgtable, iop) + +#define io_pgtable_ops_to_pgtable(x) \ + container_of((x), struct io_pgtable, ops) + +#define io_pgtable_ops_to_data(x) \ + io_pgtable_to_data(io_pgtable_ops_to_pgtable(x)) + +/* + * For consistency with the architecture, we always consider + * ARM_LPAE_MAX_LEVELS levels, with the walk starting at level n =0 +*/ +#define ARM_LPAE_START_LVL(d) (ARM_LPAE_MAX_LEVELS - (d)-levels) + +/* + * Calculate the right shift amount to get to the portion describing +level l + * in a virtual address mapped by the pagetable in d. + */ +#define ARM_LPAE_LVL_SHIFT(l,d) \ +
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 04, 2014 at 07:42:28PM +, Robin Murphy wrote:
On 04/12/14 17:58, Grant Likely wrote:
[...]
+struct of_iommu_node {
+ struct list_head list;
+ struct device_node *np;
+ struct iommu_ops *ops;
Why can't this be const? Why would anyone ever need to modify it? Also
drivers do define their iommu_ops structures const these days, so you'll
either still have to cast away at some point or the compiler will
complain once you start calling this from drivers.
...whereas if we make everything const then the drivers that _do_ want to
use the private data introduced by this series and thus pass mutable
per-instance iommu_ops will be the ones having to cast. We have no users
either way until this series is merged, and the need to stash the
per-instance data somewhere other than np-data is in the way of getting it
merged - this is just a quick hack to address that. I think we've already
agreed that mutable per-instance iommu_ops holding private data aren't
particularly pleasant and will (hopefully) go away in the next
iteration[1],
at which point this all changes anyway.
Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.
Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.
I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.
Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 0/8] Introduce automatic DMA configuration for IOMMU masters
On Fri, Dec 05, 2014 at 07:12:24AM +, Olof Johansson wrote: On Mon, Dec 1, 2014 at 8:57 AM, Will Deacon will.dea...@arm.com wrote: Hello again, This is version 6 of the patches previously posted here: RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html RFCv4: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/302711.html v5: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/307213.html The only change since v5 is the addition of acks from various maintainers. Now that the ARM bits have rmk's ack and the IOMMU bits have joro's ack, I think this is good for merging via the arm-soc tree. Please let me know if a pull request would be preferable. Hi, Yes, please collect the acks from the discussion in the last day or so and send a pull request (and include Robin's patch, obviously). Sure, I'm just working out which of Robin's patches to take (if I don't hear otherwise, it will be the one with Grant's ack on it). Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Friday 05 December 2014 12:10:37 Will Deacon wrote:
On Thu, Dec 04, 2014 at 07:42:28PM +, Robin Murphy wrote:
On 04/12/14 17:58, Grant Likely wrote:
[...]
+struct of_iommu_node {
+ struct list_head list;
+ struct device_node *np;
+ struct iommu_ops *ops;
Why can't this be const? Why would anyone ever need to modify it? Also
drivers do define their iommu_ops structures const these days, so you'll
either still have to cast away at some point or the compiler will
complain once you start calling this from drivers.
...whereas if we make everything const then the drivers that _do_ want to
use the private data introduced by this series and thus pass mutable
per-instance iommu_ops will be the ones having to cast. We have no users
either way until this series is merged, and the need to stash the
per-instance data somewhere other than np-data is in the way of getting
it
merged - this is just a quick hack to address that. I think we've already
agreed that mutable per-instance iommu_ops holding private data aren't
particularly pleasant and will (hopefully) go away in the next
iteration[1],
at which point this all changes anyway.
Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.
Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.
I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.
Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.
I think that would be ok. The fix later should be to move the
private data pointer into of_iommu_node, but I think that will
require a larger set of changes, so let's defer that.
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? Thierry pgpP_DLSxlZYQ.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 05, 2014 at 01:21:31PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging. Actually not making everything const would be a hack. Drivers already mark their struct iommu_ops as const. But I'm more referring to the series as a whole. It seems like there are various issues that still need to be ironed out, and there's committment to do that before 3.20, so unless there are drivers that need any of the unfinished patches for 3.19 I don't see why we should be merging them in the first place. If getting them into 3.19 is merely to resolve dependencies then it's not going to work well anyway. Since this is all going to change in 3.20 anyway we'd likely have new dependencies that need to be handled, so might just as well do it properly at that time. Thierry pgp7c9IuvJxq5.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hello, On 2014-12-05 14:18, Thierry Reding wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? There are Exynos SYSMMU patches ready waiting for this gets merged... Best regards -- Marek Szyprowski, PhD Samsung RD Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[GIT PULL] Automatic DMA configuration for OF-based IOMMU masters
Hi Olof, Please consider pulling the following series for 3.19. I appreciate that this is late in the day, but the patches have been around for a while and have collected all the Acks that they need. Marek has already been finding the series useful with the Exynos IOMMU, so it seems a pity to hold the patches up any longer. Cheers, Will ---8 The following changes since commit 5d01410fe4d92081f349b013a2e7a95429e4f2c9: Linux 3.18-rc6 (2014-11-23 15:25:20 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git tags/of-iommu-configure for you to fetch changes up to a42a7a1fb5f1f9004b023594609dc22da02fc08b: iommu: store DT-probed IOMMU data privately (2014-12-05 14:35:52 +) This series adds automatic IOMMU and DMA-mapping configuration for OF-based DMA masters described using the generic IOMMU devicetree bindings. Although there is plenty of future work around splitting up iommu_ops, adding default IOMMU domains and sorting out automatic IOMMU group creation for the platform_bus, this is already useful enough for people to port over their IOMMU drivers and start using the new probing infrastructure (indeed, Marek has patches queued for the Exynos IOMMU). Marek Szyprowski (1): iommu: fix initialization without 'add_device' callback Robin Murphy (1): iommu: store DT-probed IOMMU data privately Will Deacon (7): iommu: provide early initialisation hook for IOMMU drivers dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops iommu: add new iommu_ops callback for adding an OF device iommu: provide helper function to configure an IOMMU for an of master dma-mapping: detect and configure IOMMU in of_dma_configure arm: call iommu_init before of_platform_populate arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops arch/arm/include/asm/dma-mapping.h | 12 ++--- arch/arm/kernel/setup.c| 2 + arch/arm/mm/dma-mapping.c | 83 --- drivers/iommu/Kconfig | 2 +- drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 89 ++ drivers/of/platform.c | 50 +++-- include/asm-generic/vmlinux.lds.h | 2 + include/linux/dma-mapping.h| 13 +++--- include/linux/iommu.h | 8 include/linux/of_iommu.h | 23 ++ 11 files changed, 242 insertions(+), 44 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator
On Fri, Dec 05, 2014 at 10:55:11AM +, Varun Sethi wrote:
Hi Will,
Hi Varun,
Thanks for the review!
+static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
+unsigned long iova, phys_addr_t paddr,
+arm_lpae_iopte prot, int lvl,
+arm_lpae_iopte *ptep)
+{
+ arm_lpae_iopte pte = prot;
+
+ /* We require an unmap first */
+ if (iopte_leaf(*ptep, lvl))
+ return -EEXIST;
[varun] Instead of returning an error, how about displaying a warning and
replacing the entry?
I'd be ok with displaying a warning, but I don't think we should just
continue. It indicates a misuse of the IOMMU API and probably a missing
TLBI.
+static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long
iova,
+ phys_addr_t paddr, size_t size, arm_lpae_iopte prot,
+ int lvl, arm_lpae_iopte *ptep)
+{
+ arm_lpae_iopte *cptep, pte;
+ void *cookie = data-iop.cookie;
+ size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+
+ /* Find our entry at the current level */
+ ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
+
+ /* If we can install a leaf entry at this level, then do so */
+ if (size == block_size (size data-iop.cfg.pgsize_bitmap))
+ return arm_lpae_init_pte(data, iova, paddr, prot, lvl, ptep);
+
+ /* We can't allocate tables at the final level */
+ if (WARN_ON(lvl = ARM_LPAE_MAX_LEVELS - 1))
+ return -EINVAL;
[varun] A warning message would be helpful.
Sure, I can add one.
+static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
+ int prot)
+{
+ arm_lpae_iopte pte;
+
+ if (data-iop.fmt == ARM_LPAE_S1) {
+ pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+
+ if (!(prot IOMMU_WRITE) (prot IOMMU_READ))
+ pte |= ARM_LPAE_PTE_AP_RDONLY;
+
+ if (prot IOMMU_CACHE)
+ pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
+ARM_LPAE_PTE_ATTRINDX_SHIFT);
+ } else {
+ pte = ARM_LPAE_PTE_HAP_FAULT;
+ if (prot IOMMU_READ)
+ pte |= ARM_LPAE_PTE_HAP_READ;
+ if (prot IOMMU_WRITE)
+ pte |= ARM_LPAE_PTE_HAP_WRITE;
+ if (prot IOMMU_CACHE)
+ pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
+ else
+ pte |= ARM_LPAE_PTE_MEMATTR_NC;
+ }
+
+ if (prot IOMMU_NOEXEC)
+ pte |= ARM_LPAE_PTE_XN;
+
+ return pte;
+}
[[varun]] Do you plan to add a flag to indicate device memory? We had a
discussion about this on the patch submitted by me. May be you can include
that as a part of this patch.
That needs to go in as a separate patch. I think you should continue to push
that separately!
+static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
+ unsigned long iova, size_t size,
+ arm_lpae_iopte prot, int lvl,
+ arm_lpae_iopte *ptep, size_t blk_size) {
+ unsigned long blk_start, blk_end;
+ phys_addr_t blk_paddr;
+ arm_lpae_iopte table = 0;
+ void *cookie = data-iop.cookie;
+ struct iommu_gather_ops *tlb = data-iop.cfg.tlb;
+
+ blk_start = iova ~(blk_size - 1);
+ blk_end = blk_start + blk_size;
+ blk_paddr = iopte_to_pfn(*ptep, data) data-pg_shift;
+
+ for (; blk_start blk_end; blk_start += size, blk_paddr += size) {
+ arm_lpae_iopte *tablep;
+
+ /* Unmap! */
+ if (blk_start == iova)
+ continue;
+
+ /* __arm_lpae_map expects a pointer to the start of the table
*/
+ tablep = table - ARM_LPAE_LVL_IDX(blk_start, lvl, data);
[[varun]] Not clear what's happening here. May be I am missing something,
but where is the table allocated?
It is allocated in __arm_lpae_map, because the pte will be 0. The
subtraction above is to avoid us having to allocate a whole level, just for
a single invalid pte.
+ if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot,
lvl,
+ tablep) 0) {
[[varun]] Again not clear how are we unmapping the range. Index at the
current level should point to a page table (with contiguous block
mappings). Unmap would applied to the mappings at the next level. Unmap
can happen anywhere in the contiguous range. It seems that you are just
creating a subset of the block mapping.
We will be unmapping a single entry at the next level, so we basically
create a table, then map everything at the next level apart from the part we
need to
Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator
On Tue, Dec 02, 2014 at 11:47:36AM +, Laurent Pinchart wrote: On Tuesday 02 December 2014 09:41:56 Will Deacon wrote: On Mon, Dec 01, 2014 at 08:21:58PM +, Laurent Pinchart wrote: On Monday 01 December 2014 17:23:15 Will Deacon wrote: On Sun, Nov 30, 2014 at 11:29:46PM +, Laurent Pinchart wrote: On Thursday 27 November 2014 11:51:16 Will Deacon wrote: + /* Looking good; allocate a pgd */ + data-pgd = alloc_pages_exact(1UL data-pg_shift, + GFP_KERNEL | __GFP_ZERO); data-pg_shift is computed as __ffs(cfg-pgsize_bitmap). 1UL data-pg_shift will thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating. Yeah, I'll take a closer look at this. The size of the pgd really depends on the TxSZ configuration, which in turn depends on the ias and the page size. There are also alignment constraints to bear in mind, but I'll see what I can do (as it stands, over-allocating will always work). Beside wasting memory, the code also doesn't reflect the requirements. It works by chance, meaning it could break later. It won't break, as the maximum size *is* bounded by a page for stage-1 and we already handle stage-2 concatenation correctly. What I mean is that there's no correlation between the required size and the allocated size in the current code. It happens to work, but if the driver gets extended later to support more IOMMU configurations subtle bugs may crop up. That's why I'd like to see this being fixed. Can't the size be computed with something like size = (1 (ias - data-levels * data-pg_shift)) * sizeof(arm_lpae_iopte); (please add a proper detailed comment to explain the computation, as the meaning is not straightforward) That's actually the easy part. The harder part is getting the correct alignment, which means managing by own kmem_cache on a per-ops basis. That feels like overkill to me and we also need to make sure that we don't screw up the case of concatenated pgds at stage-2. On top of that, since each cache would be per-ops, I'm not even sure we'd save anything (the slab allocators all operate on pages afaict). If I use alloc_page_exact, we'll still have some wasteage, but it would be less for the case where the CPU page size is smaller than the SMMU page size. Do you think that's worth the extra complexity? We allocate full pages at all levels after the pgd, so the wasteage is relatively small. An alternative would be preinitialising some caches for `likely' pgd sizes, but that's also horrible, especially if the kernel decides that it doesn't need a bunch of the configurations at runtime. How about just computing the right size, align it to a page size, and using alloc_page_exact ? The waste is small, so it doesn't justify anything more complex than that. Ok, I'll have a go at that. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
