Received-SPF: pass (google.com: domain of linux-kernel+bounces-161682-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.100.2.1.4\))
Subject: Re: [RFC PATCH v2 1/1] x86/sgx: Explicitly give up the CPU in EDMM's
 ioctl() to avoid softlockup
From: "Bojun Zhu" <zhubojun.zbj@antgroup.com>
In-Reply-To: <30415ed3-a05a-454d-9077-c8674617f291@intel.com>
Date: Mon, 29 Apr 2024 10:23:12 +0800
Cc:  <linux-kernel@vger.kernel.org>,
   <linux-sgx@vger.kernel.org>,
  "Jarkko Sakkinen" <jarkko@kernel.org>,
   <dave.hansen@linux.intel.com>,
  "=?UTF-8?B?Q2hhdHJlLCBSZWluZXR0ZQ==?=" <reinette.chatre@intel.com>,
  "=?UTF-8?B?5YiY5Y+MKOi9qeWxuSk=?=" <ls123674@antgroup.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <F7EBCAA4-6412-42AE-9A56-36914E22B4A1@antgroup.com>
References: <20240426141823.112366-1-zhubojun.zbj@antgroup.com>
 <20240426141823.112366-2-zhubojun.zbj@antgroup.com>
 <30415ed3-a05a-454d-9077-c8674617f291@intel.com>
To: Dave Hansen <dave.hansen@intel.com>

Hi Dave,

Appreciate for your review!

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

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:
>=20
> -	  mutex_lock(&encl->lock);
> +	  sgx_lock_enclave(encl);
>=20
> 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 }=20
and sgx_ioc_add_pages()=E2=80=99s ioctl()s.=20

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

Regards,
Bojun=