linux-kernel - Re: [PATCH] tee: amdtee: add support for use of cma region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b04ac070-2149-4d61-ad90-75401e173b29@amd.com>
Date:   Tue, 29 Aug 2023 22:17:06 +0530
From:   Devaraj Rangasamy <Devaraj.Rangasamy@....com>
To:     Dave Hansen <dave.hansen@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Jens Wiklander <jens.wiklander@...aro.org>,
        Sumit Garg <sumit.garg@...aro.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Daniel Sneddon <daniel.sneddon@...ux.intel.com>,
        Rijo Thomas <Rijo-john.Thomas@....com>,
        SivaSangeetha SK <SivaSangeetha.SK@....com>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Juergen Gross <jgross@...e.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Ross Lagerwall <ross.lagerwall@...rix.com>,
        Yuntao Wang <ytcoode@...il.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Tom Lendacky <thomas.lendacky@....com>,
        Mario Limonciello <mario.limonciello@....com>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        op-tee@...ts.trustedfirmware.org
Cc:     Mythri PK <Mythri.Pandeshwarakrishna@....com>,
        Nimesh Easow <Nimesh.Easow@....com>
Subject: Re: [PATCH] tee: amdtee: add support for use of cma region


On 8/18/2023 8:56 PM, Dave Hansen wrote:
> 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.

Ack.

We had exclusive cma zone due to concern over contention for generic cma 
zone, and to guarentee buffer allocation. We profiled and migrated to 
generic cma zone.

> 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.

Ack.
This was inherently introduced by __get_free_pages() requirement, and 
constly indeed.
alloc_pages_exact() do call __get_free_pages() internally, and doesnot 
prevent memory fragmentation. but at least unnecessary pages are 
returned back to free pool.

We will update the section accordingly, and post updated patch. Thanks 
for the review.