Received-SPF: pass (google.com: domain of linux-kernel+bounces-17314-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <ff19b03a-8c3a-43ba-8d34-bd4a218b8950@intel.com>
Date: Thu, 4 Jan 2024 16:04:58 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] x86/asm: Remove the __iomem annotation of movdir64b()'s
 dst argument
To: Kai Huang <kai.huang@intel.com>, <linux-kernel@vger.kernel.org>
CC: <x86@kernel.org>, <dave.hansen@intel.com>,
	<kirill.shutemov@linux.intel.com>, <tglx@linutronix.de>, <bp@alien8.de>,
	<mingo@redhat.com>, <peterz@infradead.org>, <rafael@kernel.org>,
	<dan.j.williams@intel.com>, <hpa@zytor.com>, <geert@linux-m68k.org>,
	<bhe@redhat.com>, <akpm@linux-foundation.org>, <rppt@kernel.org>,
	<frederic@kernel.org>, <xin3.li@intel.com>, <rick.p.edgecombe@intel.com>,
	<isaku.yamahata@intel.com>, <yuan.yao@intel.com>
References: <20240104221219.572258-1-kai.huang@intel.com>
Content-Language: en-US
From: Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20240104221219.572258-1-kai.huang@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q1RPdEIvRXV2MUtWc05ZdGhOQ3l0bG5QOTBVV1pGSm41bVRDbE9qNmhDU0tL?=
 =?utf-8?B?TGlWWFRYSmoyS28vbjRMV242Q09LTlloaWZ6ZzVRL05DejM5Q0hOcEU3dWpp?=
 =?utf-8?B?Kzhxa0gxSGRuTnI3M1ZRZkZZL3Y1NkxaTXJyZi9UU1V4RFFOUmlRWGVlckgv?=
 =?utf-8?B?bkVGQUdXd01DdVRHZXpjN3lkS2RtSm5ZakliVy9zYXU4ZHZ0MWhJNytQcGFm?=
 =?utf-8?B?L1NEdGovaUZkc3JtK1JNT0dUT2M4cDVvTmJNeVIvamhUUFdCT3ZUMmZqWG5J?=
 =?utf-8?B?eVh3d0p6QWp2WTVkWmt0MzlnUUkwSGlHdU1FVTlWbU5qKzdvYjk2R01SUStn?=
 =?utf-8?B?TFpRc28wRlQ4RllWVjBSWWUrV0Y5ZnJNVHJKZ0FXSjZjdDBOaWV0NUNua2ov?=
 =?utf-8?B?cXhFdnFGVGd1dkZqYXZ4TmVQRjFYNFIrSExqNTlSdVlDYmlkVTNsc213c05H?=
 =?utf-8?B?RnJ0aEM2RWZUUks1UlZjanhTVGZEQ25JaEpJTVVMdWRwUmJwNzF2WVJuUHRh?=
 =?utf-8?B?aVBGbDdGazBodnZHTFNSY2o5WEllcFpwQVIwcDQvY2lGZWFLOFpibnlSNFk2?=
 =?utf-8?B?Tmh4Slh4ZStmVVJxczZTbkZTd0l2QnNjQ3UwR2xtQlQxMU5STWV3RmhHdWM4?=
 =?utf-8?B?QkoxSE5XRDFmaksxU1lZQUY4VmVMY3N3ODROTDBTU1NPaGhRcWRGMGhhd1pk?=
 =?utf-8?B?SVVvRy9OMSs4ZUQ0dVRzRmxmcUhsK0JVMnNFMUFjWk5IekpQdEhQSkVnUTRG?=
 =?utf-8?B?Q2dPSTZCZG15bzdUY1Y4VnQ2dnhLUE95VTJuL3dlM2R4aloyOWFzMm9nV2JI?=
 =?utf-8?B?QXhTU2d2VXdLekY1dHVuZndYNDA0bEpndmdJbWJXMzVCbmxDMlEvRjh2TWNT?=
 =?utf-8?B?UzJibXlJU3Y0aGd5MlVKOVNVckdRc0krTG1zSUpaazNQVUFyMWM1SlhLc0d5?=
 =?utf-8?B?WkZPR3dkTlMrY2U2elNvL0pGaUZRZ1U5cnhVZHBJMXFvM2pEeGhza3doajFZ?=
 =?utf-8?B?MmtBQnA5eVlqTEdhNVR3T0NQMHNseTR1OHVlOHRQTXRiMWdBVlV2NmxmRG9W?=
 =?utf-8?B?a2VWS3AxYjhqWHZLM0NGSi8zKzBtbWhMdENxeCtUNGVQM2lxRTkrOWpHV2t2?=
 =?utf-8?B?bnhlK2Iwdkxqa0FodCtXSDlxbnZPeHh3cjlQUjB0R1VOaVBzenY3a1ZTaldO?=
 =?utf-8?B?dURZMkozYllXVUVpY2xuZTUxY3NpT0VPbVR0S3liRnFqZlNWaWlFS254T2ow?=
 =?utf-8?B?aUJVSVFHaTBXMFErRWNpcGNpcTE2M1dpblYvWHQ0TnpBaUtXRkpNMndpNmRy?=
 =?utf-8?B?elhpakNaczViY2ZicGRsYkZjQWhqcEtOUTZrY1pkMHpTbUZXS1pKcDBNQU8z?=
 =?utf-8?B?bWxJSHpiU3VxL2N4c2Q5bUNablpjTUpwaXhoTlV2OXRHZEFiVFlVUWNqQzlv?=
 =?utf-8?B?NFMvb2Nmb1MyeGdoZVRDNGsxRGJYaE84bmNrTmp1dE5iUlpvdGh6Z2V3eXdx?=
 =?utf-8?B?Si9iRGdIaWNnVGVTQldLQk4zN2Nsb0prN3pzN3hWZDlVeWQyUG5OejVBSDIx?=
 =?utf-8?B?aDlGT3VjcFJ5Qlg2aHVNZDNiVDZWSlRFSEVWNDFuZHJObFkzb0hlRWUxZkd4?=
 =?utf-8?B?R2JGTVRYenhwY0x4QTFLejdjWXQxdVpLNTZ0Q1RjY1NFMzNtUVBrT1h4WWh6?=
 =?utf-8?B?QUt2TDIyYkpXKzl5cDVsaDY3Y1FuUCsrVCtGUHdpdU1Vdmp3ZVp2Y0FpdWla?=
 =?utf-8?B?emtIbThnZzhKUjRRZ3QvRFRpS1ZTaUYvN0dVaEdNdFoyU1FueGtMQXp3UEky?=
 =?utf-8?B?NThpa25ORGFabGVKQVcwWjVaTHRWWVBpV2ZjMGFPVGFIaTVVQlBUaHl6cm9s?=
 =?utf-8?B?Ky9ZS1hKaTBGeGp3RHpyc000RTBHZUQ3cXk1MUJhTWRQT040Rlg0SFVZRStk?=
 =?utf-8?B?Y3gwamF1VWNTNk5FV3oyWGdid08xQUt3M1Z0SmQxUFFOWCtrNnRTSWk4bmRK?=
 =?utf-8?B?NzBNVmh3elY4TXlSbUZFa1RGVGVIS0J2WmxzOWVoN0hZaHptanlGanZ1Zzgy?=
 =?utf-8?B?azQ5ZTg2RkxXZWJYWlBoZXhkR2pybTNvMTdIUlhuN1E3M0NGREFCUTJsQmtl?=
 =?utf-8?Q?RKbiuz5ZctMMn0artvj7p+4hR?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fc69d33c-3869-4f83-228f-08dc0d799557
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2024 23:05:02.9252
 (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: sUDQudppRMGu2ntWI1ayrF47g6SUBW30o1OO8tjSswe8EQlttvuM0NYOV04aUCZlSAi3KFXckGPhui8rk8b15g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6123
X-OriginatorOrg: intel.com


On 1/4/24 15:12, Kai Huang wrote:
> Commit e56d28df2f66 ("x86/virt/tdx: Configure global KeyID on all
> packages") causes below sparse check warning:
> 
>   arch/x86/virt/vmx/tdx/tdx.c:683:27: warning: incorrect type in argument 1 (different address spaces)
>   arch/x86/virt/vmx/tdx/tdx.c:683:27:    expected void [noderef] __iomem *dst
>   arch/x86/virt/vmx/tdx/tdx.c:683:27:    got void *
> 
> The reason is TDX must use the MOVDIR64B instruction to convert TDX
> private memory (which is normal RAM but not MMIO) back to normal.  The
> TDX code uses existing movdir64b() helper to do that, but the first
> argument @dst of movdir64b() is annotated with __iomem.
> 
> When movdir64b() was firstly introduced in commit 0888e1030d3e
> ("x86/asm: Carve out a generic movdir64b() helper for general usage"),
> it didn't have the __iomem annotation.  But this commit also introduced
> the same "incorrect type" sparse warning because the iosubmit_cmds512(),
> which was the solo caller of movdir64b(), has the __iomem annotation.
> 
> This was later fixed by commit 6ae58d871319 ("x86/asm: Annotate
> movdir64b()'s dst argument with __iomem").  That fix was reasonable
> because until TDX code the movdir64b() was only used to move data to
> MMIO location, as described by the commit message:
> 
>   ... The current usages send a 64-bytes command descriptor to an MMIO
>   location (portal) on a device for consumption. When future usages for
>   the MOVDIR64B instruction warrant a separate variant of a memory to
>   memory operation, the argument annotation can be revisited.
> 
> Now TDX code uses MOVDIR64B to move data to normal memory so it's time
> to revisit.
> 
> The SDM says the destination of MOVDIR64B is "memory location specified
> in a general register", thus it's more reasonable that movdir64b() does
> not have the __iomem annotation on the @dst.
> 
> Remove the __iomem annotation from the @dst argument of movdir64b() to
> fix the sparse warning in TDX code.  Similar to memset_io(), introduce a
> new movdir64b_io() to cover the case where the destination is an MMIO
> location, and change the solo caller iosubmit_cmds512() to use the new
> movdir64b_io().
> 
> In movdir64b_io() explicitly use __force in the type casting otherwise
> there will be below sparse warning:
> 
>   warning: cast removes address space '__iomem' of expression
> 
> Fixes: e56d28df2f66 ("x86/virt/tdx: Configure global KeyID on all packages")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312311924.tGjsBIQD-lkp@intel.com/
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  arch/x86/include/asm/io.h            | 2 +-
>  arch/x86/include/asm/special_insns.h | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index 76238842406a..de2dc9837f11 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -379,7 +379,7 @@ static inline void iosubmit_cmds512(void __iomem *dst, const void *src,
>  	const u8 *end = from + count * 64;
>  
>  	while (from < end) {
> -		movdir64b(dst, from);
> +		movdir64b_io(dst, from);
>  		from += 64;
>  	}
>  }
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index d6cd9344f6c7..f661277e52d6 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -224,10 +224,10 @@ static inline void serialize(void)
>  }
>  
>  /* The dst parameter must be 64-bytes aligned */
> -static inline void movdir64b(void __iomem *dst, const void *src)
> +static inline void movdir64b(void *dst, const void *src)
>  {
>  	const struct { char _[64]; } *__src = src;
> -	struct { char _[64]; } __iomem *__dst = dst;
> +	struct { char _[64]; } *__dst = dst;
>  
>  	/*
>  	 * MOVDIR64B %(rdx), rax.
> @@ -245,6 +245,11 @@ static inline void movdir64b(void __iomem *dst, const void *src)
>  		     :  "m" (*__src), "a" (__dst), "d" (__src));
>  }
>  
> +static inline void movdir64b_io(void __iomem *dst, const void *src)
> +{
> +	movdir64b((void __force *)dst, src);
> +}
> +
>  /**
>   * enqcmds - Enqueue a command in supervisor (CPL0) mode
>   * @dst: destination, in MMIO space (must be 512-bit aligned)
> 
> base-commit: 83e1bdc94f32dcf52dfcd2025acc7a2b9376b1e8