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@xxxxxxxxxx>; Li, Zhiquan1
> > <zhiquan1.li@xxxxxxxxx>
> > Cc: linux-sgx@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; Chatre, Reinette
> > <reinette.chatre@xxxxxxxxx>; Hansen, Dave <dave.hansen@xxxxxxxxx>; Raj,
> > Ashok <ashok.raj@xxxxxxxxx>; chao.p.peng@xxxxxxxxxxxxxxx; Zhong, Yang
> > <yang.zhong@xxxxxxxxx>
> > 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@xxxxxxxxxx>
> > > Sent: Saturday, May 21, 2022 3:11 AM
> > > To: Zhang, Cathy <cathy.zhang@xxxxxxxxx>; Li, Zhiquan1
> > > <zhiquan1.li@xxxxxxxxx>
> > > Cc: linux-sgx@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; Chatre, Reinette
> > > <reinette.chatre@xxxxxxxxx>; Hansen, Dave <dave.hansen@xxxxxxxxx>;
> > > Raj, Ashok <ashok.raj@xxxxxxxxx>; chao.p.peng@xxxxxxxxxxxxxxx; Zhong,
> > > Yang <yang.zhong@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > >
> > > > ---
> > > > 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@xxxxxxxxx/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
