Received-SPF: pass (google.com: domain of linux-kernel+bounces-144725-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: Mon, 15 Apr 2024 08:05:49 +0000
In-Reply-To: <ZhkhvtijbhxKKAEk@yzhao56-desk.sh.intel.com> (message from Yan
 Zhao on Fri, 12 Apr 2024 19:57:50 +0800)
Precedence: bulk
Mime-Version: 1.0
Message-ID: <diqzr0f7jbj6.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:

> On Fri, Apr 12, 2024 at 04:56:36AM +0000, Ackerley Tng wrote:
>> 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
> But above "__LINE__" is obviously not a valid GPA.
>
> Do you think it's better to check "data_gpa" is with shared bit on and
> aligned in 4K before setting bit 63?
>

I read "valid" in the spec to mean that the value in R13 "should be
considered as useful" or "should be passed on to the host VMM via the
TDX module", and not so much as in "validated".

We could validate the data_gpa as you suggested to check alignment and
shared bit, but perhaps that could be a higher-level function that calls
tdg_vp_vmcall_report_fatal_error()?

If it helps, shall we rename "data_gpa" to "data" for this lower-level,
generic helper function and remove these two lines

if (data_gpa)
	error_code |= 0x8000000000000000;

A higher-level function could perhaps do the validation as you suggested
and then set bit 63.

Are you objecting to the use of R13 to hold extra test information, such
as __LINE__?

I feel that R13 is just another register that could be used to hold
error information, and in the case of this test, we can use it to send
__LINE__ to aid in debugging selftests. On the host side of the
selftest we can printf() :).

>> 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>
>>