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>
