Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Will, On Wednesday 21 January 2015 14:48:35 Will Deacon wrote: On Tue, Jan 20, 2015 at 04:56:03PM +, Laurent Pinchart wrote: On Monday 19 January 2015 16:06:20 Will Deacon wrote: On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote: On Friday 14 November 2014 19:27:54 Will Deacon wrote: At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k). Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops. Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway. The code looks good to me, but what does it have to do with of_iommu_get_ops() having its own list of iommu_ops ? So, the initial patches for of_iommu_configure/of_xlate made use of a void *priv field in struct iommu_ops (which actually made it to mainline but isn't used). This was replaced by the list maintained by of_iommu_get_ops, but the discussion highlighted that iommu_ops is not per-IOMMU instance and therefore shouldn't contain the pgsize_bitmap, which can (and does) vary between different IOMMU instances in a running system. This patch is an effort to move the pgsize_bitmap field out of iommu_ops (leaving that structure to contain only function pointers) and into iommu_domain, where I think it makes more sense. OK, that's what I had understood. Thanks for the clarification. I was just puzzled by what I thought you meant was a dependency on of_iommu_get_ops(). diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes). -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
On Tue, Jan 20, 2015 at 04:56:03PM +, Laurent Pinchart wrote: Hi Will, Hi Laurent, Thank you for the patch. On Monday 19 January 2015 16:06:20 Will Deacon wrote: (resurrecting an old thread) On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote: On Friday 14 November 2014 19:27:54 Will Deacon wrote: At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k). Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops. Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway. The code looks good to me, but what does it have to do with of_iommu_get_ops() having its own list of iommu_ops ? So, the initial patches for of_iommu_configure/of_xlate made use of a void *priv field in struct iommu_ops (which actually made it to mainline but isn't used). This was replaced by the list maintained by of_iommu_get_ops, but the discussion highlighted that iommu_ops is not per-IOMMU instance and therefore shouldn't contain the pgsize_bitmap, which can (and does) vary between different IOMMU instances in a running system. This patch is an effort to move the pgsize_bitmap field out of iommu_ops (leaving that structure to contain only function pointers) and into iommu_domain, where I think it makes more sense. Will diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes). Will ---8 diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 98024856df07..ab9702d01e9a 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain *dom) dom-geometry.aperture_end = ~0ULL; dom-geometry.force_aperture = true; + dom-pgsize_bitmap = AMD_IOMMU_PGSIZES; return 0; out_free: @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = { .unmap = amd_iommu_unmap, .map_sg = default_iommu_map_sg, .iova_to_phys = amd_iommu_iova_to_phys, - .pgsize_bitmap = AMD_IOMMU_PGSIZES, }; /** *** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 6cd47b75286f..898fab8d10a0 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain *domain) spin_lock_init(smmu_domain-lock); domain-priv = smmu_domain; + domain-pgsize_bitmap = (SECTION_SIZE | + ARM_SMMU_PTE_CONT_SIZE | + PAGE_SIZE); return 0; out_free_domain: @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = { .remove_device = arm_smmu_remove_device, .domain_get_attr= arm_smmu_domain_get_attr, .domain_set_attr= arm_smmu_domain_set_attr, - .pgsize_bitmap = (SECTION_SIZE | -ARM_SMMU_PTE_CONT_SIZE | -PAGE_SIZE), }; static void arm_smmu_device_reset(struct arm_smmu_device *smmu) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 7ce52737c7a1..404cad25db9b 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain *domain) domain-geometry.aperture_end = ~0UL; domain-geometry.force_aperture = true; + domain-pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE; + domain-priv = priv; return 0; @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = { .iova_to_phys = exynos_iommu_iova_to_phys, .add_device = exynos_iommu_add_device, .remove_device = exynos_iommu_remove_device, - .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, }; static int __init exynos_iommu_init(void) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 40dfbc0444c0..e2b0f34baa9d 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain *domain) domain-geometry.aperture_end =
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Will, Thank you for the patch. On Monday 19 January 2015 16:06:20 Will Deacon wrote: (resurrecting an old thread) On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote: On Friday 14 November 2014 19:27:54 Will Deacon wrote: At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k). Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops. Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway. The code looks good to me, but what does it have to do with of_iommu_get_ops() having its own list of iommu_ops ? diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes). Will ---8 diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 98024856df07..ab9702d01e9a 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain *dom) dom-geometry.aperture_end = ~0ULL; dom-geometry.force_aperture = true; + dom-pgsize_bitmap = AMD_IOMMU_PGSIZES; return 0; out_free: @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = { .unmap = amd_iommu_unmap, .map_sg = default_iommu_map_sg, .iova_to_phys = amd_iommu_iova_to_phys, - .pgsize_bitmap = AMD_IOMMU_PGSIZES, }; /** *** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 6cd47b75286f..898fab8d10a0 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain *domain) spin_lock_init(smmu_domain-lock); domain-priv = smmu_domain; + domain-pgsize_bitmap = (SECTION_SIZE | + ARM_SMMU_PTE_CONT_SIZE | + PAGE_SIZE); return 0; out_free_domain: @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = { .remove_device = arm_smmu_remove_device, .domain_get_attr= arm_smmu_domain_get_attr, .domain_set_attr= arm_smmu_domain_set_attr, - .pgsize_bitmap = (SECTION_SIZE | -ARM_SMMU_PTE_CONT_SIZE | -PAGE_SIZE), }; static void arm_smmu_device_reset(struct arm_smmu_device *smmu) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 7ce52737c7a1..404cad25db9b 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain *domain) domain-geometry.aperture_end = ~0UL; domain-geometry.force_aperture = true; + domain-pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE; + domain-priv = priv; return 0; @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = { .iova_to_phys = exynos_iommu_iova_to_phys, .add_device = exynos_iommu_add_device, .remove_device = exynos_iommu_remove_device, - .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, }; static int __init exynos_iommu_init(void) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 40dfbc0444c0..e2b0f34baa9d 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain *domain) domain-geometry.aperture_end = __DOMAIN_MAX_ADDR(dmar_domain-gaw); domain-geometry.force_aperture = true; + domain-pgsize_bitmap = INTEL_IOMMU_PGSIZES; return 0; } @@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = { .iova_to_phys = intel_iommu_iova_to_phys, .add_device = intel_iommu_add_device, .remove_device = intel_iommu_remove_device, - .pgsize_bitmap = INTEL_IOMMU_PGSIZES, }; static void quirk_iommu_g4x_gfx(struct pci_dev *dev) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f7718d73e984..b8b6b0bc6951 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain *domain, pgsize = (1UL (pgsize_idx + 1)) - 1; /* throw away page
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
(resurrecting an old thread) On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote: On Friday 14 November 2014 19:27:54 Will Deacon wrote: At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k). Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops. Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway. diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes). Will ---8 diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 98024856df07..ab9702d01e9a 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain *dom) dom-geometry.aperture_end = ~0ULL; dom-geometry.force_aperture = true; + dom-pgsize_bitmap = AMD_IOMMU_PGSIZES; return 0; out_free: @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = { .unmap = amd_iommu_unmap, .map_sg = default_iommu_map_sg, .iova_to_phys = amd_iommu_iova_to_phys, - .pgsize_bitmap = AMD_IOMMU_PGSIZES, }; /* diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 6cd47b75286f..898fab8d10a0 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain *domain) spin_lock_init(smmu_domain-lock); domain-priv = smmu_domain; + domain-pgsize_bitmap = (SECTION_SIZE | +ARM_SMMU_PTE_CONT_SIZE | +PAGE_SIZE); return 0; out_free_domain: @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = { .remove_device = arm_smmu_remove_device, .domain_get_attr= arm_smmu_domain_get_attr, .domain_set_attr= arm_smmu_domain_set_attr, - .pgsize_bitmap = (SECTION_SIZE | - ARM_SMMU_PTE_CONT_SIZE | - PAGE_SIZE), }; static void arm_smmu_device_reset(struct arm_smmu_device *smmu) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 7ce52737c7a1..404cad25db9b 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain *domain) domain-geometry.aperture_end = ~0UL; domain-geometry.force_aperture = true; + domain-pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE; + domain-priv = priv; return 0; @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = { .iova_to_phys = exynos_iommu_iova_to_phys, .add_device = exynos_iommu_add_device, .remove_device = exynos_iommu_remove_device, - .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, }; static int __init exynos_iommu_init(void) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 40dfbc0444c0..e2b0f34baa9d 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain *domain) domain-geometry.aperture_end = __DOMAIN_MAX_ADDR(dmar_domain-gaw); domain-geometry.force_aperture = true; + domain-pgsize_bitmap = INTEL_IOMMU_PGSIZES; return 0; } @@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = { .iova_to_phys = intel_iommu_iova_to_phys, .add_device = intel_iommu_add_device, .remove_device = intel_iommu_remove_device, - .pgsize_bitmap = INTEL_IOMMU_PGSIZES, }; static void quirk_iommu_g4x_gfx(struct pci_dev *dev) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f7718d73e984..b8b6b0bc6951 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain *domain, pgsize = (1UL (pgsize_idx + 1)) - 1; /* throw away page sizes not supported by the hardware */ - pgsize = domain-ops-pgsize_bitmap; + pgsize = domain-pgsize_bitmap; /* make sure we're still sane */ BUG_ON(!pgsize); @@ -1046,11 +1046,11
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Will, On Wednesday 19 November 2014 11:41:50 Will Deacon wrote: On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote: On 2014-11-14 19:56, Will Deacon wrote: Hello everybody, Here is the fourth iteration of the RFC I've previously posted here: RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/28 3023.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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine, You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem thread. I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another. You can add to all your patches: Acked-by: Marek Szyprowski m.szyprow...@samsung.com Cheers. I'm also interested in adding get_default_domain() callback, but I assume that this can be done once the basic patchset get merged. Do you plan to work on it, do you want me to implement it? If Joerg isn't working on it already (I don't think he is), then please do have a go if you have time. You'll probably want to avoid adding devices with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks) to the default domain, otherwise you'll run into issues initialising the iova allocator. I had a go at getting ARM dma-mapping to use a hypothetical get_default_domain function, so I've included the diff I ended up with below, in case it's at all useful. Will ---8 diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index f3c0d953f6a2..5071553bf6b8 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev) } #define dma_max_pfn(dev) dma_max_pfn(dev) -static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, - u64 size, struct iommu_ops *iommu, - bool coherent) -{ - if (coherent) - set_dma_ops(dev, arm_coherent_dma_ops); -} #define arch_setup_dma_ops arch_setup_dma_ops +extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, +struct iommu_ops *iommu, bool coherent); static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr) { diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index c245d903927f..da2c2667bbb1 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = { * arm_iommu_attach_device function. */ struct dma_iommu_mapping * -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) +__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t base, + size_t size) { unsigned int bits = size PAGE_SHIFT; unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long); @@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) mapping-extensions = extensions; mapping-base = base; mapping-bits = BITS_PER_BYTE * bitmap_size; + mapping-domain = domain; spin_lock_init(mapping-lock); - mapping-domain = iommu_domain_alloc(bus); - if (!mapping-domain) - goto err4; - kref_init(mapping-kref); return mapping; -err4: - kfree(mapping-bitmaps[0]); err3: kfree(mapping-bitmaps); err2: @@ -1901,6 +1897,23 @@ err2: err: return ERR_PTR(err); } + +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) +{ + struct dma_iommu_mapping *mapping; + struct iommu_domain *domain; + + domain = iommu_domain_alloc(bus); + if (!domain) + return ERR_PTR(-ENOMEM); + + mapping = __arm_iommu_create_mapping(domain, base, size); + if (IS_ERR(mapping)) + iommu_domain_free(domain); + + return mapping; +} EXPORT_SYMBOL_GPL(arm_iommu_create_mapping); static void release_iommu_mapping(struct kref *kref) @@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping); *
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
On Mon, Dec 15, 2014 at 05:21:16PM +, Laurent Pinchart wrote: Hi Will, Hi Laurent, On Wednesday 19 November 2014 11:41:50 Will Deacon wrote: +static void __remove_iommu_mapping_entry(struct kref *kref) +{ + struct dma_iommu_mapping_entry *entry; + + entry = container_of(kref, struct dma_iommu_mapping_entry, kref); + list_del(entry-list); +} + +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, + struct iommu_ops *iommu) +{ + struct iommu_domain *domain; + struct dma_iommu_mapping_entry *entry = NULL; + + if (!iommu-get_default_domain) + return false; + + domain = iommu-get_default_domain(dev); + if (!domain) + return false; + + spin_lock(dma_iommu_mapping_lock); + + list_for_each_entry(entry, dma_iommu_mapping_table, list) { + if (entry-domain == domain) + break; + } That might be a stupid question (fighting my impostor syndrome again here), but is there a fundamental reason why we can't store the VA allocation data (probably using iova) in the domain instead of having to go through hoops and loops here to associate that data to the domain ? Well, this was very much a work-in-progress that I ended up abandoning :) Ultimately, I'd really like to see the default domain management moved into core code, at which point extending struct iommu_domain seems perfectly plausible to me (not sure if we'd have a new structure for that). Then we'd have a fairly clean device - group - domain - allocator abstraction for each master. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Will, On Monday 15 December 2014 17:34:16 Will Deacon wrote: On Mon, Dec 15, 2014 at 05:21:16PM +, Laurent Pinchart wrote: On Wednesday 19 November 2014 11:41:50 Will Deacon wrote: +static void __remove_iommu_mapping_entry(struct kref *kref) +{ + struct dma_iommu_mapping_entry *entry; + + entry = container_of(kref, struct dma_iommu_mapping_entry, kref); + list_del(entry-list); +} + +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, + struct iommu_ops *iommu) +{ + struct iommu_domain *domain; + struct dma_iommu_mapping_entry *entry = NULL; + + if (!iommu-get_default_domain) + return false; + + domain = iommu-get_default_domain(dev); + if (!domain) + return false; + + spin_lock(dma_iommu_mapping_lock); + + list_for_each_entry(entry, dma_iommu_mapping_table, list) { + if (entry-domain == domain) + break; + } That might be a stupid question (fighting my impostor syndrome again here), but is there a fundamental reason why we can't store the VA allocation data (probably using iova) in the domain instead of having to go through hoops and loops here to associate that data to the domain ? Well, this was very much a work-in-progress that I ended up abandoning :) Ultimately, I'd really like to see the default domain management moved into core code, at which point extending struct iommu_domain seems perfectly plausible to me (not sure if we'd have a new structure for that). Then we'd have a fairly clean device - group - domain - allocator abstraction for each master. I like that. Thank you for confirming that some of my questions are sensible :-) -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
On Fri, Nov 28, 2014 at 01:03:36PM +, jroe...@suse.de wrote: On Wed, Nov 26, 2014 at 05:47:07PM +, Will Deacon wrote: Joerg, would you expect this to go via your tree or via something broader like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead? Hmm, I don't like the idea of storing private data in iommu_ops. But given that this is already an improvement we can build on later, here is my Acked-by: Joerg Roedel jroe...@suse.de Thanks, Joerg! I'll repost this series shortly with the relevant acks. To further improve this we should probably introduce a seperate iommu-descriptor data-structure later which then describes a single hardware iommu device. Yes, I agree. I'd like to put the pgsize_bitmap in there too, as the page table series I posted yesterday has to play games to keep the mask restricted based on the tables in use by any given domain. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi all, On Tue, Nov 25, 2014 at 07:35:21AM +, Marek Szyprowski wrote: On 2014-11-19 12:41, Will Deacon wrote: On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote: On 2014-11-14 19:56, Will Deacon wrote: Hello everybody, Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine, You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem thread. I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another. It would be great if the iommu integration patches were merged to -next to give them a try for a few days. Joerg: do you plan to take those patches to v3.19 or do you want to wait more? I don't know what the plan is to get this series upstream. Joerg, would you expect this to go via your tree or via something broader like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead? I certainly don't see why patches 1-5 couldn't be queued, then we could add the ARM bits later. I just need to know where to send them! Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Will, On 14/11/14 18:56, Will Deacon wrote: Hello everybody, Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. This proposal has been really useful in getting IOMMU DMA mapping running on arm64, so with the proviso that patch 6 definitely causes problems, and patches 7 and 8 ported to arm64 rather than used directly, for the series: Tested-by: Robin Murphy robin.mur...@arm.com Hopefully it's now stable enough that I can finish cleaning up my hacked-up SMMU driver (oh, the rebasing pain :P) Thanks, Robin. Cheers, Will ---8 Will Deacon (8): 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 dma-mapping: set dma segment properties 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 | 80 ++ drivers/iommu/Kconfig | 2 +- drivers/iommu/of_iommu.c | 50 drivers/of/platform.c | 54 + include/asm-generic/vmlinux.lds.h | 2 + include/linux/amba/bus.h | 1 + include/linux/dma-mapping.h| 13 --- include/linux/iommu.h | 8 include/linux/of_iommu.h | 31 +++ include/linux/platform_device.h| 2 + 12 files changed, 214 insertions(+), 43 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hello, On 2014-11-19 12:41, Will Deacon wrote: Hi Marek, On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote: On 2014-11-14 19:56, Will Deacon wrote: Hello everybody, Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine, You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem thread. I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another. It would be great if the iommu integration patches were merged to -next to give them a try for a few days. Joerg: do you plan to take those patches to v3.19 or do you want to wait more? You can add to all your patches: Acked-by: Marek Szyprowski m.szyprow...@samsung.com Cheers. I'm also interested in adding get_default_domain() callback, but I assume that this can be done once the basic patchset get merged. Do you plan to work on it, do you want me to implement it? If Joerg isn't working on it already (I don't think he is), then please do have a go if you have time. You'll probably want to avoid adding devices with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks) to the default domain, otherwise you'll run into issues initialising the iova allocator. I had a go at getting ARM dma-mapping to use a hypothetical get_default_domain function, so I've included the diff I ended up with below, in case it's at all useful. I will check that soon, but I hope this is not strictly needed to get basic iommu and dma-mapping integration 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
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hello, On 2014-11-14 19:56, Will Deacon wrote: Hello everybody, Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine, You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem thread. You can add to all your patches: Acked-by: Marek Szyprowski m.szyprow...@samsung.com I'm also interested in adding get_default_domain() callback, but I assume that this can be done once the basic patchset get merged. Do you plan to work on it, do you want me to implement it? Cheers, Will ---8 Will Deacon (8): 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 dma-mapping: set dma segment properties 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 | 80 ++ drivers/iommu/Kconfig | 2 +- drivers/iommu/of_iommu.c | 50 drivers/of/platform.c | 54 + include/asm-generic/vmlinux.lds.h | 2 + include/linux/amba/bus.h | 1 + include/linux/dma-mapping.h| 13 --- include/linux/iommu.h | 8 include/linux/of_iommu.h | 31 +++ include/linux/platform_device.h| 2 + 12 files changed, 214 insertions(+), 43 deletions(-) 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: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Marek, On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote: On 2014-11-14 19:56, Will Deacon wrote: Hello everybody, Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine, You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem thread. I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another. You can add to all your patches: Acked-by: Marek Szyprowski m.szyprow...@samsung.com Cheers. I'm also interested in adding get_default_domain() callback, but I assume that this can be done once the basic patchset get merged. Do you plan to work on it, do you want me to implement it? If Joerg isn't working on it already (I don't think he is), then please do have a go if you have time. You'll probably want to avoid adding devices with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks) to the default domain, otherwise you'll run into issues initialising the iova allocator. I had a go at getting ARM dma-mapping to use a hypothetical get_default_domain function, so I've included the diff I ended up with below, in case it's at all useful. Will ---8 diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index f3c0d953f6a2..5071553bf6b8 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev) } #define dma_max_pfn(dev) dma_max_pfn(dev) -static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, - u64 size, struct iommu_ops *iommu, - bool coherent) -{ - if (coherent) - set_dma_ops(dev, arm_coherent_dma_ops); -} #define arch_setup_dma_ops arch_setup_dma_ops +extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, + struct iommu_ops *iommu, bool coherent); static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr) { diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index c245d903927f..da2c2667bbb1 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = { * arm_iommu_attach_device function. */ struct dma_iommu_mapping * -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) +__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t base, + size_t size) { unsigned int bits = size PAGE_SHIFT; unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long); @@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) mapping-extensions = extensions; mapping-base = base; mapping-bits = BITS_PER_BYTE * bitmap_size; + mapping-domain = domain; spin_lock_init(mapping-lock); - mapping-domain = iommu_domain_alloc(bus); - if (!mapping-domain) - goto err4; - kref_init(mapping-kref); return mapping; -err4: - kfree(mapping-bitmaps[0]); err3: kfree(mapping-bitmaps); err2: @@ -1901,6 +1897,23 @@ err2: err: return ERR_PTR(err); } + +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) +{ + struct dma_iommu_mapping *mapping; + struct iommu_domain *domain; + + domain = iommu_domain_alloc(bus); + if (!domain) + return ERR_PTR(-ENOMEM); + + mapping = __arm_iommu_create_mapping(domain, base, size); + if (IS_ERR(mapping)) + iommu_domain_free(domain); + + return mapping; +} EXPORT_SYMBOL_GPL(arm_iommu_create_mapping); static void release_iommu_mapping(struct kref *kref) @@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping); * arm_iommu_create_mapping) * * Attaches specified io address space mapping to the provided device, - * this replaces the dma operations
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
On Friday 14 November 2014 18:56:29 Will Deacon wrote: Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. Overall I think this is really nice, and I don't mind this going in, I only have one issue with they way you use iommu_ops now: At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I think rather than adding a .priv pointer to iommu_ops, we should do the same thing that a lot of other subsystems have: /* generic structure */ struct iommu { struct iommu_ops *ops; /* possibly other generic per-instance members */ }; /* driver specific structure */ struct arm_smmu { struct iommu iommu; /* smmu specific members */ }; static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu) { return container_of(iommu, struct arm_smmu, iommu); } Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
Hi Arnd, Thanks for having a look. On Fri, Nov 14, 2014 at 07:11:23PM +, Arnd Bergmann wrote: On Friday 14 November 2014 18:56:29 Will Deacon wrote: Here is the fourth iteration of the RFC I've 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 Changes since RFCv3 include: - Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops). - iommu_ops is used to hold the per-instance IOMMU data - Configuration of DMA segments added to of_dma_configure All feedback welcome. Overall I think this is really nice, and I don't mind this going in, I only have one issue with they way you use iommu_ops now: Hehe, I thought you might have something to say about that. I also had second thoughts, but decided it wasn't worse than what we already have (more below). At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k). I think rather than adding a .priv pointer to iommu_ops, we should do the same thing that a lot of other subsystems have: /* generic structure */ struct iommu { struct iommu_ops *ops; /* possibly other generic per-instance members */ }; /* driver specific structure */ struct arm_smmu { struct iommu iommu; /* smmu specific members */ }; static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu) { return container_of(iommu, struct arm_smmu, iommu); } Regardless of the arguments above, I think this layout is cleaner. We could also move the pgsize_bitmap into struct iommu in that case, however, that would be a more invasive patch series than I what I currently have. If I do another version of the patch, I can easily add a struct iommu and stash that in the device_node data for the IOMMU instead of directly putting the ops there. That's at least a step in the right direction. Cheers, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters
On Friday 14 November 2014 19:27:54 Will Deacon wrote: At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance. I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k). Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops. I think rather than adding a .priv pointer to iommu_ops, we should do the same thing that a lot of other subsystems have: /* generic structure */ struct iommu { struct iommu_ops *ops; /* possibly other generic per-instance members */ }; /* driver specific structure */ struct arm_smmu { struct iommu iommu; /* smmu specific members */ }; static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu) { return container_of(iommu, struct arm_smmu, iommu); } Regardless of the arguments above, I think this layout is cleaner. We could also move the pgsize_bitmap into struct iommu in that case, however, that would be a more invasive patch series than I what I currently have. Right, it can be done as a follow-up. It's certainly not urgent. If I do another version of the patch, I can easily add a struct iommu and stash that in the device_node data for the IOMMU instead of directly putting the ops there. That's at least a step in the right direction. Sounds good, yes. Alternatively, why not do the pointer in the opposite direction and put a 'struct device_node *dn' and a list_head into 'struct iommu'. This means you will have to traverse the list of iommus in of_iommu_get_ops, but I think it's safe to assume this is a short list and it gets walked rarely. It would be somewhat cleaner not to have to use device_node-data this way. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu