Re: [PATCH v10 05/16] x86/virt/tdx: Add skeleton to enable TDX on demand

[Isaku Yamahata]

On Wed, Mar 15, 2023 at 05:31:02PM -0700,
Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote:

> On Tue, Mar 07, 2023 at 03:13:50AM +1300,
> Kai Huang <kai.huang@xxxxxxxxx> wrote:
> 
> > +static int try_init_module_global(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * The TDX module global initialization only needs to be done
> > +	 * once on any cpu.
> > +	 */
> > +	spin_lock(&tdx_global_init_lock);
> 
> 
> If I use tdx_cpu_enable() via kvm hardware_enable_all(), this function is called
> in the context IPI callback and the lockdep complains.  Here is my patch to
> address it
> 
> From 0c4022ffe8cd68dfb455c418eb65538e4e100115 Mon Sep 17 00:00:00 2001
> Message-Id: <0c4022ffe8cd68dfb455c418eb65538e4e100115.1678926123.git.isaku.yamahata@xxxxxxxxx>
> In-Reply-To: <d2aa2142665b8204b628232ab615c98090371c99.1678926122.git.isaku.yamahata@xxxxxxxxx>
> References: <d2aa2142665b8204b628232ab615c98090371c99.1678926122.git.isaku.yamahata@xxxxxxxxx>
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> Date: Wed, 15 Mar 2023 14:26:37 -0700
> Subject: [PATCH] x86/virt/vmx/tdx: Use raw spin lock instead of spin lock
> 
> tdx_cpu_enable() can be called by IPI handler.  The lockdep complains about
> spin lock as follows.  Use raw spin lock.
> 
> =============================
> [ BUG: Invalid wait context ]
> 6.3.0-rc1-tdx-kvm-upstream+ #389 Not tainted
> -----------------------------
> swapper/154/0 is trying to lock:
> ffffffffa7875e58 (tdx_global_init_lock){....}-{3:3}, at: tdx_cpu_enable+0x67/0x180
> other info that might help us debug this:
> context-{2:2}
> no locks held by swapper/154/0.
> stack backtrace:
> Call Trace:
>  <IRQ>
>  dump_stack_lvl+0x64/0xb0
>  dump_stack+0x10/0x20
>  __lock_acquire+0x912/0xc30
>  lock_acquire.part.0+0x99/0x220
>  lock_acquire+0x60/0x170
>  _raw_spin_lock_irqsave+0x43/0x70
>  tdx_cpu_enable+0x67/0x180
>  vt_hardware_enable+0x3b/0x60
>  kvm_arch_hardware_enable+0xe7/0x2e0
>  hardware_enable_nolock+0x33/0x80
>  __flush_smp_call_function_queue+0xc4/0x590
>  generic_smp_call_function_single_interrupt+0x1a/0xb0
>  __sysvec_call_function+0x48/0x200
>  sysvec_call_function+0xad/0xd0
>  </IRQ>
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 2ee37a5dedcf..e1c8ffad7406 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -41,7 +41,7 @@ static u32 tdx_guest_keyid_start __ro_after_init;
>  static u32 tdx_nr_guest_keyids __ro_after_init;
>  
>  static unsigned int tdx_global_init_status;
> -static DEFINE_SPINLOCK(tdx_global_init_lock);
> +static DEFINE_RAW_SPINLOCK(tdx_global_init_lock);
>  #define TDX_GLOBAL_INIT_DONE	_BITUL(0)
>  #define TDX_GLOBAL_INIT_FAILED	_BITUL(1)
>  
> @@ -349,6 +349,7 @@ static void tdx_trace_seamcalls(u64 level)
>  
>  static int try_init_module_global(void)
>  {
> +	unsigned long flags;
>  	u64 tsx_ctrl;
>  	int ret;
>  
> @@ -356,7 +357,7 @@ static int try_init_module_global(void)
>  	 * The TDX module global initialization only needs to be done
>  	 * once on any cpu.
>  	 */
> -	spin_lock(&tdx_global_init_lock);
> +	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);

As hardware_enable_all() uses cpus_read_lock(), irqsave isn't needed.
this line should be raw_spin_lock().
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>