Re: [PATCH v5 09/18] x86/sgx: Store struct sgx_encl when allocating new VA pages

From: "Huang, Kai" <kai.huang@intel.com>
To: "mingo@redhat.com" <mingo@redhat.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"bp@alien8.de" <bp@alien8.de>,
	"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"tj@kernel.org" <tj@kernel.org>,
	"Mehta, Sohil" <sohil.mehta@intel.com>
Cc: "kristen@linux.intel.com" <kristen@linux.intel.com>,
	"anakrish@microsoft.com" <anakrish@microsoft.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"yangjie@microsoft.com" <yangjie@microsoft.com>,
	"Zhang, Bo" <zhanb@microsoft.com>
Subject: Re: [PATCH v5 09/18] x86/sgx: Store struct sgx_encl when allocating new VA pages
Date: Wed, 27 Sep 2023 21:21:19 +0000	[thread overview]
Message-ID: <fd2049b46a2e508a90006731a5d0cd2b90db9e45.camel@intel.com> (raw)
In-Reply-To: <op.2bxr9aj7wjvjmi@hhuan26-mobl.amr.corp.intel.com>

On Wed, 2023-09-27 at 10:35 -0500, Haitao Huang wrote:
> > > +
> > > +	/* Possible owner types */
> > > +	union {
> > > +		struct sgx_encl_page *encl_page;
> > > +		struct sgx_encl *encl;
> > > +	};
> > 
> > Sadly for virtual EPC page the owner is set to the 'sgx_vepc' instance it
> > belongs to.
> > 
> > Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page,  
> > perhaps we
> > should do below?
> > 
> >  	union {
> >  		struct sgx_encl_page *encl_page;
> >  		struct sgx_encl *encl;
> >  		struct sgx_vepc *vepc;
> >  		void *owner;
> >  	};
> > 
> > And in sgx_{alloc|free}_epc_page() we can use 'owner' instead.
> > 
> 
> As I mentioned in cover letter and change log in 11/18, this series does  
> not track virtual EPC.

It's not about how does the cover letter says.  We cannot ignore the fact that
currently virtual EPC uses owner too.

But given the virtual EPC code currently doesn't use the owner, I can live with
not having the 'vepc' member in the union now.

> We can add vepc field into the union in future if such tracking is needed.  
> Don't think "void *owner" is needed though.

As mentioned, using 'encl_page' arbitrarily in sgx_alloc_epc_page() doesn't look
nice.  Do you have example in the current kernel code to prove it is acceptable?