From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DF2D2C433EF for ; Thu, 19 May 2022 21:53:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245313AbiESVxc (ORCPT ); Thu, 19 May 2022 17:53:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57348 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S245288AbiESVxU (ORCPT ); Thu, 19 May 2022 17:53:20 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C68E25534C for ; Thu, 19 May 2022 14:53:18 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 59A27B828B4 for ; Thu, 19 May 2022 21:53:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9A652C385AA; Thu, 19 May 2022 21:53:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1652997196; bh=9c2B1syApvkTYkf5V2nwajxvJCYhtuwKbLt8UPgqw9w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=E86UHGH9h3PWElt3Azrc3pAqFtTg7MFR2SL8bGo+ArBteAKF8UuLDx4mDo2s5dWLp /dtq/toSD+4hkK49bQqHVwk6HL5PlcSKKTx5SQaXaZgA1pgKSQA0wnPSwTBtHS7Dv6 dw1f0+EFibbQMvvRQyYrcE0CELqqrmGmhenN3+TeJh02o/sE3PRw6XxrzvufIeWFdS jskgpiQbLBuido59S+1ki4EWoot0w/r6Q+591JfLw6Wc4C50Z8KzJo8xmfQcrjANIl 9vPLrpNSIb0bQiWlPUDZQbFog+Z/NUmYhYbsJv9g2X47xPEM0MhzuhqbzOp8Ow1ANo gPmbBXCHmfZ/A== Date: Fri, 20 May 2022 00:51:37 +0300 From: Jarkko Sakkinen To: Zhiquan Li Cc: linux-sgx@vger.kernel.org, tony.luck@intel.com, dave.hansen@linux.intel.com, seanjc@google.com, kai.huang@intel.com, fan.du@intel.com Subject: Re: [PATCH v2 2/4] x86/sgx: add struct sgx_vepc_page to manage EPC pages for vepc Message-ID: References: <20220519031137.245767-1-zhiquan1.li@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220519031137.245767-1-zhiquan1.li@intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-sgx@vger.kernel.org On Thu, May 19, 2022 at 11:11:37AM +0800, Zhiquan Li wrote: > Current SGX data structures are insufficient to track the EPC pages for > vepc. For example, if we want to retrieve the virtual address of an EPC > page allocated to an enclave on host, we can find this info from its > owner, the 'desc' field of struct sgx_encl_page. However, if the EPC > page is allocated to a KVM guest, this is not available, as their owner > is a shared vepc. Interesting, but this does not explain what is the motivation to make this information available for consumption. > So, we introduce struct sgx_vepc_page which can be the owner of EPC > pages for vepc and saves the useful info of EPC pages for vepc, like > struct sgx_encl_page. Instead of "we introduce", write "introduce". The actions taken should be always in imperative form. > Canonical memory failure collects victim tasks by iterating all the > tasks one by one and use reverse mapping to get victim tasks' virtual > address. > > This is not necessary for SGX - as one EPC page can be mapped to ONE > enclave only. So, this 1:1 mapping enforcement allows us to find task > virtual address with physical address directly. Even though an enclave > has been shared by multiple processes, the virtual address is the same. > Isn't the point put in simple terms that in order to inject #MC, the virtual address is required. Then, arch_memory_failure() can easily retrieve it. This is the motivation to do this, and it's completely missing from above, not a single word about it. I would delete most of it and focus on motivation and application. > Signed-off-by: Zhiquan Li > --- > Changes since V1: > - Add documentation suggested by Jarkko. > - Revise the commit message. > --- > arch/x86/kernel/cpu/sgx/sgx.h | 15 +++++++++++++++ > arch/x86/kernel/cpu/sgx/virt.c | 24 +++++++++++++++++++----- > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index ad3b455ed0da..9a4292168389 100644 > --- a/arch/x86/kernel/cpu/sgx/sgx.h > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > @@ -28,6 +28,8 @@ > > /* Pages on free list */ > #define SGX_EPC_PAGE_IS_FREE BIT(1) > +/* Pages is used by VM guest */ > +#define SGX_EPC_PAGE_IS_VEPC BIT(2) > > struct sgx_epc_page { > unsigned int section; > @@ -114,4 +116,17 @@ struct sgx_vepc { > struct mutex lock; > }; > > +/** > + * struct sgx_vepc_page - SGX virtual EPC page structure > + * @vaddr: the virtual address when the EPC page was mapped > + * @vepc: the owner of the virtual EPC page > + * > + * When a virtual EPC page is allocated to guest, we use this structure > + * to track the associated information on host, like struct sgx_encl_page. > + */ > +struct sgx_vepc_page { > + unsigned long vaddr; > + struct sgx_vepc *vepc; > +}; > + > #endif /* _X86_SGX_H */ > diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c > index c9c8638b5dc4..d7945a47ced8 100644 > --- a/arch/x86/kernel/cpu/sgx/virt.c > +++ b/arch/x86/kernel/cpu/sgx/virt.c > @@ -29,6 +29,7 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc, > struct vm_area_struct *vma, unsigned long addr) > { > struct sgx_epc_page *epc_page; > + struct sgx_vepc_page *owner; > unsigned long index, pfn; > int ret; > > @@ -41,13 +42,22 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc, > if (epc_page) > return 0; > > - epc_page = sgx_alloc_epc_page(vepc, false); > - if (IS_ERR(epc_page)) > - return PTR_ERR(epc_page); > + owner = kzalloc(sizeof(*owner), GFP_KERNEL); > + if (!owner) > + return -ENOMEM; > + owner->vepc = vepc; > + owner->vaddr = addr & PAGE_MASK; By addr would have lower 12 bits set, isn't it a page address? > + > + epc_page = sgx_alloc_epc_page(owner, false); > + if (IS_ERR(epc_page)) { > + ret = PTR_ERR(epc_page); > + goto err_free_owner; > + } > + epc_page->flags = SGX_EPC_PAGE_IS_VEPC; > > ret = xa_err(xa_store(&vepc->page_array, index, epc_page, GFP_KERNEL)); > if (ret) > - goto err_free; > + goto err_free_page; > > pfn = PFN_DOWN(sgx_get_epc_phys_addr(epc_page)); > > @@ -61,8 +71,10 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc, > > err_delete: > xa_erase(&vepc->page_array, index); > -err_free: > +err_free_page: > sgx_free_epc_page(epc_page); > +err_free_owner: > + kfree(owner); > return ret; > } > > @@ -122,6 +134,7 @@ static int sgx_vepc_remove_page(struct sgx_epc_page *epc_page) > > static int sgx_vepc_free_page(struct sgx_epc_page *epc_page) > { > + struct sgx_vepc_page *owner = (struct sgx_vepc_page *)epc_page->owner; > int ret = sgx_vepc_remove_page(epc_page); > if (ret) { > /* > @@ -141,6 +154,7 @@ static int sgx_vepc_free_page(struct sgx_epc_page *epc_page) > return ret; > } > > + kfree(owner); > sgx_free_epc_page(epc_page); > return 0; > } > -- > 2.25.1 > I think this patch set is going taking wrong approach. Instead it would be better just to re-use struct sgx_encl_page. Things will get out of hands, if we have a struct of each flavor of the same entity. Additionally, it will take virtualization code a step closer to be feasible for the page reclaimer consumption. BR, Jarkko