This series attempts to prepare the common DMA mapping helpers, IOMMU
subsystem and IOMMU DMA helpers to be used from within loadable modules.
It does not introduce any functional changes to the code itself. The
only things done are:
- exporting related non-static functions,
- adding a common DMA mapping helper that removes the need for the
module to import find_vm_area() and rely on implementation detail
of other DMA mapping helpers,
- adding missing cleanup function in IOMMU DMA framework.
Obviously there is no mainline user that could benefit from the above,
as for now, but there is a work in progress on mainlining the Intel IPU3
camera subsystem driver, which requires its own DMA mapping code and
IOMMU driver. It can be found on ChromiumOS gerrit at
https://chromium-review.googlesource.com/c/548626/4
or checked out directly with git with following commands
git fetch https://chromium.googlesource.com/chromiumos/third_party/kernel refs/changes/26/548626/4
git checkout FETCH_HEAD
The above is based on ChromeOS 4.4 kernel branch and has been used for
testing this series with code using it on real devices.
Tomasz Figa (5):
base: dma-mapping: Export commonly used symbols
base: dma-mapping: Provide a function to look up remapped pages
iommu: Export non-static functions to use in modules
iommu/dma: Export non-static functions to use in modules
iommu/dma: Add iommu_dma_cleanup()
drivers/base/dma-mapping.c | 18 ++++++++++++++++++
drivers/iommu/dma-iommu.c | 19 +++++++++++++++++++
drivers/iommu/iommu.c | 12 ++++++++++++
include/linux/dma-iommu.h | 6 ++++++
include/linux/dma-mapping.h | 2 ++
5 files changed, 57 insertions(+)
--
2.13.2.725.g09c95d1e9-goog
There is nothing wrong in having a loadable module implementing DMA API,
for example to be used for sub-devices registered by the module.
However, most of the functions from dma-mapping do not have their
symbols exported, making it impossible to use them from loadable modules.
Export the remaining non-static functions in the file, so that loadable
modules can benefit from them. Use EXPORT_SYMBOL() for consistency with
other exports in the file.
Signed-off-by: Tomasz Figa <[email protected]>
---
drivers/base/dma-mapping.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 5096755d185e..1fda8df3d849 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -287,6 +287,7 @@ void *dma_common_pages_remap(struct page **pages, size_t size,
return area->addr;
}
+EXPORT_SYMBOL(dma_common_pages_remap);
/*
* remaps an allocated contiguous region into another vm_area.
@@ -316,6 +317,7 @@ void *dma_common_contiguous_remap(struct page *page, size_t size,
return NULL;
return area->addr;
}
+EXPORT_SYMBOL(dma_common_contiguous_remap);
/*
* unmaps a range previously mapped by dma_common_*_remap
@@ -332,6 +334,7 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags)
unmap_kernel_range((unsigned long)cpu_addr, PAGE_ALIGN(size));
vunmap(cpu_addr);
}
+EXPORT_SYMBOL(dma_common_free_remap);
#endif
/*
--
2.13.2.725.g09c95d1e9-goog
There are some non-static functions potentially useful in IOMMU drivers
that do not have their symbols exported. Export them too, so that
loadable modules can benefit from them. Use EXPORT_SYMBOL_GPL() for
consistency with other exports in the file.
Signed-off-by: Tomasz Figa <[email protected]>
---
drivers/iommu/iommu.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3f6ea160afed..41dd6b435ae3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -96,6 +96,7 @@ int iommu_device_register(struct iommu_device *iommu)
return 0;
}
+EXPORT_SYMBOL_GPL(iommu_device_register);
void iommu_device_unregister(struct iommu_device *iommu)
{
@@ -103,6 +104,7 @@ void iommu_device_unregister(struct iommu_device *iommu)
list_del(&iommu->list);
spin_unlock(&iommu_device_lock);
}
+EXPORT_SYMBOL_GPL(iommu_device_unregister);
static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
unsigned type);
@@ -744,6 +746,7 @@ struct iommu_group *iommu_group_ref_get(struct iommu_group *group)
kobject_get(group->devices_kobj);
return group;
}
+EXPORT_SYMBOL_GPL(iommu_group_ref_get);
/**
* iommu_group_put - Decrement group reference
@@ -917,6 +920,7 @@ struct iommu_group *generic_device_group(struct device *dev)
{
return iommu_group_alloc();
}
+EXPORT_SYMBOL_GPL(generic_device_group);
/*
* Use standard PCI bus topology, isolation features, and DMA alias quirks
@@ -984,6 +988,7 @@ struct iommu_group *pci_device_group(struct device *dev)
/* No shared group found, allocate new */
return iommu_group_alloc();
}
+EXPORT_SYMBOL_GPL(pci_device_group);
/**
* iommu_group_get_for_dev - Find or create the IOMMU group for a device
@@ -1044,11 +1049,13 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
return group;
}
+EXPORT_SYMBOL_GPL(iommu_group_get_for_dev);
struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
{
return group->default_domain;
}
+EXPORT_SYMBOL_GPL(iommu_group_default_domain);
static int add_iommu_group(struct device *dev, void *data)
{
@@ -1795,6 +1802,7 @@ void iommu_get_resv_regions(struct device *dev, struct list_head *list)
if (ops && ops->get_resv_regions)
ops->get_resv_regions(dev, list);
}
+EXPORT_SYMBOL_GPL(iommu_get_resv_regions);
void iommu_put_resv_regions(struct device *dev, struct list_head *list)
{
@@ -1803,6 +1811,7 @@ void iommu_put_resv_regions(struct device *dev, struct list_head *list)
if (ops && ops->put_resv_regions)
ops->put_resv_regions(dev, list);
}
+EXPORT_SYMBOL_GPL(iommu_put_resv_regions);
struct iommu_resv_region *iommu_alloc_resv_region(phys_addr_t start,
size_t length, int prot,
@@ -1821,6 +1830,7 @@ struct iommu_resv_region *iommu_alloc_resv_region(phys_addr_t start,
region->type = type;
return region;
}
+EXPORT_SYMBOL_GPL(iommu_alloc_resv_region);
/* Request that a device is direct mapped by the IOMMU */
int iommu_request_dm_for_dev(struct device *dev)
@@ -1874,6 +1884,7 @@ int iommu_request_dm_for_dev(struct device *dev)
return ret;
}
+EXPORT_SYMBOL_GPL(iommu_request_dm_for_dev);
const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
{
@@ -1889,6 +1900,7 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
spin_unlock(&iommu_device_lock);
return ops;
}
+EXPORT_SYMBOL_GPL(iommu_ops_from_fwnode);
int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
const struct iommu_ops *ops)
--
2.13.2.725.g09c95d1e9-goog
DMA API implementations, which use the dma_common_*() helpers, typically
use them in pair with other helpers, such as iommu_dma_*(). For example,
a typical .free() callback needs to retrieve the pages remapped earlier
by dma_common_remap() and call iommu_dma_unmap() on them. Currently it
is done by calling find_vm_area() manually, however it relies on
implementation details of dma_common_remap() and is also difficult to
expose to loadable modules, due to find_vm_area() being quite a low
level function without its symbol exported.
Improve this by providing a function to look-up the pages previously
remapped. It hides implementation details, can do more sanity checks
than find_vm_area() and can be exported for use in loadable modules.
Signed-off-by: Tomasz Figa <[email protected]>
---
drivers/base/dma-mapping.c | 15 +++++++++++++++
include/linux/dma-mapping.h | 2 ++
2 files changed, 17 insertions(+)
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 1fda8df3d849..9add50dd7a08 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -335,6 +335,21 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags)
vunmap(cpu_addr);
}
EXPORT_SYMBOL(dma_common_free_remap);
+
+struct page **dma_common_get_mapped_pages(void *cpu_addr,
+ unsigned long vm_flags)
+{
+ struct vm_struct *area = find_vm_area(cpu_addr);
+
+ if (!area || (area->flags & vm_flags) != vm_flags) {
+ WARN(1, "trying to get pages for invalid coherent area: %p\n",
+ cpu_addr);
+ return NULL;
+ }
+
+ return area->pages;
+}
+EXPORT_SYMBOL(dma_common_get_mapped_pages);
#endif
/*
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 843ab866e0f4..bd20435dac16 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -422,6 +422,8 @@ void *dma_common_pages_remap(struct page **pages, size_t size,
unsigned long vm_flags, pgprot_t prot,
const void *caller);
void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags);
+struct page **dma_common_get_mapped_pages(void *cpu_addr,
+ unsigned long vm_flags);
/**
* dma_mmap_attrs - map a coherent DMA allocation into user space
--
2.13.2.725.g09c95d1e9-goog
In case of loadable modules using dma-iommu helpers, it makes sense to
drop the reference to the iova cache on module exit. Add a helper called
iommu_dma_cleanup() that undoes the effects of iommu_dma_init(), so that
modules can be unloaded cleanly.
Signed-off-by: Tomasz Figa <[email protected]>
---
drivers/iommu/dma-iommu.c | 6 ++++++
include/linux/dma-iommu.h | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7cdeaf930106..51afa18a5de1 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -82,6 +82,12 @@ int iommu_dma_init(void)
}
EXPORT_SYMBOL(iommu_dma_init);
+void iommu_dma_cleanup(void)
+{
+ iova_cache_put();
+}
+EXPORT_SYMBOL(iommu_dma_cleanup);
+
/**
* iommu_get_dma_cookie - Acquire DMA-API resources for a domain
* @domain: IOMMU domain to prepare for DMA-API usage
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 92f20832fd28..b12d9207a87a 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -24,7 +24,9 @@
#include <linux/iommu.h>
#include <linux/msi.h>
+/* Framework initialization - reference counted */
int iommu_dma_init(void);
+void iommu_dma_cleanup(void);
/* Domain management interface for IOMMU drivers */
int iommu_get_dma_cookie(struct iommu_domain *domain);
@@ -85,6 +87,10 @@ static inline int iommu_dma_init(void)
return 0;
}
+static inline void iommu_dma_cleanup(void)
+{
+}
+
static inline int iommu_get_dma_cookie(struct iommu_domain *domain)
{
return -ENODEV;
--
2.13.2.725.g09c95d1e9-goog
There is nothing wrong in having a loadable module implementing DMA API,
for example to be used for sub-devices registered by the module. However,
most of the functions from dma-iommu do not have their symbols exported,
making it impossible to use them from loadable modules.
Export all the non-static functions in the file, so that loadable modules
can benefit from them. Use EXPORT_SYMBOL() for consistency with other
exports in the file.
Signed-off-by: Tomasz Figa <[email protected]>
---
drivers/iommu/dma-iommu.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe7f6cb..7cdeaf930106 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -80,6 +80,7 @@ int iommu_dma_init(void)
{
return iova_cache_get();
}
+EXPORT_SYMBOL(iommu_dma_init);
/**
* iommu_get_dma_cookie - Acquire DMA-API resources for a domain
@@ -357,6 +358,7 @@ int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
return 0;
}
}
+EXPORT_SYMBOL(dma_info_to_prot);
static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
size_t size, dma_addr_t dma_limit, struct device *dev)
@@ -504,6 +506,7 @@ void iommu_dma_free(struct device *dev, struct page **pages, size_t size,
__iommu_dma_free_pages(pages, PAGE_ALIGN(size) >> PAGE_SHIFT);
*handle = IOMMU_MAPPING_ERROR;
}
+EXPORT_SYMBOL(iommu_dma_free);
/**
* iommu_dma_alloc - Allocate and map a buffer contiguous in IOVA space
@@ -588,6 +591,7 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
__iommu_dma_free_pages(pages, count);
return NULL;
}
+EXPORT_SYMBOL(iommu_dma_alloc);
/**
* iommu_dma_mmap - Map a buffer into provided user VMA
@@ -613,6 +617,7 @@ int iommu_dma_mmap(struct page **pages, size_t size, struct vm_area_struct *vma)
}
return ret;
}
+EXPORT_SYMBOL(iommu_dma_mmap);
static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
size_t size, int prot)
@@ -643,12 +648,14 @@ dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
{
return __iommu_dma_map(dev, page_to_phys(page) + offset, size, prot);
}
+EXPORT_SYMBOL(iommu_dma_map_page);
void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
enum dma_data_direction dir, unsigned long attrs)
{
__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
}
+EXPORT_SYMBOL(iommu_dma_unmap_page);
/*
* Prepare a successfully-mapped scatterlist to give back to the caller.
@@ -802,6 +809,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
__invalidate_sg(sg, nents);
return 0;
}
+EXPORT_SYMBOL(iommu_dma_map_sg);
void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
enum dma_data_direction dir, unsigned long attrs)
@@ -822,6 +830,7 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
end = sg_dma_address(sg) + sg_dma_len(sg);
__iommu_dma_unmap(iommu_get_domain_for_dev(dev), start, end - start);
}
+EXPORT_SYMBOL(iommu_dma_unmap_sg);
dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
size_t size, enum dma_data_direction dir, unsigned long attrs)
@@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
return __iommu_dma_map(dev, phys, size,
dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
}
+EXPORT_SYMBOL(iommu_dma_map_resource);
void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
}
+EXPORT_SYMBOL(iommu_dma_unmap_resource);
int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
{
return dma_addr == IOMMU_MAPPING_ERROR;
}
+EXPORT_SYMBOL(iommu_dma_mapping_error);
static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
phys_addr_t msi_addr, struct iommu_domain *domain)
@@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
msg->address_lo += lower_32_bits(msi_page->iova);
}
}
+EXPORT_SYMBOL(iommu_dma_map_msi_msg);
--
2.13.2.725.g09c95d1e9-goog
Please use EXPORT_SYMBOL_GPL for any of these exports, as they are
internal linux implementration details by any definition of it.
On Wed, Jul 05, 2017 at 04:12:11PM +0900, Tomasz Figa wrote:
> There is nothing wrong in having a loadable module implementing DMA API,
> for example to be used for sub-devices registered by the module.
> However, most of the functions from dma-mapping do not have their
> symbols exported, making it impossible to use them from loadable modules.
I'd like to see the patches for this use case as well. We don't
generally export symbols without seeing the users.
Hi Christoph,
Thanks for comments!
On Thu, Jul 6, 2017 at 12:17 AM, Christoph Hellwig <[email protected]> wrote:
> Please use EXPORT_SYMBOL_GPL for any of these exports, as they are
> internal linux implementration details by any definition of it.
Right. I typically lean towards EXPORT_SYMBOL_GPL(), but was misled by
existing EXPORT_SYMBOL()s in the file. Will fix.
>
> On Wed, Jul 05, 2017 at 04:12:11PM +0900, Tomasz Figa wrote:
>> There is nothing wrong in having a loadable module implementing DMA API,
>> for example to be used for sub-devices registered by the module.
>> However, most of the functions from dma-mapping do not have their
>> symbols exported, making it impossible to use them from loadable modules.
>
> I'd like to see the patches for this use case as well. We don't
> generally export symbols without seeing the users.
Generally the user is a work in progress that should be posted in a
very near future. You can find a reference to our downstream tree at
chromium.org in the cover letter. Obviously I don't mind including
patches from this series in the driver series later and that's one of
the reasons for this series being RFC.
Best regards,
Tomasz
On 05/07/17 08:12, Tomasz Figa wrote:
> There is nothing wrong in having a loadable module implementing DMA API,
> for example to be used for sub-devices registered by the module. However,
> most of the functions from dma-iommu do not have their symbols exported,
> making it impossible to use them from loadable modules.
>
> Export all the non-static functions in the file, so that loadable modules
> can benefit from them. Use EXPORT_SYMBOL() for consistency with other
> exports in the file.
To echo what Christoph said, everything not already exported here
shouldn't in any way be considered a driver-facing API in the general
sense, it's horrible glue code to sit behind an arch-specific DMA
mapping implementation (and frankly I'd consider even the current
exports more of an unfortunate abstraction leakage).
> Signed-off-by: Tomasz Figa <[email protected]>
> ---
[...]
> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
> return __iommu_dma_map(dev, phys, size,
> dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
> }
> +EXPORT_SYMBOL(iommu_dma_map_resource);
>
> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
> size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
> }
> +EXPORT_SYMBOL(iommu_dma_unmap_resource);
Do you need these two? Unless your custom DMA ops really have to support
slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more
inclined to implement dma_map_resource as "return 0;" and ignore
dma_unmap_resource.
> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
> msg->address_lo += lower_32_bits(msi_page->iova);
> }
> }
> +EXPORT_SYMBOL(iommu_dma_map_msi_msg);
Given the nature of the kind of irqchip drivers this exists for, the
chances of one ever being modular seem vanishingly small.
Robin.
On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
> Generally the user is a work in progress that should be posted in a
> very near future. You can find a reference to our downstream tree at
> chromium.org in the cover letter. Obviously I don't mind including
> patches from this series in the driver series later and that's one of
> the reasons for this series being RFC.
Please post and explain them here. In general I think moving dma
ops and iommu implementations into modules is a bad idea, but I
don't want to reject the idea before seeing the code. Or maybe
by looking at the user we can come up with an even better idea
to solve the original issue you're trying to solve, so please also
explain your rationale.
On Thu, Jul 6, 2017 at 2:20 AM, Christoph Hellwig <[email protected]> wrote:
> On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
>> Generally the user is a work in progress that should be posted in a
>> very near future. You can find a reference to our downstream tree at
>> chromium.org in the cover letter. Obviously I don't mind including
>> patches from this series in the driver series later and that's one of
>> the reasons for this series being RFC.
>
> Please post and explain them here.
That's the intention. I will talk with people to try to get it posted earlier.
You can also refer to the old series, which we are reworking to
address review comments:
https://www.mail-archive.com/[email protected]/msg113505.html
The old code essentially reimplements all the DMA mapping operations
on its own, without using the generic helpers (and not even following
the API properly, see my review comments to the original patch 2 and
3), which IMHO is really ugly (but avoids exporting symbols of the
helpers, which I assumed to be a no-problem).
> In general I think moving dma
> ops and iommu implementations into modules is a bad idea
Could you elaborate on this? I'd be interested in seeing the reasoning
behind this.
> but I
> don't want to reject the idea before seeing the code. Or maybe
> by looking at the user we can come up with an even better idea
> to solve the original issue you're trying to solve, so please also
> explain your rationale.
Basically we have an x86 platform with a camera subsystem that is a
PCI device, has its own MMU and needs cache maintenance. Moreover, the
V4L2 subsystem, which is the right place for camera drivers, heavily
relies on DMA mapping as a way to abstract memory allocation, mapping
and cache maintenance. So it feels natural to me to hide the hardware
details (additional cache maintenance, mapping into the built-in
IOMMU) in the DMA mapping ops for this camera subsystem and simply
make V4L2 just work without knowing those details.
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy <[email protected]> wrote:
> On 05/07/17 08:12, Tomasz Figa wrote:
>> There is nothing wrong in having a loadable module implementing DMA API,
>> for example to be used for sub-devices registered by the module. However,
>> most of the functions from dma-iommu do not have their symbols exported,
>> making it impossible to use them from loadable modules.
>>
>> Export all the non-static functions in the file, so that loadable modules
>> can benefit from them. Use EXPORT_SYMBOL() for consistency with other
>> exports in the file.
>
> To echo what Christoph said, everything not already exported here
> shouldn't in any way be considered a driver-facing API in the general
> sense, it's horrible glue code to sit behind an arch-specific DMA
> mapping implementation (and frankly I'd consider even the current
> exports more of an unfortunate abstraction leakage).
Well, if I remember correctly, we agreed that the IPU3 driver would
benefit from using all the iommu_dma_*() helpers in its DMA ops,
similarly to ARM64. This is IMHO much better than re-implementing them
again internally just for this driver. However almost none of
necessary helpers are currently exported...
>
>> Signed-off-by: Tomasz Figa <[email protected]>
>> ---
>
> [...]
>
>> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
>> return __iommu_dma_map(dev, phys, size,
>> dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
>> }
>> +EXPORT_SYMBOL(iommu_dma_map_resource);
>>
>> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>> size_t size, enum dma_data_direction dir, unsigned long attrs)
>> {
>> __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
>> }
>> +EXPORT_SYMBOL(iommu_dma_unmap_resource);
>
> Do you need these two? Unless your custom DMA ops really have to support
> slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more
> inclined to implement dma_map_resource as "return 0;" and ignore
> dma_unmap_resource.
I don't need them. Getting an idea what is desirable to export and
what not is actually one of the goals of this RFC.
>
>> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>> msg->address_lo += lower_32_bits(msi_page->iova);
>> }
>> }
>> +EXPORT_SYMBOL(iommu_dma_map_msi_msg);
>
> Given the nature of the kind of irqchip drivers this exists for, the
> chances of one ever being modular seem vanishingly small.
Agreed. The IPU3 driver does not need it either.
Let me list the (not yet exported) helpers it requires:
dma-iommu.c
- iommu_dma_init,
- dma_info_to_prot,
- iommu_dma_free,
- iommu_dma_alloc,
- iommu_dma_mmap,
- iommu_dma_map_page,
- iommu_dma_unmap_page,
- iommu_dma_map_sg,
- iommu_dma_unmap_sg,
- iommu_dma_mapping_error,
(added by my patch) iommu_dma_cleanup,
iommu.c
- iommu_group_get_for_dev,
base/dma-mapping.c
- dma_common_pages_remap,
- dma_common_free_remap,
(added by my patch) dma_common_get_mapped_pages (OR find_vm_area),
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 2:20 AM, Christoph Hellwig <[email protected]> wrote:
>> On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
>> In general I think moving dma
>> ops and iommu implementations into modules is a bad idea
>
> Could you elaborate on this? I'd be interested in seeing the reasoning
> behind this.
>
>> but I
>> don't want to reject the idea before seeing the code. Or maybe
>> by looking at the user we can come up with an even better idea
>> to solve the original issue you're trying to solve, so please also
>> explain your rationale.
I had pretty much the same thoughts here.
> Basically we have an x86 platform with a camera subsystem that is a
> PCI device, has its own MMU and needs cache maintenance. Moreover, the
> V4L2 subsystem, which is the right place for camera drivers, heavily
> relies on DMA mapping as a way to abstract memory allocation, mapping
> and cache maintenance. So it feels natural to me to hide the hardware
> details (additional cache maintenance, mapping into the built-in
> IOMMU) in the DMA mapping ops for this camera subsystem and simply
> make V4L2 just work without knowing those details.
I can understand your reasoning here, but I'm also not convinced
that this is the best approach. There may be a middle ground somewhere
though.
Generally speaking I don't want to have to deal with the horrors of
deciding whether an IOMMU is going to be there eventually or not
at probe() time. At some point, we had decided that IOMMUs need to
be initialized (almost) as early as irqchips and clocksources so we can
rely on them to be there at device discovery time. That got pushed
back already, and now we may have to deal with -EPROBE_DEFER
when an IOMMU has not been fully initialized at device probe time,
but at least we can reliably see if one is there or not. Making IOMMUs
modular will add further uncertainty here. Obviously we cannot attach
an IOMMU to a device once we have started using DMA mapping
calls on it.
For your particular use case, I would instead leave the knowledge
about the IOMMU in the driver itself, like we do for the IOMMUs
that are integrated in desktop GPUs, and have the code use the
DMA mapping API with the system-provided dma_map_ops to
get dma_addr_t tokens which you then program into the device
IOMMU.
An open question however would be whether to use the IOMMU
API without the DMA mapping API here, or whether to completely
leave the knowledge of the IOMMU inside of the driver itself.
I don't have a strong opinion on that part, and I guess this mostly
depends on what the hardware looks like.
Arnd
On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 2:20 AM, Christoph Hellwig <[email protected]> wrote:
>>> On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
>
>>> In general I think moving dma
>>> ops and iommu implementations into modules is a bad idea
>>
>> Could you elaborate on this? I'd be interested in seeing the reasoning
>> behind this.
>>
>>> but I
>>> don't want to reject the idea before seeing the code. Or maybe
>>> by looking at the user we can come up with an even better idea
>>> to solve the original issue you're trying to solve, so please also
>>> explain your rationale.
>
> I had pretty much the same thoughts here.
>
>> Basically we have an x86 platform with a camera subsystem that is a
>> PCI device, has its own MMU and needs cache maintenance. Moreover, the
>> V4L2 subsystem, which is the right place for camera drivers, heavily
>> relies on DMA mapping as a way to abstract memory allocation, mapping
>> and cache maintenance. So it feels natural to me to hide the hardware
>> details (additional cache maintenance, mapping into the built-in
>> IOMMU) in the DMA mapping ops for this camera subsystem and simply
>> make V4L2 just work without knowing those details.
>
> I can understand your reasoning here, but I'm also not convinced
> that this is the best approach. There may be a middle ground somewhere
> though.
>
> Generally speaking I don't want to have to deal with the horrors of
> deciding whether an IOMMU is going to be there eventually or not
> at probe() time. At some point, we had decided that IOMMUs need to
> be initialized (almost) as early as irqchips and clocksources so we can
> rely on them to be there at device discovery time. That got pushed
> back already, and now we may have to deal with -EPROBE_DEFER
> when an IOMMU has not been fully initialized at device probe time,
> but at least we can reliably see if one is there or not. Making IOMMUs
> modular will add further uncertainty here. Obviously we cannot attach
> an IOMMU to a device once we have started using DMA mapping
> calls on it.
The hardware can only work with IOMMU and so the main module is highly
tied with the IOMMU module and it initialized it directly. There is no
separate struct driver or device associated with the IOMMU, as it's a
part of the one and only one PCI device (as visible from the system
PCI bus point of view) and technically handled by one pci_driver.
>
> For your particular use case, I would instead leave the knowledge
> about the IOMMU in the driver itself, like we do for the IOMMUs
> that are integrated in desktop GPUs, and have the code use the
> DMA mapping API with the system-provided dma_map_ops to
> get dma_addr_t tokens which you then program into the device
> IOMMU.
>
> An open question however would be whether to use the IOMMU
> API without the DMA mapping API here, or whether to completely
> leave the knowledge of the IOMMU inside of the driver itself.
> I don't have a strong opinion on that part, and I guess this mostly
> depends on what the hardware looks like.
+ linux-media and some media folks
I'd say that this is something that has been consistently tried to be
avoided by V4L2 and that's why it's so tightly integrated with DMA
mapping. IMHO re-implementing the code that's already there in
videobuf2 again in the driver, only because, for no good reason
mentioned as for now, having a loadable module providing DMA ops was
disliked.
Similarly with IOMMU API. It provides a lot of help in managing the
mappings and re-implementing this would be IMHO backwards.
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 2:20 AM, Christoph Hellwig <[email protected]> wrote:
>>>> On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
>>
>>>> In general I think moving dma
>>>> ops and iommu implementations into modules is a bad idea
>>>
>>> Could you elaborate on this? I'd be interested in seeing the reasoning
>>> behind this.
>>>
>>>> but I
>>>> don't want to reject the idea before seeing the code. Or maybe
>>>> by looking at the user we can come up with an even better idea
>>>> to solve the original issue you're trying to solve, so please also
>>>> explain your rationale.
>>
>> I had pretty much the same thoughts here.
>>
>>> Basically we have an x86 platform with a camera subsystem that is a
>>> PCI device, has its own MMU and needs cache maintenance. Moreover, the
>>> V4L2 subsystem, which is the right place for camera drivers, heavily
>>> relies on DMA mapping as a way to abstract memory allocation, mapping
>>> and cache maintenance. So it feels natural to me to hide the hardware
>>> details (additional cache maintenance, mapping into the built-in
>>> IOMMU) in the DMA mapping ops for this camera subsystem and simply
>>> make V4L2 just work without knowing those details.
>>
>> I can understand your reasoning here, but I'm also not convinced
>> that this is the best approach. There may be a middle ground somewhere
>> though.
>>
>> Generally speaking I don't want to have to deal with the horrors of
>> deciding whether an IOMMU is going to be there eventually or not
>> at probe() time. At some point, we had decided that IOMMUs need to
>> be initialized (almost) as early as irqchips and clocksources so we can
>> rely on them to be there at device discovery time. That got pushed
>> back already, and now we may have to deal with -EPROBE_DEFER
>> when an IOMMU has not been fully initialized at device probe time,
>> but at least we can reliably see if one is there or not. Making IOMMUs
>> modular will add further uncertainty here. Obviously we cannot attach
>> an IOMMU to a device once we have started using DMA mapping
>> calls on it.
>
> The hardware can only work with IOMMU and so the main module is highly
> tied with the IOMMU module and it initialized it directly. There is no
> separate struct driver or device associated with the IOMMU, as it's a
> part of the one and only one PCI device (as visible from the system
> PCI bus point of view) and technically handled by one pci_driver.
>
>>
>> For your particular use case, I would instead leave the knowledge
>> about the IOMMU in the driver itself, like we do for the IOMMUs
>> that are integrated in desktop GPUs, and have the code use the
>> DMA mapping API with the system-provided dma_map_ops to
>> get dma_addr_t tokens which you then program into the device
>> IOMMU.
>>
>> An open question however would be whether to use the IOMMU
>> API without the DMA mapping API here, or whether to completely
>> leave the knowledge of the IOMMU inside of the driver itself.
>> I don't have a strong opinion on that part, and I guess this mostly
>> depends on what the hardware looks like.
>
> + linux-media and some media folks
>
> I'd say that this is something that has been consistently tried to be
> avoided by V4L2 and that's why it's so tightly integrated with DMA
> mapping. IMHO re-implementing the code that's already there in
> videobuf2 again in the driver, only because, for no good reason
> mentioned as for now, having a loadable module providing DMA ops was
> disliked.
Sorry, I intended to mean:
IMHO re-implementing the code that's already there in videobuf2 again
in the driver, only because, for no good reason mentioned as for now,
having a loadable module providing DMA ops was disliked, would make no
sense.
>
> Similarly with IOMMU API. It provides a lot of help in managing the
> mappings and re-implementing this would be IMHO backwards.
>
> Best regards,
> Tomasz
On 06/07/17 03:25, Tomasz Figa wrote:
> On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy <[email protected]> wrote:
>> On 05/07/17 08:12, Tomasz Figa wrote:
>>> There is nothing wrong in having a loadable module implementing DMA API,
>>> for example to be used for sub-devices registered by the module. However,
>>> most of the functions from dma-iommu do not have their symbols exported,
>>> making it impossible to use them from loadable modules.
>>>
>>> Export all the non-static functions in the file, so that loadable modules
>>> can benefit from them. Use EXPORT_SYMBOL() for consistency with other
>>> exports in the file.
>>
>> To echo what Christoph said, everything not already exported here
>> shouldn't in any way be considered a driver-facing API in the general
>> sense, it's horrible glue code to sit behind an arch-specific DMA
>> mapping implementation (and frankly I'd consider even the current
>> exports more of an unfortunate abstraction leakage).
>
> Well, if I remember correctly, we agreed that the IPU3 driver would
> benefit from using all the iommu_dma_*() helpers in its DMA ops,
> similarly to ARM64. This is IMHO much better than re-implementing them
> again internally just for this driver. However almost none of
> necessary helpers are currently exported...
Oh, for sure - I don't personally have much objection to arch code being
modular (even as part of a driver subsystem), I just don't want anyone
to get the impression that this layer is something that any old driver
can dip into as it fancies.
>>> Signed-off-by: Tomasz Figa <[email protected]>
>>> ---
>>
>> [...]
>>
>>> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
>>> return __iommu_dma_map(dev, phys, size,
>>> dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
>>> }
>>> +EXPORT_SYMBOL(iommu_dma_map_resource);
>>>
>>> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>> size_t size, enum dma_data_direction dir, unsigned long attrs)
>>> {
>>> __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
>>> }
>>> +EXPORT_SYMBOL(iommu_dma_unmap_resource);
>>
>> Do you need these two? Unless your custom DMA ops really have to support
>> slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more
>> inclined to implement dma_map_resource as "return 0;" and ignore
>> dma_unmap_resource.
>
> I don't need them. Getting an idea what is desirable to export and
> what not is actually one of the goals of this RFC.
>
>>
>>> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>> msg->address_lo += lower_32_bits(msi_page->iova);
>>> }
>>> }
>>> +EXPORT_SYMBOL(iommu_dma_map_msi_msg);
>>
>> Given the nature of the kind of irqchip drivers this exists for, the
>> chances of one ever being modular seem vanishingly small.
>
> Agreed. The IPU3 driver does not need it either.
>
> Let me list the (not yet exported) helpers it requires:
>
> dma-iommu.c
> - iommu_dma_init,
> - dma_info_to_prot,
> - iommu_dma_free,
> - iommu_dma_alloc,
> - iommu_dma_mmap,
> - iommu_dma_map_page,
> - iommu_dma_unmap_page,
> - iommu_dma_map_sg,
> - iommu_dma_unmap_sg,
> - iommu_dma_mapping_error,
> (added by my patch) iommu_dma_cleanup,
>
> iommu.c
> - iommu_group_get_for_dev,
>
> base/dma-mapping.c
> - dma_common_pages_remap,
> - dma_common_free_remap,
> (added by my patch) dma_common_get_mapped_pages (OR find_vm_area),
I suppose another option is to just make the IOMMU and DMA ops a
self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
code, the latter more or less just needs to create the appropriate IOMMU
device for the driver to find.
Robin.
>
> Best regards,
> Tomasz
>
On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa <[email protected]> wrote:
>>
>> I'd say that this is something that has been consistently tried to be
>> avoided by V4L2 and that's why it's so tightly integrated with DMA
>> mapping. IMHO re-implementing the code that's already there in
>> videobuf2 again in the driver, only because, for no good reason
>> mentioned as for now, having a loadable module providing DMA ops was
>> disliked.
>
> Sorry, I intended to mean:
>
> IMHO re-implementing the code that's already there in videobuf2 again
> in the driver, only because, for no good reason mentioned as for now,
> having a loadable module providing DMA ops was disliked, would make no
> sense.
Why would we need to duplicate that code? I would expect that the videobuf2
core can simply call the regular dma_mapping interfaces, and you handle the
IOPTE generation at the point when the buffer is handed off from the core
code to the device driver. Am I missing something?
Arnd
On Thu, Jul 6, 2017 at 9:23 PM, Arnd Bergmann <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann <[email protected]> wrote:
>>>> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa <[email protected]> wrote:
>
>>>
>>> I'd say that this is something that has been consistently tried to be
>>> avoided by V4L2 and that's why it's so tightly integrated with DMA
>>> mapping. IMHO re-implementing the code that's already there in
>>> videobuf2 again in the driver, only because, for no good reason
>>> mentioned as for now, having a loadable module providing DMA ops was
>>> disliked.
>>
>> Sorry, I intended to mean:
>>
>> IMHO re-implementing the code that's already there in videobuf2 again
>> in the driver, only because, for no good reason mentioned as for now,
>> having a loadable module providing DMA ops was disliked, would make no
>> sense.
>
> Why would we need to duplicate that code? I would expect that the videobuf2
> core can simply call the regular dma_mapping interfaces, and you handle the
> IOPTE generation at the point when the buffer is handed off from the core
> code to the device driver. Am I missing something?
Well, for example, the iommu-dma helpers already implement all the
IOVA management, SG iterations, IOMMU API calls, sanity checks and so
on. There is a significant amount of common code.
On the other hand, if it's strictly about base/dma-mapping, we might
not need it indeed. The driver could call iommu-dma helpers directly,
without the need to provide its own DMA ops. One caveat, though, we
are not able to obtain coherent (i.e. uncached) memory with this
approach, which might have some performance effects and complicates
the code, that would now need to flush caches even for some small
internal buffers.
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 9:23 PM, Arnd Bergmann <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa <[email protected]> wrote:
>>>> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann <[email protected]> wrote:
>>>>> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa <[email protected]> wrote:
>>
>>>>
>>>> I'd say that this is something that has been consistently tried to be
>>>> avoided by V4L2 and that's why it's so tightly integrated with DMA
>>>> mapping. IMHO re-implementing the code that's already there in
>>>> videobuf2 again in the driver, only because, for no good reason
>>>> mentioned as for now, having a loadable module providing DMA ops was
>>>> disliked.
>>>
>>> Sorry, I intended to mean:
>>>
>>> IMHO re-implementing the code that's already there in videobuf2 again
>>> in the driver, only because, for no good reason mentioned as for now,
>>> having a loadable module providing DMA ops was disliked, would make no
>>> sense.
>>
>> Why would we need to duplicate that code? I would expect that the videobuf2
>> core can simply call the regular dma_mapping interfaces, and you handle the
>> IOPTE generation at the point when the buffer is handed off from the core
>> code to the device driver. Am I missing something?
>
> Well, for example, the iommu-dma helpers already implement all the
> IOVA management, SG iterations, IOMMU API calls, sanity checks and so
> on. There is a significant amount of common code.
>
> On the other hand, if it's strictly about base/dma-mapping, we might
> not need it indeed. The driver could call iommu-dma helpers directly,
> without the need to provide its own DMA ops. One caveat, though, we
> are not able to obtain coherent (i.e. uncached) memory with this
> approach, which might have some performance effects and complicates
> the code, that would now need to flush caches even for some small
> internal buffers.
I think I should add a bit of explanation here:
1) the device is non-coherent with CPU caches, even on x86,
2) it looks like x86 does not have non-coherent DMA ops, (but it
might be something that could be fixed)
3) one technically could still use __get_vm_area() and map_vm_area(),
which _are_ exported, to create an uncached mapping. I'll leave it to
you to judge if it would be better than using the already available
generic helpers.
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 3:31 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 9:23 PM, Arnd Bergmann <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa <[email protected]> wrote:
>>>
>>> Sorry, I intended to mean:
>>>
>>> IMHO re-implementing the code that's already there in videobuf2 again
>>> in the driver, only because, for no good reason mentioned as for now,
>>> having a loadable module providing DMA ops was disliked, would make no
>>> sense.
>>
>> Why would we need to duplicate that code? I would expect that the videobuf2
>> core can simply call the regular dma_mapping interfaces, and you handle the
>> IOPTE generation at the point when the buffer is handed off from the core
>> code to the device driver. Am I missing something?
>
> Well, for example, the iommu-dma helpers already implement all the
> IOVA management, SG iterations, IOMMU API calls, sanity checks and so
> on. There is a significant amount of common code.
>
> On the other hand, if it's strictly about base/dma-mapping, we might
> not need it indeed. The driver could call iommu-dma helpers directly,
> without the need to provide its own DMA ops.
Yes, that's what I meant: if using the IOMMU interface helps, I don't
see anything wrong with that, but using the iommu based
dma_map_ops seems like it may introduce more problems than
it solves.
Arnd
On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa <[email protected]> wrote:
>> On the other hand, if it's strictly about base/dma-mapping, we might
>> not need it indeed. The driver could call iommu-dma helpers directly,
>> without the need to provide its own DMA ops. One caveat, though, we
>> are not able to obtain coherent (i.e. uncached) memory with this
>> approach, which might have some performance effects and complicates
>> the code, that would now need to flush caches even for some small
>> internal buffers.
>
> I think I should add a bit of explanation here:
> 1) the device is non-coherent with CPU caches, even on x86,
> 2) it looks like x86 does not have non-coherent DMA ops, (but it
> might be something that could be fixed)
I don't understand what this means here. The PCI on x86 is always
cache-coherent, so why is the device not?
Do you mean that the device has its own caches that may need
flushing to make the device cache coherent with the CPU cache,
rather than flushing the CPU caches?
Arnd
On Thu, Jul 6, 2017 at 11:02 PM, Arnd Bergmann <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa <[email protected]> wrote:
>
>>> On the other hand, if it's strictly about base/dma-mapping, we might
>>> not need it indeed. The driver could call iommu-dma helpers directly,
>>> without the need to provide its own DMA ops. One caveat, though, we
>>> are not able to obtain coherent (i.e. uncached) memory with this
>>> approach, which might have some performance effects and complicates
>>> the code, that would now need to flush caches even for some small
>>> internal buffers.
>>
>> I think I should add a bit of explanation here:
>> 1) the device is non-coherent with CPU caches, even on x86,
>> 2) it looks like x86 does not have non-coherent DMA ops, (but it
>> might be something that could be fixed)
>
> I don't understand what this means here. The PCI on x86 is always
> cache-coherent, so why is the device not?
>
> Do you mean that the device has its own caches that may need
> flushing to make the device cache coherent with the CPU cache,
> rather than flushing the CPU caches?
Sakari might be able to explain this with more technical details, but
generally the device is not a standard PCI device one might find on
existing x86 systems.
It is some kind of embedded subsystem that behaves mostly like a PCI
device, with certain exceptions, one being the lack of coherency with
CPU caches, at least for certain parts of the subsystem. The reference
vendor code disables the coherency completely, for reasons not known
to me, but AFAICT this is the preferred operating mode, possibly due
to performance effects (this is a memory-heavy image processing
subsystem).
Best regards,
Tomasz
On Thu, Jul 06, 2017 at 12:09:45PM +0100, Robin Murphy wrote:
> I suppose another option is to just make the IOMMU and DMA ops a
> self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
> AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
> code, the latter more or less just needs to create the appropriate IOMMU
> device for the driver to find.
I still haven't seen the driver code, but this seems to be best
solution so far. Given that it's not a plug in device but part of
an SOC that seems perfectly acceptable to me.
On Thu, Jul 6, 2017 at 11:10 PM, Christoph Hellwig <[email protected]> wrote:
> On Thu, Jul 06, 2017 at 12:09:45PM +0100, Robin Murphy wrote:
>> I suppose another option is to just make the IOMMU and DMA ops a
>> self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
>> AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
>> code, the latter more or less just needs to create the appropriate IOMMU
>> device for the driver to find.
>
> I still haven't seen the driver code, but this seems to be best
> solution so far. Given that it's not a plug in device but part of
> an SOC that seems perfectly acceptable to me.
I guess that's something that could work. With its caveats of not
being able to avoid including the very platform specific code in a
generic kernel image or do any quick testing of code changes without a
restart, but I guess that's something one could quickly hack in their
own downstream (i.e. export the symbols and turn the Kconfig entry
into tristate).
On the other hand, I'm yet to see any real reasons why not to export
those symbols. Personally I don't see anything that one wouldn't be
able to do in their downstream without the symbols exported in
mainline (one can add the exports any time or if the kernel source
can't be modified can just load a wrapper module that exports its own
symbols...)
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 11:17 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 11:10 PM, Christoph Hellwig <[email protected]> wrote:
>> On Thu, Jul 06, 2017 at 12:09:45PM +0100, Robin Murphy wrote:
>>> I suppose another option is to just make the IOMMU and DMA ops a
>>> self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
>>> AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
>>> code, the latter more or less just needs to create the appropriate IOMMU
>>> device for the driver to find.
>>
>> I still haven't seen the driver code, but this seems to be best
>> solution so far. Given that it's not a plug in device but part of
>> an SOC that seems perfectly acceptable to me.
>
> I guess that's something that could work. With its caveats of not
> being able to avoid including the very platform specific code in a
> generic kernel image or do any quick testing of code changes without a
> restart, but I guess that's something one could quickly hack in their
> own downstream (i.e. export the symbols and turn the Kconfig entry
> into tristate).
>
> On the other hand, I'm yet to see any real reasons why not to export
> those symbols. Personally I don't see anything that one wouldn't be
> able to do in their downstream without the symbols exported in
> mainline (one can add the exports any time or if the kernel source
> can't be modified can just load a wrapper module that exports its own
> symbols...)
Sorry, I just realized that last sentence might sound nonsense. By
wrapper I meant reimplementing the missing functions using some
already exported functions I mentioned in my post to another patch
from this series, such as __get_vm_area() and map_vm_area().
On Thu, Jul 6, 2017 at 4:06 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 11:02 PM, Arnd Bergmann <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa <[email protected]> wrote:
>>
>>>> On the other hand, if it's strictly about base/dma-mapping, we might
>>>> not need it indeed. The driver could call iommu-dma helpers directly,
>>>> without the need to provide its own DMA ops. One caveat, though, we
>>>> are not able to obtain coherent (i.e. uncached) memory with this
>>>> approach, which might have some performance effects and complicates
>>>> the code, that would now need to flush caches even for some small
>>>> internal buffers.
>>>
>>> I think I should add a bit of explanation here:
>>> 1) the device is non-coherent with CPU caches, even on x86,
>>> 2) it looks like x86 does not have non-coherent DMA ops, (but it
>>> might be something that could be fixed)
>>
>> I don't understand what this means here. The PCI on x86 is always
>> cache-coherent, so why is the device not?
>>
>> Do you mean that the device has its own caches that may need
>> flushing to make the device cache coherent with the CPU cache,
>> rather than flushing the CPU caches?
>
> Sakari might be able to explain this with more technical details, but
> generally the device is not a standard PCI device one might find on
> existing x86 systems.
>
> It is some kind of embedded subsystem that behaves mostly like a PCI
> device, with certain exceptions, one being the lack of coherency with
> CPU caches, at least for certain parts of the subsystem. The reference
> vendor code disables the coherency completely, for reasons not known
> to me, but AFAICT this is the preferred operating mode, possibly due
> to performance effects (this is a memory-heavy image processing
Ok, got it. I think something similar happens on integrated GPUs for
a certain CPU family. The DRM code has its own ways of dealing with
this kind of device. If you find that the hardware to be closely
related (either the implementation, or the location on the internal
buses) to the GPU on this machine, I'd recommend having a look
in drivers/gpu/drm to see how it's handled there, and if that code could
be shared.
Arnd
On Thu, Jul 6, 2017 at 4:24 PM, Tomasz Figa <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 11:17 PM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 11:10 PM, Christoph Hellwig <[email protected]> wrote:
>>> On Thu, Jul 06, 2017 at 12:09:45PM +0100, Robin Murphy wrote:
>>>> I suppose another option is to just make the IOMMU and DMA ops a
>>>> self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
>>>> AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
>>>> code, the latter more or less just needs to create the appropriate IOMMU
>>>> device for the driver to find.
>>>
>>> I still haven't seen the driver code, but this seems to be best
>>> solution so far. Given that it's not a plug in device but part of
>>> an SOC that seems perfectly acceptable to me.
>>
>> I guess that's something that could work. With its caveats of not
>> being able to avoid including the very platform specific code in a
>> generic kernel image or do any quick testing of code changes without a
>> restart, but I guess that's something one could quickly hack in their
>> own downstream (i.e. export the symbols and turn the Kconfig entry
>> into tristate).
>>
>> On the other hand, I'm yet to see any real reasons why not to export
>> those symbols. Personally I don't see anything that one wouldn't be
>> able to do in their downstream without the symbols exported in
>> mainline (one can add the exports any time or if the kernel source
>> can't be modified can just load a wrapper module that exports its own
>> symbols...)
>
> Sorry, I just realized that last sentence might sound nonsense. By
> wrapper I meant reimplementing the missing functions using some
> already exported functions I mentioned in my post to another patch
> from this series, such as __get_vm_area() and map_vm_area().
We should look at the two aspects separately: one is how to drive
the IOMMU as part of a loadable driver, the other is how to handle
DMA to uncached memory on x86. You are in an unfortunate
position of needing both, but aside from that they seem unrelated.
Arnd
On Thu, Jul 6, 2017 at 11:27 PM, Arnd Bergmann <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 4:06 PM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 11:02 PM, Arnd Bergmann <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa <[email protected]> wrote:
>>>> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa <[email protected]> wrote:
>>>
>>>>> On the other hand, if it's strictly about base/dma-mapping, we might
>>>>> not need it indeed. The driver could call iommu-dma helpers directly,
>>>>> without the need to provide its own DMA ops. One caveat, though, we
>>>>> are not able to obtain coherent (i.e. uncached) memory with this
>>>>> approach, which might have some performance effects and complicates
>>>>> the code, that would now need to flush caches even for some small
>>>>> internal buffers.
>>>>
>>>> I think I should add a bit of explanation here:
>>>> 1) the device is non-coherent with CPU caches, even on x86,
>>>> 2) it looks like x86 does not have non-coherent DMA ops, (but it
>>>> might be something that could be fixed)
>>>
>>> I don't understand what this means here. The PCI on x86 is always
>>> cache-coherent, so why is the device not?
>>>
>>> Do you mean that the device has its own caches that may need
>>> flushing to make the device cache coherent with the CPU cache,
>>> rather than flushing the CPU caches?
>>
>> Sakari might be able to explain this with more technical details, but
>> generally the device is not a standard PCI device one might find on
>> existing x86 systems.
>>
>> It is some kind of embedded subsystem that behaves mostly like a PCI
>> device, with certain exceptions, one being the lack of coherency with
>> CPU caches, at least for certain parts of the subsystem. The reference
>> vendor code disables the coherency completely, for reasons not known
>> to me, but AFAICT this is the preferred operating mode, possibly due
>> to performance effects (this is a memory-heavy image processing
>
> Ok, got it. I think something similar happens on integrated GPUs for
> a certain CPU family. The DRM code has its own ways of dealing with
> this kind of device. If you find that the hardware to be closely
> related (either the implementation, or the location on the internal
> buses) to the GPU on this machine, I'd recommend having a look
> in drivers/gpu/drm to see how it's handled there, and if that code could
> be shared.
I think it's not closely related, but might be a very similar case.
Still, DRM is very liberal in terms of not using common code for doing
things, while V4L2 tries to makes things generic as much as possible.
There is already the vb2_dma_contig backend, which allocates coherent
memory (in case of V4L2-allocated buffers), manages caches (in case of
userptr or DMA-buf buffers) and so on for you. If we can't have the
DMA ops do the right thing, the code there is essentially useless and
you are left with vb2_dma_sg that uses a page allocator and gives the
driver sg tables (it actually can also do cache management for you,
but since dma_sync_sg_*() is essentially a no-op on x86, the driver
would have to do it on its own).
Best regards,
Tomasz
On Thu, Jul 6, 2017 at 11:35 PM, Arnd Bergmann <[email protected]> wrote:
> On Thu, Jul 6, 2017 at 4:24 PM, Tomasz Figa <[email protected]> wrote:
>> On Thu, Jul 6, 2017 at 11:17 PM, Tomasz Figa <[email protected]> wrote:
>>> On Thu, Jul 6, 2017 at 11:10 PM, Christoph Hellwig <[email protected]> wrote:
>>>> On Thu, Jul 06, 2017 at 12:09:45PM +0100, Robin Murphy wrote:
>>>>> I suppose another option is to just make the IOMMU and DMA ops a
>>>>> self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
>>>>> AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
>>>>> code, the latter more or less just needs to create the appropriate IOMMU
>>>>> device for the driver to find.
>>>>
>>>> I still haven't seen the driver code, but this seems to be best
>>>> solution so far. Given that it's not a plug in device but part of
>>>> an SOC that seems perfectly acceptable to me.
>>>
>>> I guess that's something that could work. With its caveats of not
>>> being able to avoid including the very platform specific code in a
>>> generic kernel image or do any quick testing of code changes without a
>>> restart, but I guess that's something one could quickly hack in their
>>> own downstream (i.e. export the symbols and turn the Kconfig entry
>>> into tristate).
>>>
>>> On the other hand, I'm yet to see any real reasons why not to export
>>> those symbols. Personally I don't see anything that one wouldn't be
>>> able to do in their downstream without the symbols exported in
>>> mainline (one can add the exports any time or if the kernel source
>>> can't be modified can just load a wrapper module that exports its own
>>> symbols...)
>>
>> Sorry, I just realized that last sentence might sound nonsense. By
>> wrapper I meant reimplementing the missing functions using some
>> already exported functions I mentioned in my post to another patch
>> from this series, such as __get_vm_area() and map_vm_area().
>
> We should look at the two aspects separately: one is how to drive
> the IOMMU as part of a loadable driver, the other is how to handle
> DMA to uncached memory on x86. You are in an unfortunate
> position of needing both, but aside from that they seem unrelated.
So generally I'm not in such a desperate need to have this code as a
loadable module. I can just hack around it in my own working tree if I
need it. However I'm trying to understand, why this is such a bad
idea, if I already have it working.
Uncached DMA memory on x86 is actually more complicated. We could
still work around this by not relying on the memory being uncached
(with caveats obviously). But here too I'd like to understand why
having my own DMA ops is bad.
Best regards,
Tomasz