| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Nov 2019 12:30:05 +0000
From: Laurentiu Tudor <laurentiu.tudor@....com>
To: Robin Murphy <robin.murphy@....com>, "hch@....de" <hch@....de>,
"joro@...tes.org" <joro@...tes.org>,
Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Ioana Ciornei <ioana.ciornei@....com>
CC: Leo Li <leoyang.li@....com>,
Diana Madalina Craciun <diana.craciun@....com>,
"davem@...emloft.net" <davem@...emloft.net>,
Madalin Bucur <madalin.bucur@....com>
Subject: Re: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api
variants
Hi Robin,
On 28.10.2019 15:42, Robin Murphy wrote:
> On 24/10/2019 13:41, Laurentiu Tudor wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@....com>
>>
>> Introduce a few new dma unmap and sync variants that, on top of the
>> original variants, return the virtual address corresponding to the
>> input dma address.
>> In order to implement this a new dma map op is added and used:
>> void *get_virt_addr(dev, dma_handle);
>> It does the actual conversion of an input dma address to the output
>> virtual address.
>
> At this point, I think it might be better to just change the prototype
> of the .unmap_page/.sync_single_for_cpu callbacks themselves. In cases
> where .get_virt_addr would be non-trivial, it's most likely duplicating
> work that the relevant callback has to do anyway (i.e. where the virtual
> and/or physical address is needed internally for a cache maintenance or
> bounce buffer operation).
Looking in the generic dma-iommu, I didn't see any mean of freely
getting the pa or va bqcking the iova so I can't think of a way of doing
this without adding a call to iommu_iova_to_phys() somewhere in the
unmap op implementation. Obviously, this would come with an overhead
that will probably upset people.
At the moment I can't think at an option other than the initial one,
that is adding the .get_virt_addr op. Please let me know your opinions
on this.
---
Thanks & Best Regards, Laurentiu
> It would also help avoid any possible
> ambiguity about whether .get_virt_addr returns the VA corresponding
> dma_handle (if one exists) rather than the VA of the buffer *mapped to*
> dma_handle, which for a bounce-buffering implementation would be
> different, and the one you actually need - a naive
> phys_to_virt(dma_to_phys(dma_handle)) would lead you to the wrong place
> (in fact it looks like DPAA2 would currently go wrong with
> "swiotlb=force" and the SMMU disabled or in passthrough).
>
> One question there is whether we'd want careful special-casing to avoid
> introducing overhead where unmap/sync are currently complete no-ops, or
> whether an extra phys_to_virt() or so in those paths would be tolerable.
>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@....com>
>> ---
>> include/linux/dma-mapping.h | 55 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 4a1c4fca475a..ae7bb8a84b9d 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -132,6 +132,7 @@ struct dma_map_ops {
>> u64 (*get_required_mask)(struct device *dev);
>> size_t (*max_mapping_size)(struct device *dev);
>> unsigned long (*get_merge_boundary)(struct device *dev);
>> + void *(*get_virt_addr)(struct device *dev, dma_addr_t dma_handle);
>> };
>> #define DMA_MAPPING_ERROR (~(dma_addr_t)0)
>> @@ -304,6 +305,21 @@ static inline void dma_unmap_page_attrs(struct
>> device *dev, dma_addr_t addr,
>> debug_dma_unmap_page(dev, addr, size, dir);
>> }
>> +static inline struct page *
>> +dma_unmap_page_attrs_desc(struct device *dev, dma_addr_t addr, size_t
>> size,
>> + enum dma_data_direction dir, unsigned long attrs)
>> +{
>> + const struct dma_map_ops *ops = get_dma_ops(dev);
>> + void *ptr = NULL;
>> +
>> + if (ops && ops->get_virt_addr)
>> + ptr = ops->get_virt_addr(dev, addr);
>
> Note that this doesn't work for dma-direct, but for the sake of arm64 at
> least it almost certainly wants to.
>
> Robin.
>
>> + dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>> +
>> + return ptr ? virt_to_page(ptr) : NULL;
>> +}
>> +
>> /*
>> * dma_maps_sg_attrs returns 0 on error and > 0 on success.
>> * It should never return a value < 0.
>> @@ -390,6 +406,21 @@ static inline void dma_sync_single_for_cpu(struct
>> device *dev, dma_addr_t addr,
>> debug_dma_sync_single_for_cpu(dev, addr, size, dir);
>> }
>> +static inline void *
>> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr,
>> size_t size,
>> + enum dma_data_direction dir)
>> +{
>> + const struct dma_map_ops *ops = get_dma_ops(dev);
>> + void *ptr = NULL;
>> +
>> + if (ops && ops->get_virt_addr)
>> + ptr = ops->get_virt_addr(dev, addr);
>> +
>> + dma_sync_single_for_cpu(dev, addr, size, dir);
>> +
>> + return ptr;
>> +}
>> +
>> static inline void dma_sync_single_for_device(struct device *dev,
>> dma_addr_t addr, size_t size,
>> enum dma_data_direction dir)
>> @@ -500,6 +531,12 @@ static inline void dma_sync_single_for_cpu(struct
>> device *dev, dma_addr_t addr,
>> size_t size, enum dma_data_direction dir)
>> {
>> }
>> +
>> +static inline void *
>> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr,
>> size_t size,
>> + enum dma_data_direction dir)
>> +{
>> +}
>> static inline void dma_sync_single_for_device(struct device *dev,
>> dma_addr_t addr, size_t size, enum dma_data_direction dir)
>> {
>> @@ -594,6 +631,21 @@ static inline void dma_unmap_single_attrs(struct
>> device *dev, dma_addr_t addr,
>> return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>> }
>> +static inline void *
>> +dma_unmap_single_attrs_desc(struct device *dev, dma_addr_t addr,
>> size_t size,
>> + enum dma_data_direction dir, unsigned long attrs)
>> +{
>> + const struct dma_map_ops *ops = get_dma_ops(dev);
>> + void *ptr = NULL;
>> +
>> + if (ops && ops->get_virt_addr)
>> + ptr = ops->get_virt_addr(dev, addr);
>> +
>> + dma_unmap_single_attrs(dev, addr, size, dir, attrs);
>> +
>> + return ptr;
>> +}
>> +
>> static inline void dma_sync_single_range_for_cpu(struct device *dev,
>> dma_addr_t addr, unsigned long offset, size_t size,
>> enum dma_data_direction dir)
>> @@ -610,10 +662,13 @@ static inline void
>> dma_sync_single_range_for_device(struct device *dev,
>> #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
>> #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s,
>> r, 0)
>> +#define dma_unmap_single_desc(d, a, s, r) \
>> + dma_unmap_single_attrs_desc(d, a, s, r, 0)
>> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>> #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s,
>> r, 0)
>> #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
>> +#define dma_unmap_page_desc(d, a, s, r) dma_unmap_page_attrs_desc(d,
>> a, s, r, 0)
>> #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t,
>> v, h, s, 0)
>> #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h,
>> s, 0)
>>
Powered by blists - more mailing lists