linux-kernel - Re: [PATCH v6 07/12] x86/sgx: Introduce EPC page states

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]

Date: Fri, 05 Jan 2024 19:45:23 -0600
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: jarkko@...nel.org, dave.hansen@...ux.intel.com, tj@...nel.org,
 mkoutny@...e.com, linux-kernel@...r.kernel.org, linux-sgx@...r.kernel.org,
 x86@...nel.org, cgroups@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de, hpa@...or.com, sohil.mehta@...el.com, "Dave Hansen"
 <dave.hansen@...el.com>
Cc: zhiquan1.li@...el.com, kristen@...ux.intel.com, seanjc@...gle.com,
 zhanb@...rosoft.com, anakrish@...rosoft.com, mikko.ylinen@...ux.intel.com,
 yangjie@...rosoft.com, "Sean Christopherson" <sean.j.christopherson@...el.com>
Subject: Re: [PATCH v6 07/12] x86/sgx: Introduce EPC page states

On Fri, 05 Jan 2024 11:57:03 -0600, Dave Hansen <dave.hansen@...el.com>  
wrote:

> On 10/30/23 11:20, Haitao Huang wrote:
>> @@ -527,16 +530,13 @@ void sgx_mark_page_reclaimable(struct  
>> sgx_epc_page *page)
>>  int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
>>  {
>>  	spin_lock(&sgx_global_lru.lock);
>> -	if (page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED) {
>> -		/* The page is being reclaimed. */
>> -		if (list_empty(&page->list)) {
>> -			spin_unlock(&sgx_global_lru.lock);
>> -			return -EBUSY;
>> -		}
>> -
>> -		list_del(&page->list);
>> -		page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>> +	if (sgx_epc_page_reclaim_in_progress(page->flags)) {
>> +		spin_unlock(&sgx_global_lru.lock);
>> +		return -EBUSY;
>>  	}
>> +
>> +	list_del(&page->list);
>> +	sgx_epc_page_reset_state(page);
>
> I want to know how much if this series is basically line-for-line
> abstraction shifting like:
>
> -	page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
> +	sgx_epc_page_reset_state(page);
>
> versus actually adding complexity.  That way, I might be able to offer
> some advice on where this can be pared down.  That's really hard to do
> with the current series.
>
> Please don't just "introduce new page states".  This should have first
> abstracted out the sgx_epc_page_reclaim_in_progress() operation, using
> the list_empty() check as the implementation.
>
> Then, in a separate patch, introduce the concept of the "reclaim in
> progress" flag and finally flip the implementation over.
>
> Ditto for the sgx_epc_page_reset_state() abstraction.  It should have
> been introduced separately as a concept and then have the implementation
> changed.
>
> On in to patch 10 (which is much too big) which introduces the
> sgx_lru_list() abstraction.
>

Sure. I'll try to refactor according to this plan.
Thanks
Haitao