Re: [PATCH v12 07/22] x86/virt/tdx: Add skeleton to enable TDX on demand

[Huang, Kai]

On Wed, 2023-06-28 at 15:08 +0200, Peter Zijlstra wrote:
> On Tue, Jun 27, 2023 at 02:12:37AM +1200, Kai Huang wrote:
> 
> > +/*
> > + * Do the module global initialization if not done yet.
> > + * It's always called with interrupts and preemption disabled.
> > + */
> > +static int try_init_module_global(void)
> > +{
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	/*
> > +	 * The TDX module global initialization only needs to be done
> > +	 * once on any cpu.
> > +	 */
> > +	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);
> > +
> > +	if (tdx_global_initialized) {
> > +		ret = 0;
> > +		goto out;
> > +	}
> > +
> > +	/* All '0's are just unused parameters. */
> > +	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
> > +	if (!ret)
> > +		tdx_global_initialized = true;
> > +out:
> > +	raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * tdx_cpu_enable - Enable TDX on local cpu
> > + *
> > + * Do one-time TDX module per-cpu initialization SEAMCALL (and TDX module
> > + * global initialization SEAMCALL if not done) on local cpu to make this
> > + * cpu be ready to run any other SEAMCALLs.
> > + *
> > + * Call this function with preemption disabled.
> > + *
> > + * Return 0 on success, otherwise errors.
> > + */
> > +int tdx_cpu_enable(void)
> > +{
> > +	int ret;
> > +
> > +	if (!platform_tdx_enabled())
> > +		return -ENODEV;
> > +
> > +	lockdep_assert_preemption_disabled();
> > +
> > +	/* Already done */
> > +	if (__this_cpu_read(tdx_lp_initialized))
> > +		return 0;
> > +
> > +	/*
> > +	 * The TDX module global initialization is the very first step
> > +	 * to enable TDX.  Need to do it first (if hasn't been done)
> > +	 * before the per-cpu initialization.
> > +	 */
> > +	ret = try_init_module_global();
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* All '0's are just unused parameters */
> > +	ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
> > +	if (ret)
> > +		return ret;
> 
> And here you do *NOT* have IRQs disabled... so an IRQ can come in here
> and do the above again.
> 
> I suspect that's a completely insane thing to have happen, but the way
> the code is written does not tell me this and might even suggest I
> should worry about it, per the above thing actually disabling IRQs.
> 

I can change lockdep_assert_preemption_disabled() to
lockdep_assert_irqs_disabled(), making this function only being called from IPI.
As Kirill also suggested we can do this way as for now KVM is the only user of
this function and it enables hardware for all cpus via IPI.
>