Received-SPF: pass (google.com: domain of linux-kernel+bounces-141945-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 12 Apr 2024 04:56:36 +0000
In-Reply-To: <ZeHFlsrBKWR6bfRZ@yzhao56-desk.sh.intel.com> (message from Yan
 Zhao on Fri, 1 Mar 2024 20:09:58 +0800)
Precedence: bulk
Mime-Version: 1.0
Message-ID: <diqzwmp3ji0r.fsf@ctop-sg.c.googlers.com>
Subject: Re: [RFC PATCH v5 09/29] KVM: selftests: TDX: Add report_fatal_error test
From: Ackerley Tng <ackerleytng@google.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: sagis@google.com, linux-kselftest@vger.kernel.org, afranji@google.com, 
	erdemaktas@google.com, isaku.yamahata@intel.com, seanjc@google.com, 
	pbonzini@redhat.com, shuah@kernel.org, pgonda@google.com, haibo1.xu@intel.com, 
	chao.p.peng@linux.intel.com, vannapurve@google.com, runanwang@google.com, 
	vipinsh@google.com, jmattson@google.com, dmatlack@google.com, 
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"

Yan Zhao <yan.y.zhao@intel.com> writes:

> ...
>> diff --git a/tools/testing/selftests/kvm/include/x86_64/tdx/test_util.h b/tools/testing/selftests/kvm/include/x86_64/tdx/test_util.h
>> index b570b6d978ff..6d69921136bd 100644
>> --- a/tools/testing/selftests/kvm/include/x86_64/tdx/test_util.h
>> +++ b/tools/testing/selftests/kvm/include/x86_64/tdx/test_util.h
>> @@ -49,4 +49,23 @@ bool is_tdx_enabled(void);
>>   */
>>  void tdx_test_success(void);
>>  
>> +/**
>> + * Report an error with @error_code to userspace.
>> + *
>> + * Return value from tdg_vp_vmcall_report_fatal_error is ignored since execution
>> + * is not expected to continue beyond this point.
>> + */
>> +void tdx_test_fatal(uint64_t error_code);
>> +
>> +/**
>> + * Report an error with @error_code to userspace.
>> + *
>> + * @data_gpa may point to an optional shared guest memory holding the error
>> + * string.
>> + *
>> + * Return value from tdg_vp_vmcall_report_fatal_error is ignored since execution
>> + * is not expected to continue beyond this point.
>> + */
>> +void tdx_test_fatal_with_data(uint64_t error_code, uint64_t data_gpa);
> I found nowhere is using "data_gpa" as a gpa, even in patch 23, it's
> usage is to pass a line number ("tdx_test_fatal_with_data(ret, __LINE__)").
>
>

This function tdx_test_fatal_with_data() is meant to provide a generic
interface for TDX tests to use TDG.VP.VMCALL<ReportFatalError>, and so
the parameters of tdx_test_fatal_with_data() generically allow error_code and
data_gpa to be specified.

The tests just happen to use the data_gpa parameter to pass __LINE__ to
the host VMM, but other tests in future that use the
tdx_test_fatal_with_data() function in the TDX testing library could
actually pass a GPA through using data_gpa.

>>  #endif // SELFTEST_TDX_TEST_UTIL_H
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c b/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c
>> index c2414523487a..b854c3aa34ff 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c
>> @@ -1,8 +1,31 @@
>>  // SPDX-License-Identifier: GPL-2.0-only
>>  
>> +#include <string.h>
>> +
>>  #include "tdx/tdcall.h"
>>  #include "tdx/tdx.h"
>>  
>> +void handle_userspace_tdg_vp_vmcall_exit(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm_tdx_vmcall *vmcall_info = &vcpu->run->tdx.u.vmcall;
>> +	uint64_t vmcall_subfunction = vmcall_info->subfunction;
>> +
>> +	switch (vmcall_subfunction) {
>> +	case TDG_VP_VMCALL_REPORT_FATAL_ERROR:
>> +		vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>> +		vcpu->run->system_event.ndata = 3;
>> +		vcpu->run->system_event.data[0] =
>> +			TDG_VP_VMCALL_REPORT_FATAL_ERROR;
>> +		vcpu->run->system_event.data[1] = vmcall_info->in_r12;
>> +		vcpu->run->system_event.data[2] = vmcall_info->in_r13;
>> +		vmcall_info->status_code = 0;
>> +		break;
>> +	default:
>> +		TEST_FAIL("TD VMCALL subfunction %lu is unsupported.\n",
>> +			  vmcall_subfunction);
>> +	}
>> +}
>> +
>>  uint64_t tdg_vp_vmcall_instruction_io(uint64_t port, uint64_t size,
>>  				      uint64_t write, uint64_t *data)
>>  {
>> @@ -25,3 +48,19 @@ uint64_t tdg_vp_vmcall_instruction_io(uint64_t port, uint64_t size,
>>  
>>  	return ret;
>>  }
>> +
>> +void tdg_vp_vmcall_report_fatal_error(uint64_t error_code, uint64_t data_gpa)
>> +{
>> +	struct tdx_hypercall_args args;
>> +
>> +	memset(&args, 0, sizeof(struct tdx_hypercall_args));
>> +
>> +	if (data_gpa)
>> +		error_code |= 0x8000000000000000;
>> 
> So, why this error_code needs to set bit 63?
>
>

The Intel GHCI Spec says in R12, bit 63 is set if the GPA is valid. As a
generic TDX testing library function, this check allows the user to use
tdg_vp_vmcall_report_fatal_error() with error_code and data_gpa and not
worry about setting bit 63 before calling
tdg_vp_vmcall_report_fatal_error(), though if the user set bit 63 before
that, there is no issue.

>> +	args.r11 = TDG_VP_VMCALL_REPORT_FATAL_ERROR;
>> +	args.r12 = error_code;
>> +	args.r13 = data_gpa;
>> +
>> +	__tdx_hypercall(&args, 0);
>> +}

>> <snip>