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

Kuppuswamy, Sathyanarayanan
2021-08-24T17:46:35Z

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