Received-SPF: pass (google.com: domain of linux-kernel+bounces-17341-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 5 Jan 2024 02:52:01 +0300
From: kirill.shutemov@linux.intel.com
To: Kai Huang <kai.huang@intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, dave.hansen@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,
	dave.jiang@intel.com, xin3.li@intel.com, rick.p.edgecombe@intel.com,
	isaku.yamahata@intel.com, yuan.yao@intel.com
Subject: Re: [PATCH] x86/asm: Remove the __iomem annotation of movdir64b()'s
 dst argument
Message-ID: <20240104235201.hzv2kb7vxocs3asx@box>
References: <20240104221219.572258-1-kai.huang@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240104221219.572258-1-kai.huang@intel.com>

On Fri, Jan 05, 2024 at 11:12:19AM +1300, 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: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov