Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Oct 2022 08:23:03 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Kristen Carlson Accardi <kristen@linux.intel.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-sgx@vger.kernel.org>,
        "Jarkko Sakkinen" <jarkko@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        <x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH] x86/sgx: Replace kmap/kunmap_atomic calls
Message-ID: <Y0BEV+Xgkrln8xoh@iweiny-desk3>
References: <20220929160647.362798-1-kristen@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220929160647.362798-1-kristen@linux.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?UDB0IEpa+YaPbz4k1idxcNEtcPPd+ZVjsKJpGH1OpYqXJlxud2SRS4cAnZG9?=
 =?us-ascii?Q?wyZNvQEhrROxhyP1zsiBBiZKlxIzo9S9dbRvAi8/u4yb8MwNqIvP8Bi6IZjJ?=
 =?us-ascii?Q?uLHa98erj1nelYvhm+ti0hBFcbcx4KDqEBZsh16AlyFhOjGF3xCKvtrLNCuh?=
 =?us-ascii?Q?JgF2i+saQKLA1wdb8uh306TYMHtyBMgBpzs2XeTPJTj4HORo3UgYFDdd8zVE?=
 =?us-ascii?Q?EdySF6S/AdbVuI/ZHZh1kiaGC2FYTLmDukkosXh3T7hGwTOeMHkfru/5RspX?=
 =?us-ascii?Q?7/JnCqRHpTT4l8TpVMjtEUhHy5xkfpGILqgz/qNqEWmwe1//J5+0gDSn3Y9b?=
 =?us-ascii?Q?+2T0MDQbIhJnuvar8pIUmLKyx0ju7SAKZZUqyhR57T9ZM9yUWVfmWq8mRgYE?=
 =?us-ascii?Q?DPfQN2CTS/FpC8LGz6HZJFBNEHTWd1+tRf9zLlIcIB6ZWlgl2PbTvD5tWMPn?=
 =?us-ascii?Q?Wy8xVPHII6bm67ZfsCALhC9uJi7qPC2dUeF1+PTXufAnTkx2P9RXqOKenLiy?=
 =?us-ascii?Q?pg8V+39N1rartwP3MMDqcIhfSXPjK1e0/iDgn3aDs6yRVaZN1iDnABB5MwBU?=
 =?us-ascii?Q?0LtXa/svoTcOTqr/ZHZCHTqW3iwczaxxn9jQpa2WMT+xGSijlS/y/46cnJIA?=
 =?us-ascii?Q?BVMCyOYw8+u3oDgDlQdONxcoe8/0sKVuPhMSnh9w0N4YpnhpjrYLKdZTDhAR?=
 =?us-ascii?Q?MpnFI8AwmtjxGTCim/6OYPN4ETP0Ta5U4m5rF9VilC2t1bkI3LgOLPCr1x3a?=
 =?us-ascii?Q?+MvVsaWxuCGml+uIfOThsE57jDxZABmWhRLVazAccf7xpwqfda5PZBBlxvFa?=
 =?us-ascii?Q?N0XBS1v9XmLNYFp2HunugLoVLix0ktN0qsrAejqCXGzH+uL4mJMZoutPzsXO?=
 =?us-ascii?Q?KpkCrBePJE+5n+yaT3MSWKQUjgrCQpcEfb7MNBruXQDl32C2CymMXU/pHG4T?=
 =?us-ascii?Q?RqEhRWVb/N/hL0EbOGVoL5e+ifpGu7lL3X70SvCfFRsR9Q6C/Dwa15TSTuax?=
 =?us-ascii?Q?4jGLOjDHZXqmNUnBt7e7Eo0IXz8T5CbwMLpBjDS09Da033maQsNEdqhSKtHn?=
 =?us-ascii?Q?k3x92QSveemHPW65pbGkJf3aM3wifsBDT4eN2eVOkIRW0rNKhJ9fv1rbj4uh?=
 =?us-ascii?Q?zEOQwv7GEsQbVIJXKUqRjLVr7ZJx7jMlLsjbrfHaFWUOYaaxscKxm4PsGFyn?=
 =?us-ascii?Q?/Ti5WtACnuAL50VAC2JCTaqmrNN+h8XyEtTKcgz/+gB7Y88IjkX1VI7QBhZe?=
 =?us-ascii?Q?D9xsSy57W4vmgVgEtGYJpqprcwDA6ss55rBPnGqdQkX03M2ef1H0nsp/ae2h?=
 =?us-ascii?Q?i2lKfxsPUqeyCFPYxE+dJncLrSKXWOaTIPoLwmlf1I1ulKefTCKLJdTyfb1Q?=
 =?us-ascii?Q?CaqInTEGOR5NAp8nhogJOzYpmaFJRj/rrh2ekcjlct2QAJUJ68CZD+63wr6J?=
 =?us-ascii?Q?jcorBzoc2xfQSZlDtBzA+2K9MG3zRL0/AuLYGjxt+dPhxCi9LhOhoogQdnce?=
 =?us-ascii?Q?E9BxZneE8ApKMIj0OFOv85GorU+dLcTieO7L06/aCMgDiWhG7pojQv2uRcJN?=
 =?us-ascii?Q?15APlULzQvHEyv6Xr4igwdf37BOyQvtnVXACnlkX?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 39cdac21-6340-423f-42de-08daa877d614
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Oct 2022 15:23:07.4603
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ycvN9qdLQSQNJXOT/cH+nQUqHpeJd25+rAYVynUgH0A4GdK9oZxDsyRCg+4RUlAyQvFO6+ysGjXo0DHyGiYcpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5943
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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>

> -	kunmap_atomic((void *)(unsigned long)pginfo.contents);
> +	kunmap_local((void *)(unsigned long)pginfo.contents);
>  
>  	return ret;
>  }
> -- 
> 2.37.3
>