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

[Kuppuswamy, Sathyanarayanan]

On 8/24/21 9:10 AM, Borislav Petkov wrote:
> On Wed, Aug 04, 2021 at 11:13:25AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > @@ -240,6 +243,32 @@ SYM_FUNC_START(__tdx_hypercall)
> >   
> >   	movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx
> >   
> > +	/*
> > +	 * For the idle loop STI needs to be called directly before
> > +	 * the TDCALL that enters idle (EXIT_REASON_HLT case). STI
> > +	 * enables interrupts only one instruction later. If there
> > +	 * are any instructions between the STI and the TDCALL for
> > +	 * HLT then an interrupt could happen in that time, but the
> > +	 * code would go back to sleep afterwards, which can cause
> > +	 * longer delays.
>
> <-- newline in the comment here for better readability.

Ok. I will add it in next version.

> > There leads to significant difference in
>
> "There leads..." ?

Will fix this in next version. "This leads"

> > +	 * network performance benchmarks. So add a special case for
> > +	 * EXIT_REASON_HLT to trigger sti before TDCALL. But this
> > +	 * change is not required for all HLT cases. So use R15
> > +	 * register value to identify the case which needs sti. So,
>
> s/sti/STI/g

will fix this in next version.

> > +	 * if R11 is EXIT_REASON_HLT and R15 is 1, then call sti
> > +	 * before TDCALL instruction. Note that R15 register is not
> > +	 * required by TDCALL ABI when triggering the hypercall for
> > +	 * EXIT_REASON_HLT case. So use it in software to select the
> > +	 * sti case.
> > +	 */
> > +	cmpl $EXIT_REASON_HLT, %r11d
> > +	jne skip_sti
> > +	cmpl $1, %r15d
> > +	jne skip_sti
> > +	/* Set R15 register to 0, it is unused in EXIT_REASON_HLT case */
> > +	xor %r15, %r15
> > +	sti
> > +skip_sti:
> >   	tdcall
>
> ...
>
> > +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.

It is a bug in this submission. After adding STI fix, this local_irq_enable()
had to be removed. Somehow I have missed to do it. I will fix this
in next version.

> > +	 */
> > +	local_irq_enable();
> > +
> > +	/* IRQ is enabled, So set R12 as 0 */
> > +	ret = _tdx_hypercall(EXIT_REASON_HLT, 0, 0, 0, 1, NULL);
> > +
> > +	/* It should never fail */
> > +	BUG_ON(ret);
> > +}

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer