Re: [RFC PATCH v2 1/1] x86/sgx: Explicitly give up the CPU in EDMM's ioctl() to avoid softlockup

[Bojun Zhu]

Hi Dave,

Appreciate for your review!

> On Apr 27, 2024, at 01:06, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
> 
> On 4/26/24 07:18, Bojun Zhu wrote:
>> 	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
>> +		if (sgx_check_signal_and_resched()) {
>> +			if (!c)
>> +				ret = -ERESTARTSYS;
>> +
>> +			goto out;
>> +		}
> 
> This construct is rather fugly.  Let's not perpetuate it, please.  Why
> not do:
> 
> 	int ret = -ERESTARTSYS;
> 
> 	...
> 	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> 		if (sgx_check_signal_and_resched())
> 			goto out;
> 
> Then, voila, when c==0 on the first run through the loop, you'll get a
> ret=-ERESTARTSYS.
> 

Okay, I will refine it later.

> But honestly, it seems kinda silly to annotate all these loops with
> explicit cond_resched()s.  I'd much rather do this once and, for
> instance, just wrap the enclave locks:
> 
> -	  mutex_lock(&encl->lock);
> +	  sgx_lock_enclave(encl);
> 
> and then have the lock function do the rescheds.  I assume that
> mutex_lock() isn't doing this generically for performance reasons.  But
> we don't care in SGX land and can just resched to our heart's content.


`mutex_lock(&encl->lock)` appears in everywhere in SGX in-tree driver.
But it seems that we only need to additionally invoke `cond_resched()` for
the sgx_enclave_{restrict_permissions | modify_types | remove_pages } 
and sgx_ioc_add_pages()’s ioctl()s. 

Shall we replace all the `mutex_lock(&encl->lock) with `sgx_lock_enclave(encl)` 
in SGX in-tree driver and then wrap reschedule operation in
`sgx_lock_enclave()` ? 

Regards,
Bojun