Re: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap the buffer
On 4/3/22 10:05 PM, Christoph Hellwig wrote:
> To shared more code between swiotlb and xen-swiotlb, offer a
> swiotlb_init_remap interface and add a remap callback to
> swiotlb_init_late that will allow Xen to remap the buffer the
> buffer without duplicating much of the logic.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/x86/pci/sta2x11-fixup.c | 2 +-
> include/linux/swiotlb.h | 5 -
> kernel/dma/swiotlb.c | 36 +---
> 3 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
> index c7e6faf59a861..7368afc039987 100644
> --- a/arch/x86/pci/sta2x11-fixup.c
> +++ b/arch/x86/pci/sta2x11-fixup.c
> @@ -57,7 +57,7 @@ static void sta2x11_new_instance(struct pci_dev *pdev)
> int size = STA2X11_SWIOTLB_SIZE;
> /* First instance: register your own swiotlb area */
> dev_info(>dev, "Using SWIOTLB (size %i)\n", size);
> - if (swiotlb_init_late(size, GFP_DMA))
> + if (swiotlb_init_late(size, GFP_DMA, NULL))
> dev_emerg(>dev, "init swiotlb failed\n");
> }
> list_add(>list, _instance_list);
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index ee655f2e4d28b..7b50c82f84ce9 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -36,8 +36,11 @@ struct scatterlist;
>
> int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, unsigned int
> flags);
> unsigned long swiotlb_size_or_default(void);
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs));
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs));
> extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask);
> extern void __init swiotlb_update_mem_attributes(void);
>
> phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 119187afc65ec..d5fe8f5e08300 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -256,9 +256,11 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs,
> * Statically reserve bounce buffer space and initialize bounce buffer data
> * structures for the software IO TLB used to implement the DMA API.
> */
> -void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> - size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> + unsigned long nslabs = default_nslabs;
> + size_t bytes;
> void *tlb;
>
> if (!addressing_limit && !swiotlb_force_bounce)
> @@ -271,12 +273,23 @@ void __init swiotlb_init(bool addressing_limit,
> unsigned int flags)
>* allow to pick a location everywhere for hypervisors with guest
>* memory encryption.
>*/
> +retry:
> + bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> if (flags & SWIOTLB_ANY)
> tlb = memblock_alloc(bytes, PAGE_SIZE);
> else
> tlb = memblock_alloc_low(bytes, PAGE_SIZE);
> if (!tlb)
> goto fail;
> + if (remap && remap(tlb, nslabs) < 0) {
> + memblock_free(tlb, PAGE_ALIGN(bytes));
> +
> + nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
> + if (nslabs < IO_TLB_MIN_SLABS)
> + panic("%s: Failed to remap %zu bytes\n",
> + __func__, bytes);
> + goto retry;
> + }
> if (swiotlb_init_with_tbl(tlb, default_nslabs, flags))
> goto fail_free_mem;
> return;
> @@ -287,12 +300,18 @@ void __init swiotlb_init(bool addressing_limit,
> unsigned int flags)
> pr_warn("Cannot allocate buffer");
> }
>
> +void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +{
> + return swiotlb_init_remap(addressing_limit, flags, NULL);
> +}
> +
> /*
> * Systems with larger DMA zones (those that don't support ISA) can
> * initialize the swiotlb later using the slab allocator if needed.
> * This should be just like above, but with some error catching.
> */
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
> unsigned long bytes;
> @@ -303,6 +322,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> if (swiotlb_force_disable)
> return 0;
>
> +retry:
> order = get_order(nslabs << IO_TLB_SHIFT);
> nslabs = SLABS_PER_PAGE << order;
> bytes = nslabs << IO_TLB_SHIFT;
> @@ -323,6
Re: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap the buffer
Hi Christoph,
On Mon, Apr 04, 2022 at 05:05:56AM +, Christoph Hellwig wrote:
> From: Christoph Hellwig
> Subject: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap
> the buffer
>
> To shared more code between swiotlb and xen-swiotlb, offer a
> swiotlb_init_remap interface and add a remap callback to
> swiotlb_init_late that will allow Xen to remap the buffer the
s/the buffer//
> buffer without duplicating much of the logic.
Alan
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/x86/pci/sta2x11-fixup.c | 2 +-
> include/linux/swiotlb.h | 5 -
> kernel/dma/swiotlb.c | 36 +---
> 3 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
> index c7e6faf59a861..7368afc039987 100644
> --- a/arch/x86/pci/sta2x11-fixup.c
> +++ b/arch/x86/pci/sta2x11-fixup.c
> @@ -57,7 +57,7 @@ static void sta2x11_new_instance(struct pci_dev *pdev)
> int size = STA2X11_SWIOTLB_SIZE;
> /* First instance: register your own swiotlb area */
> dev_info(>dev, "Using SWIOTLB (size %i)\n", size);
> - if (swiotlb_init_late(size, GFP_DMA))
> + if (swiotlb_init_late(size, GFP_DMA, NULL))
> dev_emerg(>dev, "init swiotlb failed\n");
> }
> list_add(>list, _instance_list);
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index ee655f2e4d28b..7b50c82f84ce9 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -36,8 +36,11 @@ struct scatterlist;
>
> int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, unsigned int
> flags);
> unsigned long swiotlb_size_or_default(void);
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs));
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs));
> extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask);
> extern void __init swiotlb_update_mem_attributes(void);
>
> phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 119187afc65ec..d5fe8f5e08300 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -256,9 +256,11 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs,
> * Statically reserve bounce buffer space and initialize bounce buffer data
> * structures for the software IO TLB used to implement the DMA API.
> */
> -void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> - size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> + unsigned long nslabs = default_nslabs;
> + size_t bytes;
> void *tlb;
>
> if (!addressing_limit && !swiotlb_force_bounce)
> @@ -271,12 +273,23 @@ void __init swiotlb_init(bool addressing_limit,
> unsigned int flags)
>* allow to pick a location everywhere for hypervisors with guest
>* memory encryption.
>*/
> +retry:
> + bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> if (flags & SWIOTLB_ANY)
> tlb = memblock_alloc(bytes, PAGE_SIZE);
> else
> tlb = memblock_alloc_low(bytes, PAGE_SIZE);
> if (!tlb)
> goto fail;
> + if (remap && remap(tlb, nslabs) < 0) {
> + memblock_free(tlb, PAGE_ALIGN(bytes));
> +
> + nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
> + if (nslabs < IO_TLB_MIN_SLABS)
> + panic("%s: Failed to remap %zu bytes\n",
> + __func__, bytes);
> + goto retry;
> + }
> if (swiotlb_init_with_tbl(tlb, default_nslabs, flags))
> goto fail_free_mem;
> return;
> @@ -287,12 +300,18 @@ void __init swiotlb_init(bool addressing_limit,
> unsigned int flags)
> pr_warn("Cannot allocate buffer");
> }
>
> +void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +{
> + return swiotlb_init_remap(addressing_limit, flags, NULL);
> +}
> +
> /*
> * Systems with larger DMA zones (those that don't support ISA) can
> * initialize the swiotlb later using the slab allocator if needed.
> * This should be just like above, but with some error catching.
> */
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
> unsigned long bytes;
> @@ -303,6 +322,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> if (swiotlb_force_disable)
>
Re: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap the buffer
On 3/15/22 2:36 AM, Christoph Hellwig wrote:
@@ -271,12 +273,23 @@ void __init swiotlb_init(bool addressing_limit, unsigned
int flags)
* allow to pick a location everywhere for hypervisors with guest
* memory encryption.
*/
+retry:
+ bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
if (flags & SWIOTLB_ANY)
tlb = memblock_alloc(bytes, PAGE_SIZE);
else
tlb = memblock_alloc_low(bytes, PAGE_SIZE);
if (!tlb)
goto fail;
+ if (remap && remap(tlb, nslabs) < 0) {
+ memblock_free(tlb, PAGE_ALIGN(bytes));
+
+ if (nslabs <= IO_TLB_MIN_SLABS)
+ panic("%s: Failed to remap %zu bytes\n",
+ __func__, bytes);
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
I spoke with Konrad (who wrote the original patch ---
f4b2f07b2ed9b469ead87e06fc2fc3d12663a725) and apparently the reason for 2MB was
to optimize for Xen's slab allocator, it had nothing to do with
IO_TLB_MIN_SLABS. Since this is now common code we should not expose
Xen-specific optimizations here and smaller values will still work so
IO_TLB_MIN_SLABS is fine.
I think this should be mentioned in the commit message though, probably best in
the next patch where you switch to this code.
As far as the hunk above, I don't think we need the max() here: with
IO_TLB_MIN_SLABS being 512 we may get stuck in an infinite loop. Something like
nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
if (nslabs <= IO_TLB_MIN_SLABS)
panic()
should be sufficient.
+ goto retry;
+ }
if (swiotlb_init_with_tbl(tlb, default_nslabs, flags))
goto fail_free_mem;
return;
@@ -287,12 +300,18 @@ void __init swiotlb_init(bool addressing_limit, unsigned
int flags)
pr_warn("Cannot allocate buffer");
}
+void __init swiotlb_init(bool addressing_limit, unsigned int flags)
+{
+ return swiotlb_init_remap(addressing_limit, flags, NULL);
+}
+
/*
* Systems with larger DMA zones (those that don't support ISA) can
* initialize the swiotlb later using the slab allocator if needed.
* This should be just like above, but with some error catching.
*/
-int swiotlb_init_late(size_t size, gfp_t gfp_mask)
+int swiotlb_init_late(size_t size, gfp_t gfp_mask,
+ int (*remap)(void *tlb, unsigned long nslabs))
{
unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
unsigned long bytes;
@@ -303,6 +322,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
if (swiotlb_force_disable)
return 0;
+retry:
order = get_order(nslabs << IO_TLB_SHIFT);
nslabs = SLABS_PER_PAGE << order;
bytes = nslabs << IO_TLB_SHIFT;
@@ -317,6 +337,16 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
if (!vstart)
return -ENOMEM;
+ if (remap)
+ rc = remap(vstart, nslabs);
+ if (rc) {
+ free_pages((unsigned long)vstart, order);
+
+ if (IO_TLB_MIN_SLABS <= 1024)
+ return rc;
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
Same here. (The 'if' check above is wrong anyway).
Patches 13 and 14 look good.
-boris
+ goto retry;
+ }
if (order != get_order(bytes)) {
pr_warn("only able to allocate %ld MB\n",
Re: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap the buffer
On Mon, Mar 14, 2022 at 06:39:21PM -0400, Boris Ostrovsky wrote:
> This is IO_TLB_MIN_SLABS, isn't it? (Xen code didn't say so but that's what
> it meant to say I believe)
Yes, that makes much more sense. I've switched the patch to use
IO_TLB_MIN_SLABS and drop the 2MB comment in both places.
Can I get a review with that fixed up?
---
>From 153085bf3e6e69d676bef0fb96395a86fb8122f5 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig
Date: Mon, 14 Mar 2022 08:02:57 +0100
Subject: swiotlb: provide swiotlb_init variants that remap the buffer
To shared more code between swiotlb and xen-swiotlb, offer a
swiotlb_init_remap interface and add a remap callback to
swiotlb_init_late that will allow Xen to remap the buffer the
buffer without duplicating much of the logic.
Signed-off-by: Christoph Hellwig
---
arch/x86/pci/sta2x11-fixup.c | 2 +-
include/linux/swiotlb.h | 5 -
kernel/dma/swiotlb.c | 36 +---
3 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
index c7e6faf59a861..7368afc039987 100644
--- a/arch/x86/pci/sta2x11-fixup.c
+++ b/arch/x86/pci/sta2x11-fixup.c
@@ -57,7 +57,7 @@ static void sta2x11_new_instance(struct pci_dev *pdev)
int size = STA2X11_SWIOTLB_SIZE;
/* First instance: register your own swiotlb area */
dev_info(>dev, "Using SWIOTLB (size %i)\n", size);
- if (swiotlb_init_late(size, GFP_DMA))
+ if (swiotlb_init_late(size, GFP_DMA, NULL))
dev_emerg(>dev, "init swiotlb failed\n");
}
list_add(>list, _instance_list);
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index ee655f2e4d28b..7b50c82f84ce9 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -36,8 +36,11 @@ struct scatterlist;
int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, unsigned int flags);
unsigned long swiotlb_size_or_default(void);
+void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
+ int (*remap)(void *tlb, unsigned long nslabs));
+int swiotlb_init_late(size_t size, gfp_t gfp_mask,
+ int (*remap)(void *tlb, unsigned long nslabs));
extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
-int swiotlb_init_late(size_t size, gfp_t gfp_mask);
extern void __init swiotlb_update_mem_attributes(void);
phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 79641c446d284..b3d4f24fb5f5e 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -256,9 +256,11 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long
nslabs,
* Statically reserve bounce buffer space and initialize bounce buffer data
* structures for the software IO TLB used to implement the DMA API.
*/
-void __init swiotlb_init(bool addressing_limit, unsigned int flags)
+void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
+ int (*remap)(void *tlb, unsigned long nslabs))
{
- size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
+ unsigned long nslabs = default_nslabs;
+ size_t bytes;
void *tlb;
if (!addressing_limit && !swiotlb_force_bounce)
@@ -271,12 +273,23 @@ void __init swiotlb_init(bool addressing_limit, unsigned
int flags)
* allow to pick a location everywhere for hypervisors with guest
* memory encryption.
*/
+retry:
+ bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
if (flags & SWIOTLB_ANY)
tlb = memblock_alloc(bytes, PAGE_SIZE);
else
tlb = memblock_alloc_low(bytes, PAGE_SIZE);
if (!tlb)
goto fail;
+ if (remap && remap(tlb, nslabs) < 0) {
+ memblock_free(tlb, PAGE_ALIGN(bytes));
+
+ if (nslabs <= IO_TLB_MIN_SLABS)
+ panic("%s: Failed to remap %zu bytes\n",
+ __func__, bytes);
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
+ goto retry;
+ }
if (swiotlb_init_with_tbl(tlb, default_nslabs, flags))
goto fail_free_mem;
return;
@@ -287,12 +300,18 @@ void __init swiotlb_init(bool addressing_limit, unsigned
int flags)
pr_warn("Cannot allocate buffer");
}
+void __init swiotlb_init(bool addressing_limit, unsigned int flags)
+{
+ return swiotlb_init_remap(addressing_limit, flags, NULL);
+}
+
/*
* Systems with larger DMA zones (those that don't support ISA) can
* initialize the swiotlb later using the slab allocator if needed.
* This should be just like above, but with some error catching.
*/
-int swiotlb_init_late(size_t size, gfp_t gfp_mask)
+int swiotlb_init_late(size_t size, gfp_t gfp_mask,
+ int (*remap)(void *tlb, unsigned long nslabs))
{
unsigned
Re: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap the buffer
On 3/14/22 3:31 AM, Christoph Hellwig wrote:
-void __init swiotlb_init(bool addressing_limit, unsigned int flags)
+void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
+ int (*remap)(void *tlb, unsigned long nslabs))
{
- size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
+ unsigned long nslabs = default_nslabs;
+ size_t bytes;
void *tlb;
if (!addressing_limit && !swiotlb_force_bounce)
@@ -271,12 +273,24 @@ void __init swiotlb_init(bool addressing_limit, unsigned
int flags)
* allow to pick a location everywhere for hypervisors with guest
* memory encryption.
*/
+retry:
+ bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
if (flags & SWIOTLB_ANY)
tlb = memblock_alloc(bytes, PAGE_SIZE);
else
tlb = memblock_alloc_low(bytes, PAGE_SIZE);
if (!tlb)
goto fail;
+ if (remap && remap(tlb, nslabs) < 0) {
+ memblock_free(tlb, PAGE_ALIGN(bytes));
+
+ /* Min is 2MB */
+ if (nslabs <= 1024)
This is IO_TLB_MIN_SLABS, isn't it? (Xen code didn't say so but that's what it
meant to say I believe)
+ panic("%s: Failed to remap %zu bytes\n",
+ __func__, bytes);
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
+ goto retry;
+ }
@@ -303,6 +323,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
if (swiotlb_force_disable)
return 0;
+retry:
order = get_order(nslabs << IO_TLB_SHIFT);
nslabs = SLABS_PER_PAGE << order;
bytes = nslabs << IO_TLB_SHIFT;
@@ -317,6 +338,17 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
if (!vstart)
return -ENOMEM;
+ if (remap)
+ rc = remap(vstart, nslabs);
+ if (rc) {
+ free_pages((unsigned long)vstart, order);
+
+ /* Min is 2MB */
+ if (nslabs <= 1024)
Same here.
-boris
+ return rc;
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
+ goto retry;
+ }
if (order != get_order(bytes)) {
pr_warn("only able to allocate %ld MB\n",
