Re: [PATCH v5 06/18] x86/sgx: Introduce EPC page states

From: "Huang, Kai" <kai.huang@intel.com>
To: "tj@kernel.org" <tj@kernel.org>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"bp@alien8.de" <bp@alien8.de>,
	"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"Mehta, Sohil" <sohil.mehta@intel.com>
Cc: "kristen@linux.intel.com" <kristen@linux.intel.com>,
	"Zhang, Bo" <zhanb@microsoft.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"anakrish@microsoft.com" <anakrish@microsoft.com>,
	"yangjie@microsoft.com" <yangjie@microsoft.com>
Subject: Re: [PATCH v5 06/18] x86/sgx: Introduce EPC page states
Date: Wed, 4 Oct 2023 21:05:44 +0000	[thread overview]
Message-ID: <128730d9c552cccea3f94228b79dd546b6504dea.camel@intel.com> (raw)
In-Reply-To: <op.2caqfhkawjvjmi@hhuan26-mobl.amr.corp.intel.com>

On Wed, 2023-10-04 at 10:24 -0500, Haitao Huang wrote:
> On Tue, 03 Oct 2023 15:03:48 -0500, Huang, Kai <kai.huang@intel.com> wrote:
> 
> > On Mon, 2023-10-02 at 23:49 -0500, Haitao Huang wrote:
> > > On Wed, 27 Sep 2023 05:28:36 -0500, Huang, Kai <kai.huang@intel.com>  
> > > wrote:
> > > 
> > > > On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> > > > > Use the lower 3 bits in the flags field of sgx_epc_page struct to
> > > > > track EPC states in its life cycle and define an enum for possible
> > > > > states. More state(s) will be added later.
> > > > 
> > > > This patch does more than what the changelog claims to do.  AFAICT it
> > > > does
> > > > below:
> > > > 
> > > >  1) Use the lower 3 bits to track EPC page status
> > > >  2) Rename SGX_EPC_PAGE_RECLAIMER_TRACKED to SGX_EPC_PAGE_RERCLAIMABLE
> > > >  3) Introduce a new state SGX_EPC_PAGE_UNRECLAIMABLE
> > > >  4) Track SECS and VA pages as SGX_EPC_PAGE_UNRECLAIMABLE
> > > > 
> > > > The changelog only says 1) IIUC.
> > > > 
> > > I don't quite get why you would view 3) as a separate item from 1).
> > 
> > 1) is about using some method to track EPC page status, 3) is adding a  
> > new
> > state.
> > 
> > Why cannot they be separated?
> > 
> > > In my view, 4) is not done as long as there is not separate list to  
> > > track
> > > it.
> > 
> > You are literally doing below:
> > 
> > @@ -113,6 +113,9 @@ static int sgx_encl_create(struct sgx_encl *encl,  
> > struct
> > sgx_secs *secs)
> >  	encl->attributes = secs->attributes;
> >  	encl->attributes_mask = SGX_ATTR_UNPRIV_MASK;
> > +	sgx_record_epc_page(encl->secs.epc_page,
> > +			    SGX_EPC_PAGE_UNRECLAIMABLE);
> > +
> > 
> > Which obviously is tracking SECS as unreclaimable page here.
> > 
> > The only thing you are not doing now is to put to the actual list, which  
> > you
> > introduced in a later patch.
> > 
> > But why not just doing them together?
> > 
> > 
> I see where the problem is now.  Initially these states are bit masks so  
> UNTRACKED and UNRECLAIMABLE are all not masked (set zero). I'll change  
> these "record" calls with UNTRACKED instead, and later replace with  
> UNRECLAIMABLE when they are actually added to the list. So UNRECLAIMABLE  
> state can also be delayed until that patch with the list added.

I am not sure whether I am following, but could we just delay introducing the
"untracked" or "unreclaimable" until the list is added?

Why do we need to call sgx_record_epc_page() for SECS and VA pages in _this_
patch?

Reading again, I _think_ the reason why you added these new states because you
want to justify using the low 3 bits as EPC page states, i.e., below code ...

	+#define SGX_EPC_PAGE_STATE_MASK GENMASK(2, 0)

But for now we only have two valid states:

	- SGX_EPC_PAGE_IS_FREE
	- SGX_EPC_PAGE_RECLAIMER_TRACKED

Thus you added two more states: NOT_TRACKED/UNRECLAIMABLE.  And more
confusingly, you added calling sgx_record_epc_page() for SECS and VA pages in
this patch to try to actually use these new states, while the changelog says:

	Use the lower 3 bits in the flags field of sgx_epc_page struct to
	track EPC states in its life cycle and define an enum for possible
	states. More state(s) will be added later.

... which doesn't mention any of above.

But this doesn't stand either because you only need 2 bits for the four states
but not 3 bits.  So I don't see how adding the new states could help here.

So I would suggest two options:

1) 

In this patch, you only change the way to track EPC states to reflect your
changelog of this patch (maybe you can add NOT_TRACKED, but I am not sure /
don't care, just give a justification if you do).

And then you have a patch to introduce the new unreclaimable list, the new EPC
state, and call sgx_record_epc_page() *AND* sgx_drop_epc_page() for SECS/VA
pages.  The patch could be titled such as:

	x86/sgx: Store SECS/VA pages in the unreclaimable list

2)

You do the opposite to option 1): Introduce the patch 

	x86/sgx: Store SECS/VA pages in the unreclaimable list

... first, and then convert the EPC states to using the lower 3 bits (actually 2
bits is enough, you can extend to 3 bits in later patch when needed).

Does above make more sense?