Re: [PATCH v5 08/12] x86/tdx: Add HLT support for TDX guest

[Borislav Petkov]

On Tue, Aug 24, 2021 at 05:06:21PM +0000, Sean Christopherson wrote:
> On Tue, Aug 24, 2021, Borislav Petkov wrote:
> > On Wed, Aug 04, 2021 at 11:13:25AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > +static __cpuidle void tdg_safe_halt(void)
> > > +{
> > > +	u64 ret;
> > > +
> > > +	/*
> > > +	 * Enable interrupts next to the TDVMCALL to avoid
> > > +	 * performance degradation.
> > 
> > That comment needs some more love to say exactly what the problem is.
> 
> LOL, I guess hanging the vCPU counts as degraded performance.  But this comment
> can and should go away entirely...
> 
> > > +	 */
> > > +	local_irq_enable();
> 
> ...because this is broken.  It's also disturbing because it suggests that these
> patches are not being tested.

My complaint since '88.

> The STI _must_ immediately precede TDCALL, and it _must_ execute with interrupts
> disabled.  The whole point of the STI blocking shadow is to ensure interrupts are
> blocked until _after_ the HLT completes so that a wake event is not recongized
> before the HLT, in which case the vCPU will get stuck in HLT because its wake
> event alreadyfired.  Enabling IRQs well before the TDCALL defeats the purpose of
> the STI dance in __tdx_hypercall().

Wait, whaaaat?!

So tdg_halt() does that but tdg_safe_halt() goes to great lengths not to
do it. And it looks all legit and all, like it really wanted to do it
differently. WTF?

> There's even a massive comment in __tdx_hypercall() explaining all this...
> 
> > > +
> > > +	/* IRQ is enabled, So set R12 as 0 */
> 
> It would be helpful to use local variables to document what's up, e.g.
> 
>  	const bool irqs_enabled = true;
> 	const bool do_sti = true;
> 
> 	ret = _tdx_hypercall(EXIT_REASON_HLT, irqs_enabled0, 0, 0, do_sti, NULL);

Wait, is this do_sti thing supposed to be:

	 * ... But this
         * change is not required for all HLT cases. So use R15
         * register value to identify the case which needs sti. So,
         * if R11 is EXIT_REASON_HLT and R15 is 1, then call sti
         * before TDCALL instruction.

?


> > > +	ret = _tdx_hypercall(EXIT_REASON_HLT, 0, 0, 0, 1, NULL);
						      ^^^
Yeah, it must be it - the 1 there.

And what's with the irqs_enabled first parameter?

Is that used by the TDX module?

I think in the next version all those _tdx_hypercall() wrappers should
spell it out what the parameters they pass are used for.

Hohumm.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette