Re: [PATCH v5 05/16] swiotlb: Add restricted DMA pool initialization

On Fri, Apr 23, 2021 at 7:34 PM Steven Price steven.price@arm.com wrote:

On 22/04/2021 09:14, Claire Chang wrote:

...

Add the initialization function to create restricted DMA pools from matching reserved-memory nodes.

Signed-off-by: Claire Chang tientzu@chromium.org

include/linux/device.h | 4 +++ include/linux/swiotlb.h | 3 +- kernel/dma/swiotlb.c | 80 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/include/linux/device.h b/include/linux/device.h index 38a2071cf776..4987608ea4ff 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -416,6 +416,7 @@ struct dev_links_info {

• @dma_pools: Dma pools (if dma'ble device).
• @dma_mem: Internal for coherent mem override.
• @cma_area: Contiguous memory area for dma allocations

• • @dma_io_tlb_mem: Internal for swiotlb io_tlb_mem override.
  • @archdata: For arch-specific additions.
  • @of_node: Associated device tree node.
  • @fwnode: Associated device node supplied by platform firmware.

@@ -521,6 +522,9 @@ struct device { #ifdef CONFIG_DMA_CMA struct cma *cma_area; /* contiguous memory area for dma allocations */ +#endif +#ifdef CONFIG_DMA_RESTRICTED_POOL

• struct io_tlb_mem *dma_io_tlb_mem;
  

  #endif /* arch specific additions */ struct dev_archdata archdata;

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 216854a5e513..03ad6e3b4056 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -72,7 +72,8 @@ extern enum swiotlb_force swiotlb_force;

•      range check to see if the memory was in fact allocated by this
  

•      API.
  

• @nslabs: The number of IO TLB blocks (in groups of 64) between @start and

• •       @end. This is command line adjustable via setup_io_tlb_npages.
    

• •       @end. For default swiotlb, this is command line adjustable via
    

• •       setup_io_tlb_npages.
    

  • @used: The number of used IO TLB block.
  • @list: The free list describing the number of free entries available

  •      from each index.
    

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 57a9adb920bf..ffbb8724e06c 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -39,6 +39,13 @@ #ifdef CONFIG_DEBUG_FS #include <linux/debugfs.h> #endif +#ifdef CONFIG_DMA_RESTRICTED_POOL +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_fdt.h> +#include <linux/of_reserved_mem.h> +#include <linux/slab.h> +#endif

#include <asm/io.h> #include <asm/dma.h> @@ -681,3 +688,76 @@ static int __init swiotlb_create_default_debugfs(void) late_initcall(swiotlb_create_default_debugfs);

#endif

+#ifdef CONFIG_DMA_RESTRICTED_POOL +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,

•                             struct device *dev)
  

+{

• struct io_tlb_mem *mem = rmem->priv;
  

• unsigned long nslabs = rmem->size >> IO_TLB_SHIFT;
  

• if (dev->dma_io_tlb_mem)
  

•         return 0;
  

• /* Since multiple devices can share the same pool, the private data,
  

•  * io_tlb_mem struct, will be initialized by the first device attached
  

•  * to it.
  

•  */
  

• if (!mem) {
  

•         mem = kzalloc(struct_size(mem, slots, nslabs), GFP_KERNEL);
  

•         if (!mem)
  

•                 return -ENOMEM;
  

+#ifdef CONFIG_ARM

•         if (!PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))) {
  

•                 kfree(mem);
  

•                 return -EINVAL;
  

•         }
  

+#endif /* CONFIG_ARM */

•         swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, false);
  

•         rmem->priv = mem;
  

• }
  

+#ifdef CONFIG_DEBUG_FS

• if (!io_tlb_default_mem->debugfs)
  

•         io_tlb_default_mem->debugfs =
  

•                 debugfs_create_dir("swiotlb", NULL);
  

At this point it's possible for io_tlb_default_mem to be NULL, leading to a splat.

Thanks for pointing this out.

But even then if it's not and we have the situation where debugfs==NULL then the debugfs_create_dir() here will cause a subsequent attempt in swiotlb_create_debugfs() to fail (directory already exists) leading to mem->debugfs being assigned an error value. I suspect the creation of the debugfs directory needs to be separated from io_tlb_default_mem being set.

debugfs creation should move into the if (!mem) {...} above to avoid duplication. I think having a separated struct dentry pointer for the default debugfs should be enough?

if (!debugfs) debugfs = debugfs_create_dir("swiotlb", NULL); swiotlb_create_debugfs(mem, rmem->name, debugfs);

Other than that I gave this series a go with our prototype of Arm's Confidential Computer Architecture[1] - since the majority of the guest's memory is protected from the host the restricted DMA pool allows (only) a small area to be shared with the host.

After fixing (well hacking round) the above it all seems to be working fine with virtio drivers.

Thanks,

Steve

[1] https://www.arm.com/why-arm/architecture/security-features/arm-confidential-...