On 8/18/23 07:42, Devaraj Rangasamy wrote:
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1223,6 +1223,8 @@ void __init setup_arch(char **cmdline_p)
> initmem_init();
> dma_contiguous_reserve(max_pfn_mapped << PAGE_SHIFT);
>
> + amdtee_cma_reserve();
> +
> if (boot_cpu_has(X86_FEATURE_GBPAGES))
> hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT);
Right now, we have *A* global CMA pool set up in
dma_contiguous_reserve() that everyone shares.
Why does this *one* driver deserve to be a special snowflake and get its
own private CMA area and own command-line options?
It seems to me like you should just tell users to set
CONFIG_CMA_SIZE_MBYTES or use cma=size on the command-line and just use
the global pool. If you want to make it a special snowflake, there's a
much higher bar to clear, and there's zero justification for that right now.
Oh, and this:
> static int pool_op_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
> size_t size, size_t align)
> {
> unsigned int order = get_order(size);
> unsigned long va;
> int rc;
>
> /*
> * Ignore alignment since this is already going to be page aligned
> * and there's no need for any larger alignment.
> */
> va = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
is goofy. It either only needs powers-of-2 and can take "order" as an
argument instead of 'size', or it should be using alloc_pages_exact() to
avoid wasting memory.
