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

["Zhang, Cathy" <cathy.zhang@intel.com>]

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!

> >
> > BR, Jarkko