Re: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

From: Robin Murphy
Date: Tue Dec 13 2016 - 14:12:05 EST

On 13/12/16 18:46, Sricharan wrote:
> Hi Robin,
>
> <snip..>
>
>>>>>> return prot | IOMMU_READ | IOMMU_WRITE;
>>>>>
>>>>> ...and applying against -next now also needs this hunk:
>>>>>
>>>>> @@ -639,7 +639,7 @@ 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)
>>>>> {
>>>>> return __iommu_dma_map(dev, phys, size,
>>>>> - dma_direction_to_prot(dir, false) | IOMMU_MMIO);
>>>>> + dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
>>>>> }
>>>>>
>>>>> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>>>>
>>>>> With those two issues fixed up, I've given the series (applied to
>>>>> next-20161213) a spin on a SMMUv3/PL330 fast model and it still checks out.
>>>>>
>>>>
>>>> oops, sorry that i missed this in rebase. I can repost now with this fixed,
>>>> 'checks out' you mean something is not working correct ?
>>>
>>> No, I mean it _is_ still correct - I guess that's more of an idiom than
>>> I thought :)
>>>
>>
>> ha ok, thanks for the testing as well. I will just send a v8 with those two fixed now.
>
> Just while checking that i have not missed anything else, realized that the
> dma-mapping apis in arm as to be modified to pass the PRIVILIGED attributes
> as well. While my testing path was using the iommu_map directly i was not
> seeing this, but then i did a patch like below. I will just figure out another
> other codebase where the master uses the dma apis, test and add it in the
> V8 that i would send.

True, adding support to 32-bit as well can't hurt, and I guess it's
equally relevant to QC's GPU use-case. I haven't considered it myself
because AArch32 is immune to the specific PL330 problem which caught me
out - that subtle corner of VMSAv8 is unique to AArch64.

> From: Sricharan R <sricharan@xxxxxxxxxxxxxx>
> Date: Tue, 13 Dec 2016 23:25:01 +0530
> Subject: [PATCH V8 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
>
> The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
> are only accessible to privileged DMA engines. Implementing it in dma-mapping
> for it to get used from the dma mappings apis.
>
> Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
> ---
> arch/arm/mm/dma-mapping.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index ab77100..e0d9923 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1394,7 +1394,8 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
> * Create a mapping in device IO address space for specified pages
> */
> static dma_addr_t
> -__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
> +__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
> + unsigned long attrs)
> {
> struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> @@ -1419,7 +1420,7 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
>
> len = (j - i) << PAGE_SHIFT;
> ret = iommu_map(mapping->domain, iova, phys, len,
> - IOMMU_READ|IOMMU_WRITE);
> + __dma_info_to_prot(DMA_BIRECTIONAL, attrs));
> if (ret < 0)
> goto fail;
> iova += len;
> @@ -1476,7 +1477,8 @@ static struct page **__iommu_get_pages(void *cpu_addr, unsigned long attrs)
> }
>
> static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
> - dma_addr_t *handle, int coherent_flag)
> + dma_addr_t *handle, int coherent_flag,
> + unsigned long attrs)
> {
> struct page *page;
> void *addr;
> @@ -1488,7 +1490,7 @@ static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
> if (!addr)
> return NULL;
>
> - *handle = __iommu_create_mapping(dev, &page, size);
> + *handle = __iommu_create_mapping(dev, &page, size, attrs);
> if (*handle == DMA_ERROR_CODE)
> goto err_mapping;
>
> @@ -1522,7 +1524,8 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, size_t size,
>
> if (coherent_flag == COHERENT || !gfpflags_allow_blocking(gfp))
> return __iommu_alloc_simple(dev, size, gfp, handle,
> - coherent_flag);
> + coherent_flag,
> + attrs);

Super-nit: unnecessary line break.

>
> /*
> * Following is a work-around (a.k.a. hack) to prevent pages
> @@ -1672,10 +1675,13 @@ static int arm_iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
> GFP_KERNEL);
> }
>
> -static int __dma_direction_to_prot(enum dma_data_direction dir)
> +static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long attrs)
> {
> int prot;
>
> + if (attrs & DMA_ATTR_PRIVILEGED)
> + prot |= IOMMU_PRIV;
> +
> switch (dir) {
> case DMA_BIDIRECTIONAL:
> prot = IOMMU_READ | IOMMU_WRITE;
> @@ -1722,7 +1728,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
> if (!is_coherent && (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
> __dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, dir);
>
> - prot = __dma_direction_to_prot(dir);
> + prot = __dma_info_to_prot(dir, attrs);
>
> ret = iommu_map(mapping->domain, iova, phys, len, prot);
> if (ret < 0)
> @@ -1930,7 +1936,7 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
> if (dma_addr == DMA_ERROR_CODE)
> return dma_addr;
>
> - prot = __dma_direction_to_prot(dir);
> + prot = __dma_info_to_prot(dir, attrs);
>
> ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);
> if (ret < 0)
> @@ -2036,7 +2042,7 @@ static dma_addr_t arm_iommu_map_resource(struct device *dev,
> if (dma_addr == DMA_ERROR_CODE)
> return dma_addr;
>
> - prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
> + prot = __dma_info_to_prot(dir, attrs) | IOMMU_MMIO;
>
> ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
> if (ret < 0)
>

Looks reasonable to me. Assuming it survives testing:

Acked-by: Robin Murphy <robin.murphy@xxxxxxx>