lkml.org 
[lkml]   [2017]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
    Date
    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@codeaurora.org>
    >> 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@codeaurora.org>
    >> ---
    >> 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@arm.com>

    Posted V8 [1]. I changed a few more things after the testing,
    though functionally the same. So have not taken your ack and
    would be nice to have it once again

    [1] https://lkml.org/lkml/2017/1/2/224

    Regards,
    Sricharan

    \
     
     \ /
      Last update: 2017-01-02 14:23    [W:5.156 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site