Re: [PATCH v6 6/8] dma: swiotlb: Centralize memory-encryption pool sizing

[Kohei Enju <enju.kohei@fujitsu.com>]

Hi Aneesh,

On 11/01 09:47, Aneesh Kumar K.V wrote:
[...]
> >
> > Hi Aneesh,
> >
> > I can reproduce the CCA regression discussed in this thread:
> >   https://lore.kernel.org/all/yq5azeyxyfol.fsf@kernel.org/#t
> >
> > The guest runs out of SWIOTLB slots, causing disk I/O to fail and
> > preventing the guest from booting.
> >
> > I would like to test the fix on real CCA hardware. Could you please Cc
> > me when you respin the series? I follow LKML, but I may still miss the
> > posting by chance.
> >
> 
> The conclusion was to either use CONFIG_SWIOTLB_DYNAMIC or swiotlb kernel command line
> option

Thanks for the clarification.

After reading this whole thread, I agree that dynamic sizing would be
appropriate.

However, currently some distributions do not enable
CONFIG_SWIOTLB_DYNAMIC. Users of such distributions would therefore need
to rebuild their kernels with the option enabled. This is not a
practical solution for many users, particularly cloud users who use
kernels provided by their distributions or cloud providers.

Specifying swiotlb= on the kernel command line is also not particularly
user-friendly. It requires users or cloud providers to choose a fixed
value in advance, without automatically taking into account properties
of the VM, such as its memory size or whether it is a confidential
guest.

I do not think the existing heuristic used for x86 CoCo guests is
necessarily optimal, but a fixed swiotlb= value chosen by each user may
be even less appropriate.

Overall, I would prefer the dynamic sizing approach. However, I believe
the initial pool size still matters.

Also, as Catalin pointed out earlier in the following message:

    https://lore.kernel.org/all/ann6noRzoPLL9kDY@arm.com/

the arm64 adjustment also reduces the target size used for dynamic
pool allocation, since both the initial pool and dynamic allocation
currently use default_nslabs.

I am testing the patch below to decouple the initial pool size from the
dynamic allocation target. It adds swiotlb_adjust_initial_size() for
callers that only want to reduce the initial allocation, while keeping
the existing swiotlb_adjust_size() semantics for callers that want to
adjust both sizes.

Thanks,
Kohei

--->8---
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 4e7682dfe366..80a2c171895f 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -349,7 +349,7 @@ void __init arch_mm_preinit(void)
                unsigned long size =
                        DIV_ROUND_UP(memblock_phys_mem_size(), 1024);

-               swiotlb_adjust_size(min(swiotlb_default_pool_size(), size));
+               swiotlb_adjust_initial_size(min(swiotlb_default_pool_size(), size));
        }

        swiotlb_init(true, flags);
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 070ee69eabb3..7dd9fcec90a5 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -189,7 +189,7 @@ void __init arch_mm_preinit(void)
                 */
                unsigned long size =
                        DIV_ROUND_UP(memblock_phys_mem_size(), 1024);
-               swiotlb_adjust_size(min(swiotlb_default_pool_size(), size));
+               swiotlb_adjust_initial_size(min(swiotlb_default_pool_size(), size));
                swiotlb = true;
                swiotlb_flags |= SWIOTLB_ANY;
        }
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 5f24edf47b68..c1984ee12355 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -190,6 +190,7 @@ size_t swiotlb_max_mapping_size(struct device *dev);
 bool is_swiotlb_allocated(void);
 bool is_swiotlb_active(struct device *dev);
 void __init swiotlb_adjust_size(unsigned long size);
+void __init swiotlb_adjust_initial_size(unsigned long size);
 phys_addr_t default_swiotlb_base(void);
 phys_addr_t default_swiotlb_limit(void);
 #else
@@ -232,6 +233,10 @@ static inline void swiotlb_adjust_size(unsigned long size)
 {
 }

+static inline void swiotlb_adjust_initial_size(unsigned long size)
+{
+}
+
 static inline phys_addr_t default_swiotlb_base(void)
 {
        return 0;
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 2c57b49ac7c0..211fefb32fb5 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -99,6 +99,7 @@ static struct io_tlb_mem io_tlb_default_mem;
 #endif /* CONFIG_SWIOTLB_DYNAMIC */

 static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
+static unsigned long dynamic_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
 static unsigned long default_nareas;
 static bool swiotlb_cmdline_size_set __initdata;

@@ -152,6 +153,8 @@ static void swiotlb_adjust_nareas(unsigned int nareas)
                nareas = roundup_pow_of_two(nareas);

        default_nareas = nareas;
+       dynamic_nslabs =
+               swiotlb_calc_nslabs(dynamic_nslabs << IO_TLB_SHIFT, nareas);

        pr_info("area num %d.\n", nareas);
        nslabs = swiotlb_calc_nslabs(default_nslabs << IO_TLB_SHIFT, nareas);
@@ -254,6 +257,7 @@ setup_io_tlb_npages(char *str)
                /* avoid tail segment of size < IO_TLB_SEGSIZE */
                default_nslabs =
                        ALIGN(simple_strtoul(str, &str, 0), IO_TLB_SEGSIZE);
+               dynamic_nslabs = default_nslabs;
                swiotlb_cmdline_size_set = true;
        }
        if (*str == ',')
@@ -290,7 +294,7 @@ unsigned long swiotlb_default_pool_size(void)
        return default_nslabs << IO_TLB_SHIFT;
 }

-void __init swiotlb_adjust_size(unsigned long size)
+void __init swiotlb_adjust_initial_size(unsigned long size)
 {
        /*
         * If swiotlb parameter has not been specified, give a chance to
@@ -305,6 +309,12 @@ void __init swiotlb_adjust_size(unsigned long size)
        pr_info("SWIOTLB bounce buffer size adjusted to %luMB", size >> 20);
 }

+void __init swiotlb_adjust_size(unsigned long size)
+{
+       swiotlb_adjust_initial_size(size);
+       dynamic_nslabs = default_nslabs;
+}
+
 void swiotlb_print_info(void)
 {
        struct io_tlb_pool *mem = &io_tlb_default_mem.defpool;
@@ -507,6 +517,7 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
                pr_info("SWIOTLB bounce buffer size adjusted %lu -> %lu slabs",
                        default_nslabs, nslabs);
                default_nslabs = nslabs;
+               dynamic_nslabs = default_nslabs;
        }

        alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
@@ -889,7 +900,7 @@ static void swiotlb_dyn_alloc(struct work_struct *work)
                container_of(work, struct io_tlb_mem, dyn_alloc);
        struct io_tlb_pool *pool;

-       pool = swiotlb_alloc_pool(NULL, mem, IO_TLB_MIN_SLABS, default_nslabs,
+       pool = swiotlb_alloc_pool(NULL, mem, IO_TLB_MIN_SLABS, dynamic_nslabs,
                                  default_nareas, mem->phys_limit, GFP_KERNEL);
        if (!pool) {
                pr_warn_ratelimited("Failed to allocate new pool");