Re: [PATCH 4/7] KVM: TDX: Handle TDG.VP.VMCALL<MapGPA>

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

On 12/1/2024 11:53 AM, Binbin Wu wrote:
> Convert TDG.VP.VMCALL<MapGPA> to KVM_EXIT_HYPERCALL with
> KVM_HC_MAP_GPA_RANGE and forward it to userspace for handling.
> 
> MapGPA is used by TDX guest to request to map a GPA range as private
> or shared memory.  It needs to exit to userspace for handling.  KVM has
> already implemented a similar hypercall KVM_HC_MAP_GPA_RANGE, which will
> exit to userspace with exit reason KVM_EXIT_HYPERCALL.  Do sanity checks,
> convert TDVMCALL_MAP_GPA to KVM_HC_MAP_GPA_RANGE and forward the request
> to userspace.
> 
> To prevent a TDG.VP.VMCALL<MapGPA> call from taking too long, the MapGPA
> range is split into 2MB chunks and check interrupt pending between chunks.
> This allows for timely injection of interrupts and prevents issues with
> guest lockup detection.  TDX guest should retry the operation for the
> GPA starting at the address specified in R11 when the TDVMCALL return
> TDVMCALL_RETRY as status code.
> 
> Note userspace needs to enable KVM_CAP_EXIT_HYPERCALL with
> KVM_HC_MAP_GPA_RANGE bit set for TD VM.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> Hypercalls exit to userspace breakout:
> - New added.
>    Implement one of the hypercalls need to exit to userspace for handling after
>    dropping "KVM: TDX: Add KVM Exit for TDX TDG.VP.VMCALL", which tries to resolve
>    Sean's comment.
>    https://lore.kernel.org/kvm/Zg18ul8Q4PGQMWam@google.com/
> - Check interrupt pending between chunks suggested by Sean.
>    https://lore.kernel.org/kvm/ZleJvmCawKqmpFIa@google.com/
> - Use TDVMCALL_STATUS prefix for TDX call status codes (Binbin)
> - Use vt_is_tdx_private_gpa()
> ---
>   arch/x86/include/asm/shared/tdx.h |   1 +
>   arch/x86/kvm/vmx/tdx.c            | 110 ++++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/tdx.h            |   3 +
>   3 files changed, 114 insertions(+)
> 
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 620327f0161f..a602d081cf1c 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -32,6 +32,7 @@
>   #define TDVMCALL_STATUS_SUCCESS		0x0000000000000000ULL
>   #define TDVMCALL_STATUS_RETRY		0x0000000000000001ULL
>   #define TDVMCALL_STATUS_INVALID_OPERAND	0x8000000000000000ULL
> +#define TDVMCALL_STATUS_ALIGN_ERROR	0x8000000000000002ULL
>   
>   /*
>    * Bitmasks of exposed registers (with VMM).
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 4cc55b120ab0..553f4cbe0693 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -985,12 +985,122 @@ static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
>   	return r > 0;
>   }
>   
> +/*
> + * Split into chunks and check interrupt pending between chunks.  This allows
> + * for timely injection of interrupts to prevent issues with guest lockup
> + * detection.
> + */
> +#define TDX_MAP_GPA_MAX_LEN (2 * 1024 * 1024)
> +static void __tdx_map_gpa(struct vcpu_tdx * tdx);
> +
> +static int tdx_complete_vmcall_map_gpa(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_tdx * tdx = to_tdx(vcpu);
> +
> +	if(vcpu->run->hypercall.ret) {
> +		tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND);
> +		kvm_r11_write(vcpu, tdx->map_gpa_next);
> +		return 1;
> +	}
> +
> +	tdx->map_gpa_next += TDX_MAP_GPA_MAX_LEN;
> +	if (tdx->map_gpa_next >= tdx->map_gpa_end) {
> +		tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_SUCCESS);
> +		return 1;
> +	}
> +
> +	/*
> +	 * Stop processing the remaining part if there is pending interrupt.
> +	 * Skip checking pending virtual interrupt (reflected by
> +	 * TDX_VCPU_STATE_DETAILS_INTR_PENDING bit) to save a seamcall because
> +	 * if guest disabled interrupt, it's OK not returning back to guest
> +	 * due to non-NMI interrupt. Also it's rare to TDVMCALL_MAP_GPA
> +	 * immediately after STI or MOV/POP SS.
> +	 */
> +	if (pi_has_pending_interrupt(vcpu) ||
> +	    kvm_test_request(KVM_REQ_NMI, vcpu) || vcpu->arch.nmi_pending) {
> +		tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_RETRY);
> +		kvm_r11_write(vcpu, tdx->map_gpa_next);
> +		return 1;
> +	}
> +
> +	__tdx_map_gpa(tdx);
> +	/* Forward request to userspace. */
> +	return 0;
> +}
> +
> +static void __tdx_map_gpa(struct vcpu_tdx * tdx)
> +{
> +	u64 gpa = tdx->map_gpa_next;
> +	u64 size = tdx->map_gpa_end - tdx->map_gpa_next;
> +
> +	if(size > TDX_MAP_GPA_MAX_LEN)
> +		size = TDX_MAP_GPA_MAX_LEN;
> +
> +	tdx->vcpu.run->exit_reason       = KVM_EXIT_HYPERCALL;
> +	tdx->vcpu.run->hypercall.nr      = KVM_HC_MAP_GPA_RANGE;
> +	tdx->vcpu.run->hypercall.args[0] = gpa & ~gfn_to_gpa(kvm_gfn_direct_bits(tdx->vcpu.kvm));
> +	tdx->vcpu.run->hypercall.args[1] = size / PAGE_SIZE;
> +	tdx->vcpu.run->hypercall.args[2] = vt_is_tdx_private_gpa(tdx->vcpu.kvm, gpa) ?
> +					   KVM_MAP_GPA_RANGE_ENCRYPTED :
> +					   KVM_MAP_GPA_RANGE_DECRYPTED;
> +	tdx->vcpu.run->hypercall.flags   = KVM_EXIT_HYPERCALL_LONG_MODE;
> +
> +	tdx->vcpu.arch.complete_userspace_io = tdx_complete_vmcall_map_gpa;
> +}
> +
> +static int tdx_map_gpa(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_tdx * tdx = to_tdx(vcpu);
> +	u64 gpa = tdvmcall_a0_read(vcpu);

We can use kvm_r12_read() directly, which is more intuitive. And we can 
drop the MACRO for a0/a1/a2/a3 accessors in patch 022.

> +	u64 size = tdvmcall_a1_read(vcpu);
> +	u64 ret;
> +
> +	/*
> +	 * Converting TDVMCALL_MAP_GPA to KVM_HC_MAP_GPA_RANGE requires
> +	 * userspace to enable KVM_CAP_EXIT_HYPERCALL with KVM_HC_MAP_GPA_RANGE
> +	 * bit set.  If not, the error code is not defined in GHCI for TDX, use
> +	 * TDVMCALL_STATUS_INVALID_OPERAND for this case.
> +	 */
> +	if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
> +		ret = TDVMCALL_STATUS_INVALID_OPERAND;
> +		goto error;
> +	}
> +
> +	if (gpa + size <= gpa || !kvm_vcpu_is_legal_gpa(vcpu, gpa) ||
> +	    !kvm_vcpu_is_legal_gpa(vcpu, gpa + size -1) ||
> +	    (vt_is_tdx_private_gpa(vcpu->kvm, gpa) !=
> +	     vt_is_tdx_private_gpa(vcpu->kvm, gpa + size -1))) {
> +		ret = TDVMCALL_STATUS_INVALID_OPERAND;
> +		goto error;
> +	}
> +
> +	if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(size)) {
> +		ret = TDVMCALL_STATUS_ALIGN_ERROR;
> +		goto error;
> +	}
> +
> +	tdx->map_gpa_end = gpa + size;
> +	tdx->map_gpa_next = gpa;
> +
> +	__tdx_map_gpa(tdx);
> +	/* Forward request to userspace. */
> +	return 0;

Maybe let __tdx_map_gpa() return a int to decide whether it needs to 
return to userspace and

	return __tdx_map_gpa(tdx);

?


> +
> +error:
> +	tdvmcall_set_return_code(vcpu, ret);
> +	kvm_r11_write(vcpu, gpa);
> +	return 1;
> +}
> +
>   static int handle_tdvmcall(struct kvm_vcpu *vcpu)
>   {
>   	if (tdvmcall_exit_type(vcpu))
>   		return tdx_emulate_vmcall(vcpu);
>   
>   	switch (tdvmcall_leaf(vcpu)) {
> +	case TDVMCALL_MAP_GPA:
> +		return tdx_map_gpa(vcpu);
>   	default:
>   		break;
>   	}
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index 1abc94b046a0..bfae70887695 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -71,6 +71,9 @@ struct vcpu_tdx {
>   
>   	enum tdx_prepare_switch_state prep_switch_state;
>   	u64 msr_host_kernel_gs_base;
> +
> +	u64 map_gpa_next;
> +	u64 map_gpa_end;
>   };
>   
>   void tdh_vp_rd_failed(struct vcpu_tdx *tdx, char *uclass, u32 field, u64 err);