Received-SPF: pass (google.com: domain of linux-kernel+bounces-90344-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 4 Mar 2024 17:18:30 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Binbin Wu <binbin.wu@linux.intel.com>
CC: Sagi Shahar <sagis@google.com>, <linux-kselftest@vger.kernel.org>,
	Ackerley Tng <ackerleytng@google.com>, Ryan Afranji <afranji@google.com>,
	Erdem Aktas <erdemaktas@google.com>, Isaku Yamahata
	<isaku.yamahata@intel.com>, Sean Christopherson <seanjc@google.com>, "Paolo
 Bonzini" <pbonzini@redhat.com>, Shuah Khan <shuah@kernel.org>, Peter Gonda
	<pgonda@google.com>, Haibo Xu <haibo1.xu@intel.com>, Chao Peng
	<chao.p.peng@linux.intel.com>, Vishal Annapurve <vannapurve@google.com>,
	Roger Wang <runanwang@google.com>, Vipin Sharma <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 10/29] KVM: selftests: TDX: Adding test case for
 TDX port IO
Message-ID: <ZeWR5loUdXfMm/Wp@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20231212204647.2170650-1-sagis@google.com>
 <20231212204647.2170650-11-sagis@google.com>
 <ZeUvtzHmMo9jdMnu@yzhao56-desk.sh.intel.com>
 <0344d85f-d6ab-4d78-abac-d0293d71ef91@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <0344d85f-d6ab-4d78-abac-d0293d71ef91@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?qFFul5d2jTDHufIItg37dKIPkoFpBB13+3T9JarGhFrIJTbi/SqS88Se5OP4?=
 =?us-ascii?Q?h3v8WThvmVsg3oKC3OryS1rhECb99XO2Cvcom7/bDgsc2aSReURiz3ewX1q4?=
 =?us-ascii?Q?8u0kaEFy8gra/W4wL6KB/5vnJnm+mJVMar9ikFtGCnzT/ifdYseS/HWLWutY?=
 =?us-ascii?Q?eIl6Sv7rDPPk434a7sc4gfu3DAhY9ms7TPbsL7maCJPK6wPQuCX9ow+P1BCn?=
 =?us-ascii?Q?STt5wj8QhSiUdClcckVAmjBYB9+05rg+ChgMQOrOsA6RL0V6oh5FqD1eDUcH?=
 =?us-ascii?Q?Nz675t2EzkVlMNdjPnAWu6B144wWGot0D2H6pZWzS83A8AlhTCrMWIEShXpH?=
 =?us-ascii?Q?viX1o807LpJeMiM1efRD90s7jHb53Pjx1Qf15AXlZxf+SW9eYfcMfOsxUNce?=
 =?us-ascii?Q?y6PpJp7H5MEbcRyAPCI1CdrO1VN7vSC5j3fyrD7EXniKuA2id3RgCfdPZ4an?=
 =?us-ascii?Q?N4gvceIbfYuwSv6t/vbEj0pcS8p967cAsvouRYAL3/hnE2eSqlTFUBvtMWyC?=
 =?us-ascii?Q?dbc5B5e5mrPv8OODqviBLHRVruJkSqlKoTXjjqGjlSXkiXv3gJ/lBqmTTuQ5?=
 =?us-ascii?Q?FafL0OZXLllW6RKartfU4iQf1ISSL71HVE5gN3gypwtwhQC8Fqwq1uWkDRbQ?=
 =?us-ascii?Q?VUbpibmE+LB2G3mGMxd9yrYNj8vrFc0OktqeAY6rX4JIxtI92E5nAA13C6pj?=
 =?us-ascii?Q?IQ+wtPJOxjep3Va4TNfD/xBRSG9KvW7z5hd/w8aF517uyRQN80NMgO93U1OJ?=
 =?us-ascii?Q?ANas+vIVNBRqjnTu4XIGs4OsoVl61aReSf01PIQG81+xiPcD8ekGV+LwgdXN?=
 =?us-ascii?Q?8Y/55HG5YjBELrayggYvogJwN8kmOxbjFlK/dnWwWsFzWgg7NXqV6yAlgMOK?=
 =?us-ascii?Q?nxX5vp7R2uLbRChH52mVXkEsMun1c2YKSEbN1bZJ1hwViW33tVn7lT0vQt7g?=
 =?us-ascii?Q?MmtTUFLPaW99m0sKD4TIs4ZwqnpfHSOy08Nc8wUtaYu1l7HSP7V9zZpJ4kZs?=
 =?us-ascii?Q?ddxL4cYn0T3MBdv422ADYvEwhl54Ps1Jn6w6OP2CCLWvLRPP66X+qbRsgCfa?=
 =?us-ascii?Q?DhJBC4MifDsM/tswLxGPYw+W/tZIAMwUBcOojGZVX+5Akl37op4lJxaF9EHr?=
 =?us-ascii?Q?n0Kl/ssyVQ4BLdYJdKxMqgL6/ei+7dg/lU8al9ABXilpu9gJC/YF61R6oNhy?=
 =?us-ascii?Q?PuZneS7r38xU1JQgtLAERE3wTShkoYZcV91uYckS7iS+hMF0rIydiQ1EUa9c?=
 =?us-ascii?Q?Mj50tQNSzd80J35v4HeSU9q8czgrCsMACqPxL1fyqIHuJl2JJTc0h9aiv82z?=
 =?us-ascii?Q?/TKc0GVVunG+pDe/+QaqSfxdCXBQ7kn4NdIv1o6M5FB56IS+YcgHtSgi64Qi?=
 =?us-ascii?Q?823vnY5ZIW3oseKSK0rvLv0GPlZNDCSyoqeLlNgKYHZ6fgQV2R1hzIgntT/w?=
 =?us-ascii?Q?4SByZRQlu8jb2wVAI1FiuXhuVXfVt5UjD+zOgiysvW0weQJe+bl1fg1RsUmN?=
 =?us-ascii?Q?pg7stPBRcL0Z/5tHkyHRnC/TShMzvL5GLcw1psll08RUSbxLMyv2lkqFdKu4?=
 =?us-ascii?Q?+mCrUmx8bAlOqRyR0XzHYH23PGfuUkrBhW9iVeM4?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4d334e4f-9aee-48d4-4a41-08dc3c303a70
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 09:48:22.1772
 (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: c4+VzFN9UYhWUDbX6tvcDBoXOh9TBYorTRnLxL0OiwuHuUJjY/emCffxLNUu/s6cROZ/XPto3b6gI5Na5bb8yQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB8267
X-OriginatorOrg: intel.com

On Mon, Mar 04, 2024 at 05:16:53PM +0800, Binbin Wu wrote:
> 
> 
> On 3/4/2024 10:19 AM, Yan Zhao wrote:
> > On Tue, Dec 12, 2023 at 12:46:25PM -0800, Sagi Shahar wrote:
> > > From: Erdem Aktas <erdemaktas@google.com>
> > > 
> > > Verifies TDVMCALL<INSTRUCTION.IO> READ and WRITE operations.
> > > 
> > > Signed-off-by: Erdem Aktas <erdemaktas@google.com>
> > > Signed-off-by: Sagi Shahar <sagis@google.com>
> > > Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> > > Signed-off-by: Ryan Afranji <afranji@google.com>
> > > ---
> > >   .../kvm/include/x86_64/tdx/test_util.h        | 34 ++++++++
> > >   .../selftests/kvm/x86_64/tdx_vm_tests.c       | 82 +++++++++++++++++++
> > >   2 files changed, 116 insertions(+)
> > > 
> > > 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 6d69921136bd..95a5d5be7f0b 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
> > > @@ -9,6 +9,40 @@
> > >   #define TDX_TEST_SUCCESS_PORT 0x30
> > >   #define TDX_TEST_SUCCESS_SIZE 4
> > > +/**
> > > + * Assert that some IO operation involving tdg_vp_vmcall_instruction_io() was
> > > + * called in the guest.
> > > + */
> > > +#define TDX_TEST_ASSERT_IO(VCPU, PORT, SIZE, DIR)			\
> > > +	do {								\
> > > +		TEST_ASSERT((VCPU)->run->exit_reason == KVM_EXIT_IO,	\
> > > +			"Got exit_reason other than KVM_EXIT_IO: %u (%s)\n", \
> > > +			(VCPU)->run->exit_reason,			\
> > > +			exit_reason_str((VCPU)->run->exit_reason));	\
> > > +									\
> > > +		TEST_ASSERT(((VCPU)->run->exit_reason == KVM_EXIT_IO) && \
> > > +			((VCPU)->run->io.port == (PORT)) &&		\
> > > +			((VCPU)->run->io.size == (SIZE)) &&		\
> > > +			((VCPU)->run->io.direction == (DIR)),		\
> > > +			"Got unexpected IO exit values: %u (%s) %d %d %d\n", \
> > > +			(VCPU)->run->exit_reason,			\
> > > +			exit_reason_str((VCPU)->run->exit_reason),	\
> > > +			(VCPU)->run->io.port, (VCPU)->run->io.size,	\
> > > +			(VCPU)->run->io.direction);			\
> > > +	} while (0)
> > > +
> > > +/**
> > > + * Check and report if there was some failure in the guest, either an exception
> > > + * like a triple fault, or if a tdx_test_fatal() was hit.
> > > + */
> > > +#define TDX_TEST_CHECK_GUEST_FAILURE(VCPU)				\
> > > +	do {								\
> > > +		if ((VCPU)->run->exit_reason == KVM_EXIT_SYSTEM_EVENT)	\
> > > +			TEST_FAIL("Guest reported error. error code: %lld (0x%llx)\n", \
> > > +				(VCPU)->run->system_event.data[1],	\
> > > +				(VCPU)->run->system_event.data[1]);	\
> > > +	} while (0)
> > > +
> > >   /**
> > >    * Assert that tdx_test_success() was called in the guest.
> > >    */
> > > diff --git a/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c b/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c
> > > index 8638c7bbedaa..75467c407ca7 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c
> > > @@ -2,6 +2,7 @@
> > >   #include <signal.h>
> > >   #include "kvm_util_base.h"
> > > +#include "tdx/tdcall.h"
> > >   #include "tdx/tdx.h"
> > >   #include "tdx/tdx_util.h"
> > >   #include "tdx/test_util.h"
> > > @@ -74,6 +75,86 @@ void verify_report_fatal_error(void)
> > >   	printf("\t ... PASSED\n");
> > >   }
> > > +#define TDX_IOEXIT_TEST_PORT 0x50
> > > +
> > > +/*
> > > + * Verifies IO functionality by writing a |value| to a predefined port.
> > > + * Verifies that the read value is |value| + 1 from the same port.
> > > + * If all the tests are passed then write a value to port TDX_TEST_PORT
> > > + */
> > > +void guest_ioexit(void)
> > > +{
> > > +	uint64_t data_out, data_in, delta;
> > > +	uint64_t ret;
> > > +
> > > +	data_out = 0xAB;
> > > +	ret = tdg_vp_vmcall_instruction_io(TDX_IOEXIT_TEST_PORT, 1,
> > > +					TDG_VP_VMCALL_INSTRUCTION_IO_WRITE,
> > > +					&data_out);
> > > +	if (ret)
> > > +		tdx_test_fatal(ret);
> > > +
> > > +	ret = tdg_vp_vmcall_instruction_io(TDX_IOEXIT_TEST_PORT, 1,
> > > +					TDG_VP_VMCALL_INSTRUCTION_IO_READ,
> > > +					&data_in);
> > > +	if (ret)
> > > +		tdx_test_fatal(ret);
> > > +
> > > +	delta = data_in - data_out;
> > > +	if (delta != 1)
> > > +		tdx_test_fatal(ret);
> > > +
> > > +	tdx_test_success();
> > > +}
> > > +
> > > +void verify_td_ioexit(void)
> > > +{
> > > +	struct kvm_vm *vm;
> > > +	struct kvm_vcpu *vcpu;
> > > +
> > > +	uint32_t port_data;
> > > +
> > > +	vm = td_create();
> > > +	td_initialize(vm, VM_MEM_SRC_ANONYMOUS, 0);
> > > +	vcpu = td_vcpu_add(vm, 0, guest_ioexit);
> > > +	td_finalize(vm);
> > > +
> > > +	printf("Verifying TD IO Exit:\n");
> > > +
> > > +	/* Wait for guest to do a IO write */
> > > +	td_vcpu_run(vcpu);
> > > +	TDX_TEST_CHECK_GUEST_FAILURE(vcpu);
> > This check is a vain, because the first VMExit from vcpu run is always
> > KVM_EXIT_IO caused by tdg_vp_vmcall_instruction_io().
> 
> I think tdg_vp_vmcall_instruction_io() could fail if RCX (GPR select)
> doesn't
> meet the requirement (some bits must be 0).
> Although RCX is set by guest code (in selftest, it set in __tdx_hypercall())
> and it will not trigger the error, it still can be used as a guard to make
> sure guest doesn't pass a invalid RCX.
>
Right. This check can be kept in case an failure is delivered to TD in host
kernel directly, though it cannot guard if the failure will trigger an
exit to user space (e.g. if kernel TDX code has a bug).
> 
> > 
> > 
> > > +	TDX_TEST_ASSERT_IO(vcpu, TDX_IOEXIT_TEST_PORT, 1,
> > > +			TDG_VP_VMCALL_INSTRUCTION_IO_WRITE);
> > > +	port_data = *(uint8_t *)((void *)vcpu->run + vcpu->run->io.data_offset);
> > > +
> > > +	printf("\t ... IO WRITE: OK\n");
> > So,  even if there's an error in emulating writing of TDX_IOEXIT_TEST_PORT,
> > and guest would then find a failure and trigger tdx_test_fatal(), this line
> > will still print "IO WRITE: OK", which is not right.
> > 
> > > +
> > > +	/*
> > > +	 * Wait for the guest to do a IO read. Provide the previous written data
> > > +	 * + 1 back to the guest
> > > +	 */
> > > +	td_vcpu_run(vcpu);
> > > +	TDX_TEST_CHECK_GUEST_FAILURE(vcpu);
> > This check is a vain, too, as in  write case.
> > 
> > > +	TDX_TEST_ASSERT_IO(vcpu, TDX_IOEXIT_TEST_PORT, 1,
> > > +			TDG_VP_VMCALL_INSTRUCTION_IO_READ);
> > > +	*(uint8_t *)((void *)vcpu->run + vcpu->run->io.data_offset) = port_data + 1;
> > > +
> > > +	printf("\t ... IO READ: OK\n");
> > Same as in write case, this line should not be printed until after guest
> > finishing checking return code.
> > 
> > > +
> > > +	/*
> > > +	 * Wait for the guest to complete execution successfully. The read
> > > +	 * value is checked within the guest.
> > > +	 */
> > > +	td_vcpu_run(vcpu);
> > > +	TDX_TEST_CHECK_GUEST_FAILURE(vcpu);
> > > +	TDX_TEST_ASSERT_SUCCESS(vcpu);
> > > +
> > > +	printf("\t ... IO verify read/write values: OK\n");
> > > +	kvm_vm_free(vm);
> > > +	printf("\t ... PASSED\n");
> > > +}
> > > +
> > >   int main(int argc, char **argv)
> > >   {
> > >   	setbuf(stdout, NULL);
> > > @@ -85,6 +166,7 @@ int main(int argc, char **argv)
> > >   	run_in_new_process(&verify_td_lifecycle);
> > >   	run_in_new_process(&verify_report_fatal_error);
> > > +	run_in_new_process(&verify_td_ioexit);
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.43.0.472.g3155946c3a-goog
> > > 
> > > 
>