Re: [PATCH] x86/sgx: Replace kmap/kunmap_atomic calls

[Jarkko Sakkinen <jarkko@kernel.org>]

On Fri, Oct 07, 2022 at 08:23:03AM -0700, Ira Weiny wrote:
> On Thu, Sep 29, 2022 at 09:06:46AM -0700, Kristen Carlson Accardi wrote:
> > It is not necessary to disable page faults or preemption when
> > using kmap calls, so replace kmap_atomic() and kunmap_atomic()
> > calls with more the more appropriate kmap_local_page() and
> > kunmap_local() calls.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> Minor comment below.
> 
> > ---
> >  arch/x86/kernel/cpu/sgx/encl.c  | 12 ++++++------
> >  arch/x86/kernel/cpu/sgx/ioctl.c |  4 ++--
> >  arch/x86/kernel/cpu/sgx/main.c  |  8 ++++----
> >  3 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index f40d64206ded..63dd92bd3288 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -160,8 +160,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> >  		return ret;
> >  
> >  	pginfo.addr = encl_page->desc & PAGE_MASK;
> > -	pginfo.contents = (unsigned long)kmap_atomic(b.contents);
> > -	pcmd_page = kmap_atomic(b.pcmd);
> > +	pginfo.contents = (unsigned long)kmap_local_page(b.contents);
> > +	pcmd_page = kmap_local_page(b.pcmd);
> >  	pginfo.metadata = (unsigned long)pcmd_page + b.pcmd_offset;
> >  
> >  	if (secs_page)
> > @@ -187,8 +187,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> >  	 */
> >  	pcmd_page_empty = !memchr_inv(pcmd_page, 0, PAGE_SIZE);
> >  
> > -	kunmap_atomic(pcmd_page);
> > -	kunmap_atomic((void *)(unsigned long)pginfo.contents);
> > +	kunmap_local(pcmd_page);
> > +	kunmap_local((void *)(unsigned long)pginfo.contents);
> >  
> >  	get_page(b.pcmd);
> >  	sgx_encl_put_backing(&b);
> > @@ -197,10 +197,10 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> >  
> >  	if (pcmd_page_empty && !reclaimer_writing_to_pcmd(encl, pcmd_first_page)) {
> >  		sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off));
> > -		pcmd_page = kmap_atomic(b.pcmd);
> > +		pcmd_page = kmap_local_page(b.pcmd);
> >  		if (memchr_inv(pcmd_page, 0, PAGE_SIZE))
> >  			pr_warn("PCMD page not empty after truncate.\n");
> > -		kunmap_atomic(pcmd_page);
> > +		kunmap_local(pcmd_page);
> >  	}
> >  
> >  	put_page(b.pcmd);
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index ebe79d60619f..f2f918b8b9b1 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -221,11 +221,11 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
> >  	pginfo.secs = (unsigned long)sgx_get_epc_virt_addr(encl->secs.epc_page);
> >  	pginfo.addr = encl_page->desc & PAGE_MASK;
> >  	pginfo.metadata = (unsigned long)secinfo;
> > -	pginfo.contents = (unsigned long)kmap_atomic(src_page);
> > +	pginfo.contents = (unsigned long)kmap_local_page(src_page);
> >  
> >  	ret = __eadd(&pginfo, sgx_get_epc_virt_addr(epc_page));
> >  
> > -	kunmap_atomic((void *)pginfo.contents);
> > +	kunmap_local((void *)pginfo.contents);
> >  	put_page(src_page);
> >  
> >  	return ret ? -EIO : 0;
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 515e2a5f25bb..4efda5e8cadf 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -159,17 +159,17 @@ static int __sgx_encl_ewb(struct sgx_epc_page *epc_page, void *va_slot,
> >  	pginfo.addr = 0;
> >  	pginfo.secs = 0;
> >  
> > -	pginfo.contents = (unsigned long)kmap_atomic(backing->contents);
> > -	pginfo.metadata = (unsigned long)kmap_atomic(backing->pcmd) +
> > +	pginfo.contents = (unsigned long)kmap_local_page(backing->contents);
> > +	pginfo.metadata = (unsigned long)kmap_local_page(backing->pcmd) +
> >  			  backing->pcmd_offset;
> >  
> >  	ret = __ewb(&pginfo, sgx_get_epc_virt_addr(epc_page), va_slot);
> >  	set_page_dirty(backing->pcmd);
> >  	set_page_dirty(backing->contents);
> >  
> > -	kunmap_atomic((void *)(unsigned long)(pginfo.metadata -
> > +	kunmap_local((void *)(unsigned long)(pginfo.metadata -
> >  					      backing->pcmd_offset));
> 
> For kunmap_local() one can use any address in the page.  So this can be:
> 
> 	kunmap_local((void *)(unsigned long)(pginfo.metadata));
> 
> 
> Regardless:
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

There's no data to show that this change would be useful to do.

Thus, as said earlier, the commit message is missing "why".

Definitive NAK with the current offering.

BR, Jarkko