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!
> >
> > BR, Jarkko
