On Wed, Oct 19, 2022 at 12:14:10PM -0700, Haitao Huang wrote:
> Change return type so it can be reused later for fops->fadvise
Does not mention the change in return values or reasoning for that.
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/encl.c | 46 ++++++++++++++++++++++------------
> arch/x86/kernel/cpu/sgx/encl.h | 3 ++-
> 2 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 8bdeae2fc309..c57e60d5a0aa 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -305,11 +305,11 @@ struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> * on a SGX2 system then the EPC can be added dynamically via the SGX2
> * ENCLS[EAUG] instruction.
> *
> - * Returns: Appropriate vm_fault_t: VM_FAULT_NOPAGE when PTE was installed
> - * successfully, VM_FAULT_SIGBUS or VM_FAULT_OOM as error otherwise.
> + * Returns: 0 when PTE was installed successfully, -EBUSY for waiting on
> + * reclaimer to free EPC, -ENOMEM for out of RAM, -EFAULT as error otherwise.
> */
> -static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
- struct sgx_encl *encl, unsigned long addr)
> +int sgx_encl_eaug_page(struct vm_area_struct *vma,
> + struct sgx_encl *encl, unsigned long addr)
I'd prefer export changes to be separated to their own commits.
> {
> vm_fault_t vmret = VM_FAULT_SIGBUS;
> struct sgx_pageinfo pginfo = {0};
> @@ -318,10 +318,10 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
> struct sgx_va_page *va_page;
> unsigned long phys_addr;
> u64 secinfo_flags;
> - int ret;
> + int ret = -EFAULT;
>
> if (!test_bit(SGX_ENCL_INITIALIZED, &encl->flags))
> - return VM_FAULT_SIGBUS;
> + return -EFAULT;
>
> /*
> * Ignore internal permission checking for dynamically added pages.
> @@ -332,21 +332,21 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
> secinfo_flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
> encl_page = sgx_encl_page_alloc(encl, addr - encl->base, secinfo_flags);
> if (IS_ERR(encl_page))
> - return VM_FAULT_OOM;
> + return -ENOMEM;
>
> mutex_lock(&encl->lock);
>
> epc_page = sgx_alloc_epc_page(encl_page, false);
> if (IS_ERR(epc_page)) {
> if (PTR_ERR(epc_page) == -EBUSY)
> - vmret = VM_FAULT_NOPAGE;
> + ret = -EBUSY;
> goto err_out_unlock;
> }
>
> va_page = sgx_encl_grow(encl, false);
> if (IS_ERR(va_page)) {
> if (PTR_ERR(va_page) == -EBUSY)
> - vmret = VM_FAULT_NOPAGE;
> + ret = -EBUSY;
> goto err_out_epc;
> }
>
> @@ -359,16 +359,20 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
> * If ret == -EBUSY then page was created in another flow while
> * running without encl->lock
> */
> - if (ret)
> + if (ret) {
> + ret = -EFAULT;
> goto err_out_shrink;
> + }
>
> pginfo.secs = (unsigned long)sgx_get_epc_virt_addr(encl->secs.epc_page);
> pginfo.addr = encl_page->desc & PAGE_MASK;
> pginfo.metadata = 0;
>
> ret = __eaug(&pginfo, sgx_get_epc_virt_addr(epc_page));
> - if (ret)
> + if (ret) {
> + ret = -EFAULT;
> goto err_out;
> + }
>
> encl_page->encl = encl;
> encl_page->epc_page = epc_page;
> @@ -385,10 +389,10 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
> vmret = vmf_insert_pfn(vma, addr, PFN_DOWN(phys_addr));
> if (vmret != VM_FAULT_NOPAGE) {
> mutex_unlock(&encl->lock);
> - return VM_FAULT_SIGBUS;
> + return -EFAULT;
> }
> mutex_unlock(&encl->lock);
> - return VM_FAULT_NOPAGE;
> + return 0;
>
> err_out:
> xa_erase(&encl->page_array, PFN_DOWN(encl_page->desc));
> @@ -401,7 +405,7 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
> mutex_unlock(&encl->lock);
> kfree(encl_page);
>
> - return vmret;
> + return ret;
> }
>
> static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
> @@ -431,8 +435,18 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
> * enclave that will be checked for right away.
> */
> if (cpu_feature_enabled(X86_FEATURE_SGX2) &&
> - (!xa_load(&encl->page_array, PFN_DOWN(addr))))
> - return sgx_encl_eaug_page(vma, encl, addr);
> + (!xa_load(&encl->page_array, PFN_DOWN(addr)))) {
> + switch (sgx_encl_eaug_page(vma, encl, addr)) {
> + case 0:
> + case -EBUSY:
> + return VM_FAULT_NOPAGE;
> + case -ENOMEM:
> + return VM_FAULT_OOM;
> + case -EFAULT:
> + default:
> + return VM_FAULT_SIGBUS;
> + }
> + }
I.e. why it is better to change working code like this, and not
instead add such conversion to the fadvise handler.
No need to response here. This should be explained in the commit
message.
>
> mutex_lock(&encl->lock);
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index a65a952116fd..36059d35e1bc 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -127,5 +127,6 @@ struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> unsigned long addr);
> struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl, bool reclaim);
> void sgx_encl_shrink(struct sgx_encl *encl, struct sgx_va_page *va_page);
> -
> +int sgx_encl_eaug_page(struct vm_area_struct *vma,
> + struct sgx_encl *encl, unsigned long addr);
> #endif /* _X86_ENCL_H */
> --
> 2.25.1
>
BR, Jarkko
