Re: [PATCH 2/7] KVM: TDX: Implement TDX vcpu enter/exit path

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 11/22/2024 4:14 AM, Adrian Hunter wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> This patch implements running TDX vcpu.  Once vcpu runs on the logical
> processor (LP), the TDX vcpu is associated with it.  When the TDX vcpu
> moves to another LP, the TDX vcpu needs to flush its status on the LP.
> When destroying TDX vcpu, it needs to complete flush, and flush cpu memory
> cache.  Track which LP the TDX vcpu run and flush it as necessary.

The changelog needs update. It doesn't match the patch content.

> Compared to VMX, do nothing on sched_in event as TDX doesn't support pause
> loop.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> TD vcpu enter/exit v1:
> - Make argument of tdx_vcpu_enter_exit() struct kvm_vcpu.
> - Update for the wrapper functions for SEAMCALLs. (Sean)
> - Remove noinstr (Sean)
> - Add a missing comma, clarify sched_in part, and update changelog to
>    match code by dropping the PMU related paragraph (Binbin)
>    https://lore.kernel.org/lkml/c0029d4d-3dee-4f11-a929-d64d2651bfb3@linux.intel.com/
> - Remove the union tdx_exit_reason. (Sean)
>    https://lore.kernel.org/kvm/ZfSExlemFMKjBtZb@google.com/
> - Remove the code of special handling of vcpu->kvm->vm_bugged (Rick)
>    https://lore.kernel.org/kvm/20240318234010.GD1645738@ls.amr.corp.intel.com/
> - For !tdx->initialized case, set tdx->vp_enter_ret to TDX_SW_ERROR to avoid
>    collision with EXIT_REASON_EXCEPTION_NMI.
> 
> v19:
> - Removed export_symbol_gpl(host_xcr0) to the patch that uses it
> 
> Changes v15 -> v16:
> - use __seamcall_saved_ret()
> - As struct tdx_module_args doesn't match with vcpu.arch.regs, copy regs
>    before/after calling __seamcall_saved_ret().
> ---
>   arch/x86/kvm/vmx/main.c    | 21 ++++++++++-
>   arch/x86/kvm/vmx/tdx.c     | 76 ++++++++++++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/tdx.h     |  2 +
>   arch/x86/kvm/vmx/x86_ops.h |  5 +++
>   4 files changed, 102 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index bfed421e6fbb..44ec6005a448 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -129,6 +129,23 @@ static void vt_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   	vmx_vcpu_load(vcpu, cpu);
>   }
>   
> +static int vt_vcpu_pre_run(struct kvm_vcpu *vcpu)
> +{
> +	if (is_td_vcpu(vcpu))
> +		/* Unconditionally continue to vcpu_run(). */
> +		return 1;
> +
> +	return vmx_vcpu_pre_run(vcpu);
> +}
> +
> +static fastpath_t vt_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
> +{
> +	if (is_td_vcpu(vcpu))
> +		return tdx_vcpu_run(vcpu, force_immediate_exit);
> +
> +	return vmx_vcpu_run(vcpu, force_immediate_exit);
> +}
> +
>   static void vt_flush_tlb_all(struct kvm_vcpu *vcpu)
>   {
>   	if (is_td_vcpu(vcpu)) {
> @@ -267,8 +284,8 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>   	.flush_tlb_gva = vt_flush_tlb_gva,
>   	.flush_tlb_guest = vt_flush_tlb_guest,
>   
> -	.vcpu_pre_run = vmx_vcpu_pre_run,
> -	.vcpu_run = vmx_vcpu_run,
> +	.vcpu_pre_run = vt_vcpu_pre_run,
> +	.vcpu_run = vt_vcpu_run,
>   	.handle_exit = vmx_handle_exit,
>   	.skip_emulated_instruction = vmx_skip_emulated_instruction,
>   	.update_emulated_instruction = vmx_update_emulated_instruction,
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index dc6c5f40608e..5fa5b65b9588 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -10,6 +10,9 @@
>   #include "mmu/spte.h"
>   #include "common.h"
>   
> +#include <trace/events/kvm.h>
> +#include "trace.h"
> +
>   #undef pr_fmt
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
> @@ -662,6 +665,79 @@ void tdx_vcpu_free(struct kvm_vcpu *vcpu)
>   }
>   
>   
> +static void tdx_vcpu_enter_exit(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_tdx *tdx = to_tdx(vcpu);
> +	struct tdx_module_args args;
> +
> +	guest_state_enter_irqoff();
> +
> +	/*
> +	 * TODO: optimization:
> +	 * - Eliminate copy between args and vcpu->arch.regs.
> +	 * - copyin/copyout registers only if (tdx->tdvmvall.regs_mask != 0)
> +	 *   which means TDG.VP.VMCALL.
> +	 */
> +	args = (struct tdx_module_args) {
> +		.rcx = tdx->tdvpr_pa,
> +#define REG(reg, REG)	.reg = vcpu->arch.regs[VCPU_REGS_ ## REG]
> +		REG(rdx, RDX),
> +		REG(r8,  R8),
> +		REG(r9,  R9),
> +		REG(r10, R10),
> +		REG(r11, R11),
> +		REG(r12, R12),
> +		REG(r13, R13),
> +		REG(r14, R14),
> +		REG(r15, R15),
> +		REG(rbx, RBX),
> +		REG(rdi, RDI),
> +		REG(rsi, RSI),
> +#undef REG
> +	};
> +
> +	tdx->vp_enter_ret = tdh_vp_enter(tdx->tdvpr_pa, &args);
> +
> +#define REG(reg, REG)	vcpu->arch.regs[VCPU_REGS_ ## REG] = args.reg
> +	REG(rcx, RCX);
> +	REG(rdx, RDX);
> +	REG(r8,  R8);
> +	REG(r9,  R9);
> +	REG(r10, R10);
> +	REG(r11, R11);
> +	REG(r12, R12);
> +	REG(r13, R13);
> +	REG(r14, R14);
> +	REG(r15, R15);
> +	REG(rbx, RBX);
> +	REG(rdi, RDI);
> +	REG(rsi, RSI);
> +#undef REG
> +
> +	guest_state_exit_irqoff();
> +}
> +
> +fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
> +{
> +	struct vcpu_tdx *tdx = to_tdx(vcpu);
> +
> +	/* TDX exit handle takes care of this error case. */
> +	if (unlikely(tdx->state != VCPU_TD_STATE_INITIALIZED)) {
> +		/* Set to avoid collision with EXIT_REASON_EXCEPTION_NMI. */

It seems the check fits better in tdx_vcpu_pre_run().

And without the patch of how TDX handles Exit (i.e., how deal with 
vp_enter_ret), it's hard to review this comment.

> +		tdx->vp_enter_ret = TDX_SW_ERROR;
> +		return EXIT_FASTPATH_NONE;
> +	}
> +
> +	trace_kvm_entry(vcpu, force_immediate_exit);
> +
> +	tdx_vcpu_enter_exit(vcpu);
> +
> +	vcpu->arch.regs_avail &= ~VMX_REGS_LAZY_LOAD_SET;
> +	trace_kvm_exit(vcpu, KVM_ISA_VMX);
> +
> +	return EXIT_FASTPATH_NONE;
> +}
> +
>   void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, int pgd_level)
>   {
>   	u64 shared_bit = (pgd_level == 5) ? TDX_SHARED_BIT_PWL_5 :
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index 899654519df6..ebee1049b08b 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -51,6 +51,8 @@ struct vcpu_tdx {
>   
>   	struct list_head cpu_list;
>   
> +	u64 vp_enter_ret;
> +
>   	enum vcpu_tdx_state state;
>   };
>   
> diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
> index 06583b1afa4f..3d292a677b92 100644
> --- a/arch/x86/kvm/vmx/x86_ops.h
> +++ b/arch/x86/kvm/vmx/x86_ops.h
> @@ -129,6 +129,7 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp);
>   int tdx_vcpu_create(struct kvm_vcpu *vcpu);
>   void tdx_vcpu_free(struct kvm_vcpu *vcpu);
>   void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
> +fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit);
>   
>   int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp);
>   
> @@ -156,6 +157,10 @@ static inline int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) { return -EOP
>   static inline int tdx_vcpu_create(struct kvm_vcpu *vcpu) { return -EOPNOTSUPP; }
>   static inline void tdx_vcpu_free(struct kvm_vcpu *vcpu) {}
>   static inline void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) {}
> +static inline fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
> +{
> +	return EXIT_FASTPATH_NONE;
> +}
>   
>   static inline int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp) { return -EOPNOTSUPP; }
>