Message-ID: <aJpbbqsW_LIu2exc@google.com>
Date: Mon, 11 Aug 2025 14:06:54 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Sagi Shahar <sagis@...gle.com>
Cc: linux-kselftest@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>, 
	Shuah Khan <shuah@...nel.org>, Ackerley Tng <ackerleytng@...gle.com>, 
	Ryan Afranji <afranji@...gle.com>, Andrew Jones <ajones@...tanamicro.com>, 
	Isaku Yamahata <isaku.yamahata@...el.com>, Erdem Aktas <erdemaktas@...gle.com>, 
	Rick Edgecombe <rick.p.edgecombe@...el.com>, Roger Wang <runanwang@...gle.com>, 
	Binbin Wu <binbin.wu@...ux.intel.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	"Pratik R. Sampat" <pratikrajesh.sampat@....com>, Reinette Chatre <reinette.chatre@...el.com>, 
	Ira Weiny <ira.weiny@...el.com>, linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v8 24/30] KVM: selftests: TDX: Add shared memory test

On Thu, Aug 07, 2025, Sagi Shahar wrote:
> @@ -189,3 +199,19 @@ uint64_t tdg_vp_info(uint64_t *rcx, uint64_t *rdx,
>  
>  	return ret;
>  }
> +
> +uint64_t tdg_vp_vmcall_map_gpa(uint64_t address, uint64_t size, uint64_t *data_out)
> +{
> +	struct tdx_hypercall_args args = {
> +		.r11 = TDG_VP_VMCALL_MAP_GPA,
> +		.r12 = address,
> +		.r13 = size
> +	};
> +	uint64_t ret;
> +
> +	ret = __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT);
> +
> +	if (data_out)
> +		*data_out = args.r11;
> +	return ret;

Assert instead of returning the error.  If there's a use for negative tests, then
add a double-underscores variant.  And drop @data_out, IIUC, it's only relevant
on failure.

> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c b/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> index 5e4455be828a..c5bee67099c5 100644
> --- a/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> +++ b/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> @@ -608,4 +608,36 @@ void td_finalize(struct kvm_vm *vm)
>  void td_vcpu_run(struct kvm_vcpu *vcpu)
>  {
>  	vcpu_run(vcpu);
> +
> +	/* Handle TD VMCALLs that require userspace handling. */
> +	if (vcpu->run->exit_reason == KVM_EXIT_HYPERCALL &&
> +	    vcpu->run->hypercall.nr == KVM_HC_MAP_GPA_RANGE) {

Unnecessary curly braces.

> +		handle_userspace_map_gpa(vcpu);
> +	}
> +}
> +
> +/*
> + * Handle conversion of memory with @size beginning @gpa for @vm. Set
> + * @shared_to_private to true for shared to private conversions and false
> + * otherwise.
> + *
> + * Since this is just for selftests, just keep both pieces of backing
> + * memory allocated and not deallocate/allocate memory; just do the
> + * minimum of calling KVM_MEMORY_ENCRYPT_REG_REGION and
> + * KVM_MEMORY_ENCRYPT_UNREG_REGION.
> + */
> +void handle_memory_conversion(struct kvm_vm *vm, uint32_t vcpu_id, uint64_t gpa,
> +			      uint64_t size, bool shared_to_private)

So, vm_set_memory_attributes()?

> +{
> +	struct kvm_memory_attributes range;
> +
> +	range.address = gpa;
> +	range.size = size;
> +	range.attributes = shared_to_private ? KVM_MEMORY_ATTRIBUTE_PRIVATE : 0;
> +	range.flags = 0;
> +
> +	pr_debug("\t... call KVM_SET_MEMORY_ATTRIBUTES ioctl from vCPU %u with gpa=%#lx, size=%#lx, attributes=%#llx\n",
> +		 vcpu_id, gpa, size, range.attributes);

Drop these types of prints.  strace can probably do the job 99% of the time, and
for the remaining 1%, I doubt this help all that much.

> +
> +	vm_ioctl(vm, KVM_SET_MEMORY_ATTRIBUTES, &range);
>  }
> +void guest_shared_mem(void)
> +{
> +	uint32_t *test_mem_shared_gva =
> +		(uint32_t *)TDX_SHARED_MEM_TEST_SHARED_GVA;
> +
> +	uint64_t placeholder;
> +	uint64_t ret;
> +
> +	/* Map gpa as shared */
> +	ret = tdg_vp_vmcall_map_gpa(test_mem_shared_gpa, PAGE_SIZE,
> +				    &placeholder);
> +	if (ret)
> +		tdx_test_fatal_with_data(ret, __LINE__);
> +
> +	*test_mem_shared_gva = TDX_SHARED_MEM_TEST_GUEST_WRITE_VALUE;
> +
> +	/* Exit so host can read shared value */
> +	ret = tdg_vp_vmcall_instruction_io(TDX_SHARED_MEM_TEST_INFO_PORT, 4,
> +					   PORT_WRITE, &placeholder);
> +	if (ret)

GUEST_ASSERT().  Don't use TDX's "fatal error" crud to report test failures.

> +		tdx_test_fatal_with_data(ret, __LINE__);
> +
> +	/* Read value written by host and send it back out for verification */
> +	ret = tdg_vp_vmcall_instruction_io(TDX_SHARED_MEM_TEST_INFO_PORT, 4,
> +					   PORT_WRITE,
> +					   (uint64_t *)test_mem_shared_gva);
> +	if (ret)
> +		tdx_test_fatal_with_data(ret, __LINE__);
> +}
> +
> +int verify_shared_mem(void)
> +{
> +	vm_vaddr_t test_mem_private_gva;
> +	uint64_t test_mem_private_gpa;
> +	uint32_t *test_mem_hva;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +
> +	vm = td_create();
> +	td_initialize(vm, VM_MEM_SRC_ANONYMOUS, 0);
> +	vcpu = td_vcpu_add(vm, 0, guest_shared_mem);
> +
> +	/*
> +	 * Set up shared memory page for testing by first allocating as private
> +	 * and then mapping the same GPA again as shared. This way, the TD does
> +	 * not have to remap its page tables at runtime.
> +	 */
> +	test_mem_private_gva = vm_vaddr_alloc(vm, vm->page_size,
> +					      TDX_SHARED_MEM_TEST_PRIVATE_GVA);
> +	TEST_ASSERT_EQ(test_mem_private_gva, TDX_SHARED_MEM_TEST_PRIVATE_GVA);
> +
> +	test_mem_hva = addr_gva2hva(vm, test_mem_private_gva);
> +	TEST_ASSERT(test_mem_hva,
> +		    "Guest address not found in guest memory regions\n");
> +
> +	test_mem_private_gpa = addr_gva2gpa(vm, test_mem_private_gva);
> +	virt_map_shared(vm, TDX_SHARED_MEM_TEST_SHARED_GVA, test_mem_private_gpa, 1);
> +
> +	test_mem_shared_gpa = test_mem_private_gpa | vm->arch.s_bit;
> +	sync_global_to_guest(vm, test_mem_shared_gpa);
> +
> +	td_finalize(vm);
> +
> +	vm_enable_cap(vm, KVM_CAP_EXIT_HYPERCALL, BIT_ULL(KVM_HC_MAP_GPA_RANGE));
> +
> +	printf("Verifying shared memory accesses for TDX\n");
> +
> +	/* Begin guest execution; guest writes to shared memory. */
> +	printf("\t ... Starting guest execution\n");
> +
> +	/* Handle map gpa as shared */
> +	tdx_run(vcpu);
> +
> +	tdx_run(vcpu);
> +	tdx_test_assert_io(vcpu, TDX_SHARED_MEM_TEST_INFO_PORT, 4, PORT_WRITE);

AFAICT, there's nothing TDX-specific about these assert helpers.  And I would
prefer they be macros; while ugly, macros provide precise file+line information,
i.e. don't require a stack trace.

Maybe TEST_ASSERT_EXIT_IO() and TEST_ASSERT_EXIT_MMIO()?

> +	TEST_ASSERT_EQ(*test_mem_hva, TDX_SHARED_MEM_TEST_GUEST_WRITE_VALUE);
> +
> +	*test_mem_hva = TDX_SHARED_MEM_TEST_HOST_WRITE_VALUE;
> +	tdx_run(vcpu);
> +	tdx_test_assert_io(vcpu, TDX_SHARED_MEM_TEST_INFO_PORT, 4, PORT_WRITE);
> +	TEST_ASSERT_EQ(*(uint32_t *)((void *)vcpu->run + vcpu->run->io.data_offset),
> +		       TDX_SHARED_MEM_TEST_HOST_WRITE_VALUE);
> +
> +	printf("\t ... PASSED\n");

No.  Printing PASSED is worse than useless.  Exit codes exist for a reason; this
is pure spam.  pr_debug() if necessary, but all of these printfs can probably be
dropped.

> +
> +	kvm_vm_free(vm);
> +
> +	return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	if (!is_tdx_enabled()) {
> +		printf("TDX is not supported by the KVM\n"
> +		       "Skipping the TDX tests.\n");
> +		return 0;

TEST_REQUIRE()

> +	}
> +
> +	return verify_shared_mem();
> +}
> -- 
> 2.51.0.rc0.155.g4a0f42376b-goog
> 

Powered by blists - more mailing lists