Re: [PATCH v5 3/9] x86/sgx: Keep record for SGX VA and Guest page type

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, May 23, 2022 at 06:09:55AM +0000, Zhang, Cathy wrote:
> Hi Jarkko,
> 
> > -----Original Message-----
> > From: Zhang, Cathy
> > Sent: Monday, May 23, 2022 8:07 AM
> > To: 'Jarkko Sakkinen' <jarkko@kernel.org>; Li, Zhiquan1
> > <zhiquan1.li@intel.com>
> > Cc: linux-sgx@vger.kernel.org; x86@kernel.org; Chatre, Reinette
> > <reinette.chatre@intel.com>; Hansen, Dave <dave.hansen@intel.com>; Raj,
> > Ashok <ashok.raj@intel.com>; chao.p.peng@linux.intel.com; Zhong, Yang
> > <yang.zhong@intel.com>
> > Subject: RE: [PATCH v5 3/9] x86/sgx: Keep record for SGX VA and Guest page
> > type
> > 
> > Hi Jarkko,
> > 
> > > -----Original Message-----
> > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > Sent: Saturday, May 21, 2022 3:11 AM
> > > To: Zhang, Cathy <cathy.zhang@intel.com>; Li, Zhiquan1
> > > <zhiquan1.li@intel.com>
> > > Cc: linux-sgx@vger.kernel.org; x86@kernel.org; Chatre, Reinette
> > > <reinette.chatre@intel.com>; Hansen, Dave <dave.hansen@intel.com>;
> > > Raj, Ashok <ashok.raj@intel.com>; chao.p.peng@linux.intel.com; Zhong,
> > > Yang <yang.zhong@intel.com>
> > > Subject: Re: [PATCH v5 3/9] x86/sgx: Keep record for SGX VA and Guest
> > > page type
> > >
> > > On Fri, May 20, 2022 at 06:38:58PM +0800, Cathy Zhang wrote:
> > > > Regular enclave EPC pages have sgx_encl_page as their owner, but SGX
> > > > VA page and KVM guest EPC page are maintained by different owner
> > > > structures.
> > > >
> > > > SGX CPUSVN update requires to know the EPC page owner's status and
> > > > then decide how to handle the page.
> > > >
> > > > Keep a record of page type for SGX VA and KVM guest page while the
> > > > other EPC pages already have their type tracked, so that CPUSVN
> > > > update can get EPC page's owner by type and handle it then.
> > > >
> > > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > > >
> > > > ---
> > > > Changes since v3:
> > > >  - Rename SGX_EPC_PAGE_GUEST as SGX_EPC_PAGE_KVM_GUEST.
> > > (Suggested by
> > > >    Jarkko, Sakkinen)
> > > > ---
> > > >  arch/x86/kernel/cpu/sgx/sgx.h  | 4 ++++
> > > > arch/x86/kernel/cpu/sgx/encl.c | 2 ++
> > > > arch/x86/kernel/cpu/sgx/virt.c
> > > > | 2 ++
> > > >  3 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> > > > b/arch/x86/kernel/cpu/sgx/sgx.h index f8ed9deac18b..4ad0e5396eef
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > > > @@ -28,6 +28,10 @@
> > > >
> > > >  /* Pages on free list */
> > > >  #define SGX_EPC_PAGE_IS_FREE		BIT(1)
> > > > +/* VA page */
> > > > +#define SGX_EPC_PAGE_VA			BIT(2)
> > > > +/* Pages allocated for KVM guest */
> > > > +#define SGX_EPC_PAGE_KVM_GUEST		BIT(3)
> > > >
> > > >  struct sgx_epc_page {
> > > >  	unsigned int section;
> > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> > > > b/arch/x86/kernel/cpu/sgx/encl.c index 08f39fa03a39..383abd538ac9
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > > > @@ -912,6 +912,8 @@ struct sgx_epc_page *sgx_alloc_va_page(struct
> > > sgx_va_page *va_page)
> > > >  		return ERR_PTR(-EFAULT);
> > > >  	}
> > > >
> > > > +	epc_page->flags |= SGX_EPC_PAGE_VA;
> > > > +
> > > >  	return epc_page;
> > > >  }
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/sgx/virt.c
> > > > b/arch/x86/kernel/cpu/sgx/virt.c index e953816d7c8b..104487b72fb8
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > > > @@ -50,6 +50,8 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc,
> > > >  	if (IS_ERR(epc_page))
> > > >  		return PTR_ERR(epc_page);
> > > >
> > > > +	epc_page->flags |= SGX_EPC_PAGE_KVM_GUEST;
> > >
> > > This would need to be synced up with SGX_EPC_IS_VEPC:
> > >
> > > https://lore.kernel.org/linux-sgx/694234d7-6a0d-e85f-f2f9-
> > > e52b4a61e1ec@intel.com/T/#t
> > >
> > > Otherwise, we have a chaos.
> > >
> > > Can you sync up with Zhiquan Li and find some common approach. IMHO,
> > > it might even sense to merge these patch sets into one. Then it would
> > > be easier to review them as a whole.
> > 
> > Thanks for showing the above related patches, Jarkko! I just take a quick look
> > at that series, AFAIK, __sgx_vepc_fault() will only be called during VM boot
> > up to allocate physical EPC pages.
> > Then, host does not know how VM works with those pages. VM SGX driver
> > will handle any errors itself.
> > So with the approach in that series, what it actually tracks is the vaddr in
> > QEMU address space for all EPC pages allocated to the VM. Does host need
> > to track such information? QEMU side should provide it easily I think. @Li,
> > Zhiquan1.
> > 
> 
> I've synced with Zhiquan Li and got the whole picture of his patch logic. I suggest
> to follow the way how we handle the same change between EDMM and Seamless,
> that is Zhiquan Li will apply patch 3 from Seamless and then add his left patches.
> Please let's know if you have other suggestion. Thanks!

Sounds reasonable!

BR, Jarkko