Re: [RFC PATCH v3 4/6] swiotlb: Add restricted DMA alloc/free support.
On Wed, Jan 13, 2021 at 06:27:08PM +, Robin Murphy wrote: >> Can we hook in somewhat lower level in the dma-direct code so that all >> the remapping in dma-direct can be reused instead of duplicated? That >> also becomes important if we want to use non-remapping uncached support, >> e.g. on mips or x86, or the direct changing of the attributes that Will >> planned to look into for arm64. > > Indeed, AFAICS this ought to boil down to a direct equivalent of > __dma_direct_alloc_pages() - other than the address there should be no > conceptual difference between pages from the restricted pool and those from > the regular page allocator, so this probably deserves to be plumbed in as > an alternative to that. Yes, that's what I mean. You managed to word it much better, though.
Re: [RFC PATCH v3 4/6] swiotlb: Add restricted DMA alloc/free support.
On 2021-01-13 12:48, Christoph Hellwig wrote:
+#ifdef CONFIG_SWIOTLB
+ if (unlikely(dev->dma_io_tlb_mem))
+ return swiotlb_alloc(dev, size, dma_handle, attrs);
+#endif
Another place where the dma_io_tlb_mem is useful to avoid the ifdef.
-phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr,
- size_t mapping_size, size_t alloc_size,
- enum dma_data_direction dir, unsigned long attrs)
+static int swiotlb_tbl_find_free_region(struct device *hwdev,
+ dma_addr_t tbl_dma_addr,
+ size_t alloc_size,
+ unsigned long attrs)
+static void swiotlb_tbl_release_region(struct device *hwdev, int index,
+ size_t size)
This refactoring should be another prep patch.
+void *swiotlb_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
+ unsigned long attrs)
I'd rather have the names convey there are for the per-device bounce
buffer in some form.
+ struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
While we're at it I wonder if the io_tlb is something we could change
while we're at it. Maybe replace io_tlb_mem with struct swiotlb
and rename the field in struct device to dev_swiotlb?
+ int index;
+ void *vaddr;
+ phys_addr_t tlb_addr;
+
+ size = PAGE_ALIGN(size);
+ index = swiotlb_tbl_find_free_region(dev, mem->start, size, attrs);
+ if (index < 0)
+ return NULL;
+
+ tlb_addr = mem->start + (index << IO_TLB_SHIFT);
+ *dma_handle = phys_to_dma_unencrypted(dev, tlb_addr);
+
+ if (!dev_is_dma_coherent(dev)) {
+ unsigned long pfn = PFN_DOWN(tlb_addr);
+
+ /* remove any dirty cache lines on the kernel alias */
+ arch_dma_prep_coherent(pfn_to_page(pfn), size);
Can we hook in somewhat lower level in the dma-direct code so that all
the remapping in dma-direct can be reused instead of duplicated? That
also becomes important if we want to use non-remapping uncached support,
e.g. on mips or x86, or the direct changing of the attributes that Will
planned to look into for arm64.
Indeed, AFAICS this ought to boil down to a direct equivalent of
__dma_direct_alloc_pages() - other than the address there should be no
conceptual difference between pages from the restricted pool and those
from the regular page allocator, so this probably deserves to be plumbed
in as an alternative to that.
Robin.
Re: [RFC PATCH v3 4/6] swiotlb: Add restricted DMA alloc/free support.
> +#ifdef CONFIG_SWIOTLB
> + if (unlikely(dev->dma_io_tlb_mem))
> + return swiotlb_alloc(dev, size, dma_handle, attrs);
> +#endif
Another place where the dma_io_tlb_mem is useful to avoid the ifdef.
> -phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t
> orig_addr,
> - size_t mapping_size, size_t alloc_size,
> - enum dma_data_direction dir, unsigned long attrs)
> +static int swiotlb_tbl_find_free_region(struct device *hwdev,
> + dma_addr_t tbl_dma_addr,
> + size_t alloc_size,
> + unsigned long attrs)
> +static void swiotlb_tbl_release_region(struct device *hwdev, int index,
> +size_t size)
This refactoring should be another prep patch.
> +void *swiotlb_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
> + unsigned long attrs)
I'd rather have the names convey there are for the per-device bounce
buffer in some form.
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
While we're at it I wonder if the io_tlb is something we could change
while we're at it. Maybe replace io_tlb_mem with struct swiotlb
and rename the field in struct device to dev_swiotlb?
> + int index;
> + void *vaddr;
> + phys_addr_t tlb_addr;
> +
> + size = PAGE_ALIGN(size);
> + index = swiotlb_tbl_find_free_region(dev, mem->start, size, attrs);
> + if (index < 0)
> + return NULL;
> +
> + tlb_addr = mem->start + (index << IO_TLB_SHIFT);
> + *dma_handle = phys_to_dma_unencrypted(dev, tlb_addr);
> +
> + if (!dev_is_dma_coherent(dev)) {
> + unsigned long pfn = PFN_DOWN(tlb_addr);
> +
> + /* remove any dirty cache lines on the kernel alias */
> + arch_dma_prep_coherent(pfn_to_page(pfn), size);
Can we hook in somewhat lower level in the dma-direct code so that all
the remapping in dma-direct can be reused instead of duplicated? That
also becomes important if we want to use non-remapping uncached support,
e.g. on mips or x86, or the direct changing of the attributes that Will
planned to look into for arm64.
Re: [RFC PATCH v3 4/6] swiotlb: Add restricted DMA alloc/free support.
On 1/5/21 7:41 PM, Claire Chang wrote: > Add the functions, swiotlb_alloc and swiotlb_free to support the > memory allocation from restricted DMA pool. > > Signed-off-by: Claire Chang > --- [snip] > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 30ccbc08e229..126e9b3354d6 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -137,6 +137,11 @@ void *dma_direct_alloc(struct device *dev, size_t size, > void *ret; > int err; > > +#ifdef CONFIG_SWIOTLB > + if (unlikely(dev->dma_io_tlb_mem)) > + return swiotlb_alloc(dev, size, dma_handle, attrs); > +#endif While this is potentially a hot path, I am not sure of the unkikely is warranted, maybe best left as a plain conditional. -- Florian
[RFC PATCH v3 4/6] swiotlb: Add restricted DMA alloc/free support.
Add the functions, swiotlb_alloc and swiotlb_free to support the
memory allocation from restricted DMA pool.
Signed-off-by: Claire Chang
---
include/linux/swiotlb.h | 6 ++
kernel/dma/direct.c | 12 +++
kernel/dma/swiotlb.c| 171 +---
3 files changed, 144 insertions(+), 45 deletions(-)
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 5135e5636042..84fe96e40685 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -68,6 +68,12 @@ extern void swiotlb_tbl_sync_single(struct device *hwdev,
dma_addr_t swiotlb_map(struct device *dev, phys_addr_t phys,
size_t size, enum dma_data_direction dir, unsigned long attrs);
+void *swiotlb_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
+ unsigned long attrs);
+
+void swiotlb_free(struct device *dev, size_t size, void *vaddr,
+ dma_addr_t dma_addr, unsigned long attrs);
+
#ifdef CONFIG_SWIOTLB
extern enum swiotlb_force swiotlb_force;
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 30ccbc08e229..126e9b3354d6 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -137,6 +137,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
void *ret;
int err;
+#ifdef CONFIG_SWIOTLB
+ if (unlikely(dev->dma_io_tlb_mem))
+ return swiotlb_alloc(dev, size, dma_handle, attrs);
+#endif
+
size = PAGE_ALIGN(size);
if (attrs & DMA_ATTR_NO_WARN)
gfp |= __GFP_NOWARN;
@@ -246,6 +251,13 @@ void dma_direct_free(struct device *dev, size_t size,
{
unsigned int page_order = get_order(size);
+#ifdef CONFIG_SWIOTLB
+ if (unlikely(dev->dma_io_tlb_mem)) {
+ swiotlb_free(dev, size, cpu_addr, dma_addr, attrs);
+ return;
+ }
+#endif
+
if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
!force_dma_unencrypted(dev)) {
/* cpu_addr is a struct page cookie, not a kernel address */
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 1f05af09e61a..ca88ef59435d 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -459,14 +459,13 @@ static void swiotlb_bounce(phys_addr_t orig_addr,
phys_addr_t tlb_addr,
}
}
-phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr,
- size_t mapping_size, size_t alloc_size,
- enum dma_data_direction dir, unsigned long attrs)
+static int swiotlb_tbl_find_free_region(struct device *hwdev,
+ dma_addr_t tbl_dma_addr,
+ size_t alloc_size,
+ unsigned long attrs)
{
struct io_tlb_mem *mem = get_io_tlb_mem(hwdev);
- dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(hwdev, mem->start);
unsigned long flags;
- phys_addr_t tlb_addr;
unsigned int nslots, stride, index, wrap;
int i;
unsigned long mask;
@@ -477,15 +476,6 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
phys_addr_t orig_addr,
if (no_iotlb_memory && !hwdev->dma_io_tlb_mem)
panic("Can not allocate SWIOTLB buffer earlier and can't now
provide you with the DMA bounce buffer");
- if (mem_encrypt_active())
- pr_warn_once("Memory encryption is active and system is using
DMA bounce buffers\n");
-
- if (mapping_size > alloc_size) {
- dev_warn_once(hwdev, "Invalid sizes (mapping: %zd bytes, alloc:
%zd bytes)",
- mapping_size, alloc_size);
- return (phys_addr_t)DMA_MAPPING_ERROR;
- }
-
mask = dma_get_seg_boundary(hwdev);
tbl_dma_addr &= mask;
@@ -547,7 +537,6 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
phys_addr_t orig_addr,
mem->list[i] = 0;
for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) !=
IO_TLB_SEGSIZE - 1) && mem->list[i]; i--)
mem->list[i] = ++count;
- tlb_addr = mem->start + (index << IO_TLB_SHIFT);
/*
* Update the indices to avoid searching in the next
@@ -570,45 +559,21 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
phys_addr_t orig_addr,
if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit())
dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes), total
%lu (slots), used %lu (slots)\n",
alloc_size, mem->nslabs, tmp_io_tlb_used);
- return (phys_addr_t)DMA_MAPPING_ERROR;
+ return -ENOMEM;
+
found:
mem->used += nslots;
spin_unlock_irqrestore(>lock, flags);
- /*
-* Save away the mapping from the original address to the DMA address.
-* This is needed when we sync the memory. Then we sync the buffer if
-* needed.
-*/
