lkml.org 
[lkml]   [2021]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 2/3] iommu/dma-iommu: Support iovad->granule > PAGE_SIZE
From
Date
On 2021-08-07 09:41, Sven Peter wrote:
> Hi,
>
> Thanks a lot for quick reply!
>
> On Fri, Aug 6, 2021, at 20:04, Robin Murphy wrote:
>> On 2021-08-06 16:55, Sven Peter via iommu wrote:
>>> DMA IOMMU domains can support hardware where the IOMMU page size is
>>> larger than the CPU page size.
>>> Alignments need to be done with respect to both PAGE_SIZE and
>>> iovad->granule. Additionally, the sg list optimization to use a single
>>> IOVA allocation cannot be used in those cases since the physical
>>> addresses will very likely not be aligned to the larger IOMMU page size.
>>>
>>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>>> ---
>>> drivers/iommu/dma-iommu.c | 87 ++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 77 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 6f0df629353f..e072d9030d9f 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -8,6 +8,7 @@
>>> * Copyright (C) 2000-2004 Russell King
>>> */
>>>
>>> +#include <linux/align.h>
>>> #include <linux/acpi_iort.h>
>>> #include <linux/device.h>
>>> #include <linux/dma-map-ops.h>
>>> @@ -51,6 +52,15 @@ struct iommu_dma_cookie {
>>> struct iommu_domain *fq_domain;
>>> };
>>>
>>> +/* aligns size to CPU and IOMMU page size */
>>> +static inline size_t iommu_page_align(struct device *dev, size_t size)
>>> +{
>>> + struct iommu_domain *domain = iommu_get_dma_domain(dev);
>>> + struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>> +
>>> + return iova_align(&cookie->iovad, PAGE_ALIGN(size));
>>> +}
>>> +
>>> static DEFINE_STATIC_KEY_FALSE(iommu_deferred_attach_enabled);
>>> bool iommu_dma_forcedac __read_mostly;
>>>
>>> @@ -647,6 +657,8 @@ static struct page **__iommu_dma_alloc_pages(struct device *dev,
>>> /*
>>> * If size is less than PAGE_SIZE, then a full CPU page will be allocated,
>>> * but an IOMMU which supports smaller pages might not map the whole thing.
>>> + * If the IOMMU page size is larger than the CPU page size, then the size
>>> + * will be aligned to that granularity and some memory will be left unused.
>>
>> Why do we need to increase the actual memory allocation? The point here
>> is that we allocate the smallest thing we can allocate and map the
>> smallest thing we can map - I think that still works the "wrong" way
>> round too, we should just need to start taking an IOVA offset into
>> account as in dma_map_page() if we can no longer assume it's 0 for a CPU
>> page. Sure we may expose some unrelated adjacent pages, but we'll
>> already be doing that to excess for streaming DMA so whoop de do.
>
> I agree for trusted devices, but untrusted ones (Thunderbolt, and depending on your
> risk tolerance possibly even the broadcom wifi) might also end up calling this.

Oh, right, I hadn't considered actual untrusted device support at this
stage.

> For streaming DMA swiotlb will make sure that these won't see memory
> they're not supposed to access.

I was slightly surprised to see that that does appear to work out OK,
but I guess SWIOTLB slots are already smaller than typical IOMMU pages,
so it falls out of that. Neat.

> But, at least as far as I understand it, no swiotlb is in the way to catch devices
> who end up calling this function. That wasn't required because we used to get
> PAGE_SIZE aligned allocation here and every IOMMU so far would be able to easily
> map them without any spill overs.
> But now we'll end up exposing three more unrelated pages if the allocation
> is not increased.

Fair enough, but then why still waste memory in the (usual) cases where
it logically *isn't* necessary?

>>> */
>>> static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
>>> size_t size, struct sg_table *sgt, gfp_t gfp, pgprot_t prot,
>>> @@ -736,7 +748,7 @@ static void *iommu_dma_alloc_remap(struct device *dev, size_t size,
>>>
>>> out_unmap:
>>> __iommu_dma_unmap(dev, *dma_handle, size);
>>> - __iommu_dma_free_pages(pages, PAGE_ALIGN(size) >> PAGE_SHIFT);
>>> + __iommu_dma_free_pages(pages, iommu_page_align(dev, size) >> PAGE_SHIFT);
>>> return NULL;
>>> }
>>>
>>> @@ -766,7 +778,8 @@ static void iommu_dma_free_noncontiguous(struct device *dev, size_t size,
>>> struct dma_sgt_handle *sh = sgt_handle(sgt);
>>>
>>> __iommu_dma_unmap(dev, sgt->sgl->dma_address, size);
>>> - __iommu_dma_free_pages(sh->pages, PAGE_ALIGN(size) >> PAGE_SHIFT);
>>> + __iommu_dma_free_pages(sh->pages,
>>> + iommu_page_align(dev, size) >> PAGE_SHIFT);
>>> sg_free_table(&sh->sgt);
>>> kfree(sh);
>>> }
>>> @@ -1006,6 +1019,31 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>>> if (dev_is_untrusted(dev))
>>> return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
>>>
>>> + /*
>>> + * If the IOMMU pagesize is larger than the CPU pagesize we will
>>> + * very likely run into sgs with a physical address that is not aligned
>>> + * to an IOMMU page boundary. Fall back to just mapping every entry
>>> + * independently with __iommu_dma_map then.
>>
>> Scatterlist segments often don't have nicely aligned ends, which is why
>> we already align things to IOVA granules in main loop here. I think in
>> principle we'd just need to move the non-IOVA-aligned part of the
>> address from sg->page to sg->offset in the temporary transformation for
>> the rest of the assumptions to hold. I don't blame you for being timid
>> about touching that, though - it took me 3 tries to get right when I
>> first wrote it...
>
> I was a little bit afraid I'd get this exact reply :D
> I'll try to modify the transformation again, but I'm sure it'll take me more than
> 3 tries to get it right :)
>
>>
>>> + */
>>> + if (iovad->granule > PAGE_SIZE) {
>>> + for_each_sg(sg, s, nents, i) {
>>> + sg_dma_address(s) = __iommu_dma_map(dev, sg_phys(s),
>>> + s->length, prot, dma_get_mask(dev));
>>> + if (sg_dma_address(s) == DMA_MAPPING_ERROR)
>>> + break;
>>> + sg_dma_len(s) = s->length;
>>> + }
>>> +
>>> + if (unlikely(i != nents)) {
>>> + nents = i;
>>> + for_each_sg(sg, s, nents, i)
>>> + __iommu_dma_unmap(dev, sg_dma_address(s), sg_dma_len(s));
>>> + return 0;
>>> + }
>>> +
>>> + return nents;
>>> + }
>>
>> Either way, NAK to having a *third* implementation of SG mapping in this
>> file which is fundamentally no different from the second one.
>
> Good point. If for some reason I'm not able to modify the transformation correctly
> I'll just fall back to iommu_dma_map_sg_swiotlb.
>
>>
>>> +
>>> /*
>>> * Work out how much IOVA space we need, and align the segments to
>>> * IOVA granules for the IOMMU driver to handle. With some clever
>>> @@ -1068,6 +1106,9 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>>> static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>>> int nents, enum dma_data_direction dir, unsigned long attrs)
>>> {
>>> + struct iommu_domain *domain = iommu_get_dma_domain(dev);
>>> + struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>> + struct iova_domain *iovad = &cookie->iovad;
>>> dma_addr_t start, end;
>>> struct scatterlist *tmp;
>>> int i;
>>> @@ -1080,6 +1121,17 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>>> return;
>>> }
>>>
>>> + /*
>>> + * If the IOMMU pagesize is larger than the CPU pagesize we mapped
>>> + * every entry indepedently with __iommu_dma_map then. Let's do the
>>> + * opposite here.
>>> + */
>>> + if (iovad->granule > PAGE_SIZE) {
>>> + for_each_sg(sg, tmp, nents, i)
>>> + __iommu_dma_unmap(dev, sg_dma_address(tmp), sg_dma_len(tmp));
>>> + return;
>>> + }
>>
>> As above, this is just __iommu_dma_unmap_sg_swiotlb() with fewer clothes on.
>>
>>> +
>>> /*
>>> * The scatterlist segments are mapped into a single
>>> * contiguous IOVA allocation, so this is incredibly easy.
>>> @@ -1110,7 +1162,7 @@ static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>>
>>> static void __iommu_dma_free(struct device *dev, size_t size, void *cpu_addr)
>>> {
>>> - size_t alloc_size = PAGE_ALIGN(size);
>>> + size_t alloc_size = iommu_page_align(dev, size);
>>> int count = alloc_size >> PAGE_SHIFT;
>>> struct page *page = NULL, **pages = NULL;
>>>
>>> @@ -1150,7 +1202,7 @@ static void *iommu_dma_alloc_pages(struct device *dev, size_t size,
>>> struct page **pagep, gfp_t gfp, unsigned long attrs)
>>> {
>>> bool coherent = dev_is_dma_coherent(dev);
>>> - size_t alloc_size = PAGE_ALIGN(size);
>>> + size_t alloc_size = iommu_page_align(dev, size);
>>> int node = dev_to_node(dev);
>>> struct page *page = NULL;
>>> void *cpu_addr;
>>> @@ -1201,8 +1253,8 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
>>>
>>> if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
>>> !gfpflags_allow_blocking(gfp) && !coherent)
>>> - page = dma_alloc_from_pool(dev, PAGE_ALIGN(size), &cpu_addr,
>>> - gfp, NULL);
>>> + page = dma_alloc_from_pool(dev, iommu_page_align(dev, size),
>>> + &cpu_addr, gfp, NULL);
>>> else
>>> cpu_addr = iommu_dma_alloc_pages(dev, size, &page, gfp, attrs);
>>> if (!cpu_addr)
>>> @@ -1253,6 +1305,7 @@ static int iommu_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>>> void *cpu_addr, dma_addr_t dma_addr, size_t size,
>>> unsigned long attrs)
>>
>> Can we just not bother trying to support this? TBH I don't know exactly
>> how the interface is supposed to work - what you're doing here looks
>> like it's probably either too much or not enough, depending on whether
>> the address and size arguments are supposed to allow representing
>> partial buffers - and frankly I can't imagine you'll be needing to
>> support dma-buf exports from the USB/ethernet/wifi drivers any time soon...
>
> I'm not really sure how this is going to work even before my changes.
> Just from reading the code it looks like even then it might be doing too much
> or too little.
> But this will also be used for Thunderbolt and who knows what kind of devices
> will be connected there. I'm fine with just not supporting this interface unless
> something actually breaks for some user though.

I would bet that the set of random Thunderbolt-attachable devices which
participate in dma-buf exports and the set of media devices which expect
vb2_dma_contig to work for arbitrary user buffers have a non-empty
intersection. If you eat your cake you may subsequently discover that
you no longer have your cake ;)

If we can't easily test it or reason about it, let's not pretend to
implement it. I'd rather somebody discovered the lack of working support
in a few years' time because it reliably and safely returned an error,
rather than because it ate their page cache. Besides, it's not like
people can't use a kernel built with the right PAGE_SIZE (which might
perform better anyway) and not have the problem in the first place.

>>> {
>>> + size_t aligned_size = iommu_page_align(dev, size);
>>> struct page *page;
>>> int ret;
>>>
>>> @@ -1261,7 +1314,7 @@ static int iommu_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>>>
>>> if (pages) {
>>> return sg_alloc_table_from_pages(sgt, pages,
>>> - PAGE_ALIGN(size) >> PAGE_SHIFT,
>>> + aligned_size >> PAGE_SHIFT,
>>> 0, size, GFP_KERNEL);
>>> }
>>>
>>> @@ -1272,7 +1325,7 @@ static int iommu_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>>>
>>> ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
>>> if (!ret)
>>> - sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
>>> + sg_set_page(sgt->sgl, page, aligned_size, 0);
>>> return ret;
>>> }
>>>
>>> @@ -1283,11 +1336,25 @@ static unsigned long iommu_dma_get_merge_boundary(struct device *dev)
>>> return (1UL << __ffs(domain->pgsize_bitmap)) - 1;
>>> }
>>>
>>> +static struct page *iommu_dma_alloc_aligned_pages(struct device *dev, size_t size,
>>> + dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp)
>>> +{
>>> + size = iommu_page_align(dev, size);
>>> + return dma_common_alloc_pages(dev, size, dma_handle, dir, gfp);
>>> +}
>>> +
>>> +static void iommu_dma_free_aligned_pages(struct device *dev, size_t size, struct page *page,
>>> + dma_addr_t dma_handle, enum dma_data_direction dir)
>>> +{
>>> + size = iommu_page_align(dev, size);
>>> + return dma_common_free_pages(dev, size, page, dma_handle, dir);
>>> +}
>>
>> Again, what's the point of these? iommu_dma_map_page() still has to cope
>> with whatever the caller provides, so there's no difference in the one
>> case when that caller happens to be dma_common_map_pages().
>
> Same as above, untrusted devices.

Again fair enough, but in that case do it for untrusted devices. Not for
the whole world for most of whom it still *is* a needless waste.

Robin.

\
 
 \ /
  Last update: 2021-08-09 20:38    [W:0.083 / U:0.808 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site