Received-SPF: pass (google.com: domain of linux-kernel+bounces-142616-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 12 Apr 2024 19:57:50 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Ackerley Tng <ackerleytng@google.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>
Subject: Re: [RFC PATCH v5 09/29] KVM: selftests: TDX: Add report_fatal_error
 test
Message-ID: <ZhkhvtijbhxKKAEk@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <ZeHFlsrBKWR6bfRZ@yzhao56-desk.sh.intel.com>
 <diqzwmp3ji0r.fsf@ctop-sg.c.googlers.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <diqzwmp3ji0r.fsf@ctop-sg.c.googlers.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?m1dKb71r9T9N1thuLe0qtByWU8hRYf8iU00IiSv+4RaFTmFstoBzqDKFcsY9?=
 =?us-ascii?Q?yaL8F9gYu89lAnrkNHeYr3xvprJW4y0xd1qYqRh4jJoCW1thOlhkSuVvmh3q?=
 =?us-ascii?Q?LlDbKnNZU13x2ljkH6FaHOjssq0lHWffdIrZGPcQ348Yv3XleUlDhmIov33m?=
 =?us-ascii?Q?XWGWQjYB4BrfPjfL/wABOUcliVGIliR2nKVppiWY2KSpH4CXiC7nuy/EpiWb?=
 =?us-ascii?Q?sqA6NU6EShOhWCTAANohVLdQnzIHtAEt0+1Rwo2gDvboHXbSiBcHB0IobIga?=
 =?us-ascii?Q?nYxbDp854g7YREvgV55nhGK4TW8G5kVZp1prLZkM3gJYUsozjQ1jxrcwOO4V?=
 =?us-ascii?Q?W4AAskLKEO9sHbGnS3Ox4GlFWaZDDoCP6qsugbAUPgt65L9kCqiUgo4l0VXW?=
 =?us-ascii?Q?i0y47bnUJ030h0erw875SmW7AbdRt5M6pTP2XbkjE3KJbl8n+r7zVl6xkJnS?=
 =?us-ascii?Q?OrbbSx9n/WpiUrEhDGOIO9FZ801Sj96UYZRK+NVT4+kzsNATg2KWEma7tOsX?=
 =?us-ascii?Q?BIQl6bV9ddPxSlnXhmYIszf4JFoukpCsaYPSuvoj3QnWJs7mRrJzuptP1jsT?=
 =?us-ascii?Q?hoMFZJVJAX9RAeVrNg1hjoxVeJyhxzZyPtip2f+aDm8gXc+ITEsjN4S5gRvC?=
 =?us-ascii?Q?L0E9tEKbjfEywu52eaLSkaPVoGS8+A4AmYj8odeM/R0ZzHo4Rd9eCuJHNqEg?=
 =?us-ascii?Q?n6cWuU1d1xph3nq3kVAKsp28HEzG+/Mqgowo+WDheNsEKwWHmlzfzjFG/w3s?=
 =?us-ascii?Q?y2ChPMoQD9NMxg++IrOldz6SKxJogZM19cFeQp6epEa7uyw8p4V4STkMyyfL?=
 =?us-ascii?Q?jjIPOwmLl7QrrDIB0TteI0yXdqv8wDSMh/pknrSl7ruALgZlUz/FuLU8FYft?=
 =?us-ascii?Q?HqYOYIjk80s7G3zBky9kZJkf4BNux85XG+cxgSMfoITs/+UG9YqLnTOjudpT?=
 =?us-ascii?Q?oEGf4U+q3UhHU2ubpIpUmWRrXisjEjUKoAIbRQlS9AbHQNove0z+gnHcnyA6?=
 =?us-ascii?Q?WlD0dEYHB3o6+o6zZRsEivbIB+SSHTVUISKAhgHgRvYQx31vLMpTf9P4mWs8?=
 =?us-ascii?Q?buEWHJAGAovMAor9E/qSYBEuYMsw3GcgKsLg6A8bqzTpsWQ263osgvNVmGqc?=
 =?us-ascii?Q?GYYrwK3HkSWjAjhjdhRQT8gcQZxXXLYsDugjcWBYtd489JhKWgqsDW7j4Wo/?=
 =?us-ascii?Q?LUkQ5XtUIO9FpAA1D1MUZP3DMQ7q1DLilebO+hbeSln3fI5dRe9ie6YbUV7I?=
 =?us-ascii?Q?QPlw0gAm5xKvyCf7R2jY1f5kKqS/fNgPaoHOvYzc/ZH53j4LJKBCOeGrclaW?=
 =?us-ascii?Q?1kVOtcizvpvebKFSEuwYH2ImCn+VTcwHCmqTgeKAVTTEpshzHzXqgy86NdaC?=
 =?us-ascii?Q?SoTWZr9+IwQytUmZY5u5YNiHdCktmsAgZJwCL3UJLLQ9mczgMowgqPlr0Jon?=
 =?us-ascii?Q?WOEPrI1rThizOZS3ywEy3Hfzop9WJjLV93XLZvXg98MYGqbKD85uugcYEJAO?=
 =?us-ascii?Q?g+n4fLYEOTY4V3Ua4lSutZCtV3kxdsL4sEpTzTqniuBCq5KGRkQuVLj98L5L?=
 =?us-ascii?Q?Hdlw30Rcu8aKuarF5Z9KnWNp8sEepCU9PmI43eVX?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 103cef75-9643-494f-b476-08dc5ae7d7e2
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2024 11:58:18.7060
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nj2/AhWtZjnMeT2/lv7RmYW9immonWG4t/MCGnZwyXP6JOso/9WLXgdMpwg0khg0nCkPmdx6qIWInSITG0vKpw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7206
X-OriginatorOrg: intel.com

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?

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