Re: [PATCH v4 04/24] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs

[Huang, Kai]

> 
> > 
> > But I don't know why do you even need to talk about NP-SEAMLDR.
> 
> I included this because Dave had some confusion about NP-SEAMLDR [1], so I
> wanted to clarify it.
> 
> [1]: https://lore.kernel.org/kvm/aXt0+lRvpvf5knKP@xxxxxxxxx/

I thought that was under assumption both NP-SEAMLDR and P-SEAMLDR are SEAM
software (which is why both of them are mentioned).  But only P-SEAMLDR is,
so I thought we can skip NP-SEAMLDR.

> 
> And, since NP-SEAMLDR and P-SEAMLDR have similar names, I thought it would be
> helpful to clarify the difference. This follows Dave's earlier suggestion to
> explain SEAM_INFO and SEAM_SEAMINFO SEAMCALLs for clarity [2].
> 
> [2]: https://lore.kernel.org/kvm/b2e2fd5e-8aff-4eda-a648-9ae9f8234d25@xxxxxxxxx/
> 

Sure.  If you feel that helps.

[...]

> 
> > > + * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
> > > + * interact with P-SEAMLDR simultaneously.
> > > + */
> > > +static DEFINE_RAW_SPINLOCK(seamldr_lock);
> > > +
> > > +static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> > > +{
> > > +	/*
> > > +	 * Serialize P-SEAMLDR calls and disable interrupts as the calls
> > > +	 * can be made from IRQ context.
> > > +	 */
> > > +	guard(raw_spinlock_irqsave)(&seamldr_lock);
> > 
> > Why do you need to disable IRQ?  A plain raw_spinlock should work with both
> > cases where seamldr_call() is called from IRQ disabled context and normal
> > task context? 
> 
> No, that's not safe. Without _irqsave, a deadlock can occur if an interrupt
> fires while a task context already holds the lock, and the interrupt handler
> also tries to acquire the same lock.

I thought that's not possible to happen because during module update we have
a machine state to serialize these P-SEAMLDR SEAMCALLs.

But I agree making it IRQ safe is the simplest way so that we don't need to
worry about the deadlock.


Sorry about the noise.