Re: [PATCH v10 07/11] x86/tdx: Add HLT support for TDX guest

[Sathyanarayanan Kuppuswamy]

On 10/14/21 2:30 AM, Thomas Gleixner wrote:
> On Fri, Oct 08 2021 at 22:37, Kuppuswamy Sathyanarayanan wrote:
> > +/* HLT TDVMCALL sub-function ID */
> > +#define EXIT_REASON_HLT			12
> arch/x86/include/uapi/asm/vmx.h:#define EXIT_REASON_HLT                 12
>
> Is there a _good_ reason why this can't be reused?

As per current use case we can re-use it. Out of all TDX hypercall sub 
function
IDs, only Instruction.PCONFIG (65) exit reason id is missing in vmx.h. 
But currently
we are not handling it. So we can ignore it for now.

I will fix this in next version.

> >   /*
> >    * __tdx_module_call()  - Helper function used by TDX guests to request
> >    * services from the TDX module (does not include VMM services).
> > @@ -235,6 +238,33 @@ 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.
> > +	 *
> > +	 * This leads to significant difference in 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, 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
> You already have a nice define for EXIT_REASON_HLT. Please add one for this
> constant as well.

Ok. I will add it.

> > +	jne skip_sti
> > +	/* Set R15 register to 0, it is unused in EXIT_REASON_HLT case */
> > +	xor %r15, %r15
> > +	sti
> > +skip_sti:
> >   	tdcall
> >   bool tdx_get_ve_info(struct ve_info *ve)
> >   {
> >   	struct tdx_module_output out;
> > @@ -84,8 +141,19 @@ bool tdx_get_ve_info(struct ve_info *ve)
> >   bool tdx_handle_virtualization_exception(struct pt_regs *regs,
> >   					 struct ve_info *ve)
> >   {
> > -	pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
> > -	return false;
> > +	switch (ve->exit_reason) {
> > +	case EXIT_REASON_HLT:
> > +		tdx_halt();
> > +		break;
> > +	default:
> > +		pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
> > +		return false;
> > +	}
> > +
> > +	/* After successful #VE handling, move the IP */
> > +	regs->ip += ve->instr_len;
> > +
> > +	return true;
> >   }
> >   
> >   void __init tdx_early_init(void)
> > @@ -95,5 +163,8 @@ void __init tdx_early_init(void)
> >   
> >   	setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
> >   
> > +	pv_ops.irq.safe_halt = tdx_safe_halt;
> > +	pv_ops.irq.halt = tdx_halt;
> Colour me confused, but why do we end up in #VE with EXIT_REASON_HLT
> when halt/safe_halt is paravirtualized?
>
> There may be a valid reason. If so then this lacks a comment.

No, with halt/safe_halt para-virtualized, we should never get
#VE for it. I think this is a redundant handler code. This was
added before we have added the pv_ops support. I will remove
it in next version.

> Thanks,
>
>          tglx

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer