Message-ID: <689cb7b077704_20a6d929455@iweiny-mobl.notmuch>
Date: Wed, 13 Aug 2025 11:05:04 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Sagi Shahar <sagis@...gle.com>, <linux-kselftest@...r.kernel.org>, "Paolo
 Bonzini" <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>, "Sean
 Christopherson" <seanjc@...gle.com>, 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>
CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>
Subject: Re: [PATCH v8 28/30] KVM: selftests: TDX: Add TDX UPM selftest

Sagi Shahar wrote:
> From: Ackerley Tng <ackerleytng@...gle.com>

NIT: UPM?

Also for consistency with the next patch:

	"KVM: selftests: TDX: Add TDX UPM selftests for explicit conversion"

> 
> This tests the use of guest memory with explicit TDG.VP.VMCALL<MapGPA>
> calls.
> 

[snip]

> +
> +/*
> + * 0x80000000 is arbitrarily selected. The selected address need not be the same
> + * as TDX_UPM_TEST_AREA_GVA_PRIVATE, but it should not overlap with selftest
> + * code or boot page.
> + */
> +#define TDX_UPM_TEST_AREA_GPA (0x80000000)
> +/* Test area GPA is arbitrarily selected */
> +#define TDX_UPM_TEST_AREA_GVA_PRIVATE (0x90000000)
> +/* Select any bit that can be used as a flag */
> +#define TDX_UPM_TEST_AREA_GVA_SHARED_BIT (32)
> +/*
> + * TDX_UPM_TEST_AREA_GVA_SHARED is used to map the same GPA twice into the
> + * guest, once as shared and once as private
> + */
> +#define TDX_UPM_TEST_AREA_GVA_SHARED				\
> +	(TDX_UPM_TEST_AREA_GVA_PRIVATE |			\
> +		BIT_ULL(TDX_UPM_TEST_AREA_GVA_SHARED_BIT))
> +
> +/* The test area is 2MB in size */
> +#define TDX_UPM_TEST_AREA_SIZE SZ_2M
> +/* 0th general area is 1MB in size */
> +#define TDX_UPM_GENERAL_AREA_0_SIZE SZ_1M
> +/* Focus area is 40KB in size */
> +#define TDX_UPM_FOCUS_AREA_SIZE (SZ_32K + SZ_8K)
> +/* 1st general area is the rest of the space in the test area */
> +#define TDX_UPM_GENERAL_AREA_1_SIZE				\
> +	(TDX_UPM_TEST_AREA_SIZE - TDX_UPM_GENERAL_AREA_0_SIZE -	\
> +		TDX_UPM_FOCUS_AREA_SIZE)
> +
> +/*
> + * The test memory area is set up as two general areas, sandwiching a focus
> + * area.  The general areas act as control areas. After they are filled, they
> + * are not expected to change throughout the tests. The focus area is memory
> + * permissions change from private to shared and vice-versa.
> + *
> + * The focus area is intentionally small, and sandwiched to test that when the
> + * focus area's permissions change, the other areas' permissions are not
> + * affected.
> + */
> +struct __packed tdx_upm_test_area {
> +	uint8_t general_area_0[TDX_UPM_GENERAL_AREA_0_SIZE];
> +	uint8_t focus_area[TDX_UPM_FOCUS_AREA_SIZE];
> +	uint8_t general_area_1[TDX_UPM_GENERAL_AREA_1_SIZE];
> +};

Is this really needed with the defines and helpers you have?

> +
> +static void fill_test_area(struct tdx_upm_test_area *test_area_base,
> +			   uint8_t pattern)
> +{
> +	memset(test_area_base, pattern, sizeof(*test_area_base));
> +}
> +
> +static void fill_focus_area(struct tdx_upm_test_area *test_area_base,
> +			    uint8_t pattern)
> +{
> +	memset(test_area_base->focus_area, pattern,
> +	       sizeof(test_area_base->focus_area));
> +}
> +
> +static bool check_area(uint8_t *base, uint64_t size, uint8_t expected_pattern)

memchr_inv()?

> +{
> +	size_t i;
> +
> +	for (i = 0; i < size; i++) {
> +		if (base[i] != expected_pattern)
> +			return false;
> +	}
> +
> +	return true;
> +}

[snip]

> +
> +
> +#define TDX_UPM_TEST_ASSERT(x)				\
> +	do {						\
> +		if (!(x))				\
> +			tdx_test_fatal(__LINE__);	\

I think Sean mentioned he did not want to use the tdx_* error functions.
And why is a special assert needed for this test only?

> +	} while (0)
> +
> +/*
> + * Shared variables between guest and host
> + */
> +static struct tdx_upm_test_area *test_area_gpa_private;
> +static struct tdx_upm_test_area *test_area_gpa_shared;
> +
> +/*
> + * Test stages for syncing with host
> + */
> +enum {
> +	SYNC_CHECK_READ_PRIVATE_MEMORY_FROM_HOST = 1,
> +	SYNC_CHECK_READ_SHARED_MEMORY_FROM_HOST,
> +	SYNC_CHECK_READ_PRIVATE_MEMORY_FROM_HOST_AGAIN,
> +};

I don't follow what these are used for.  It seems like a synchronization
mechanism between the guest and host test code?  But I don't see any state
machine which is transitioning from 1 stage to the next.

Ah I think I see it now.  These are not the test stages.  Rather they are
the return values that the guess is sending to the host to signal
completion of each stage.

> +
> +#define TDX_UPM_TEST_ACCEPT_PRINT_PORT 0x87
> +
> +/*
> + * Does vcpu_run, and also manages memory conversions if requested by the TD.

NIT: "vcpu_run; also manages memory conversions if requested by the TD."

> + */
> +void vcpu_run_and_manage_memory_conversions(struct kvm_vm *vm,
> +					    struct kvm_vcpu *vcpu)
> +{
> +	for (;;) {
> +		vcpu_run(vcpu);
> +		if (vcpu->run->exit_reason == KVM_EXIT_HYPERCALL &&
> +		    vcpu->run->hypercall.nr == KVM_HC_MAP_GPA_RANGE) {
> +			uint64_t gpa = vcpu->run->hypercall.args[0];
> +
> +			handle_memory_conversion(vm, vcpu->id, gpa,
> +						 vcpu->run->hypercall.args[1] << 12,
> +						 vcpu->run->hypercall.args[2] &
> +						  KVM_MAP_GPA_RANGE_ENCRYPTED);
> +			vcpu->run->hypercall.ret = 0;
> +			continue;
> +		} else if (vcpu->run->exit_reason == KVM_EXIT_IO &&
> +			   vcpu->run->io.port == TDX_UPM_TEST_ACCEPT_PRINT_PORT) {
> +			uint64_t gpa = tdx_test_read_64bit(vcpu,
> +							   TDX_UPM_TEST_ACCEPT_PRINT_PORT);
> +
> +			printf("\t ... guest accepting 1 page at GPA: 0x%lx\n",
> +			       gpa);
> +			continue;
> +		} else 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[12],
> +				  vcpu->run->system_event.data[13]);
> +		}
> +		break;
> +	}
> +}
> +

[snip]

> +
> +static void verify_upm_test(void)
> +{
> +	struct tdx_upm_test_area *test_area_base_hva;
> +	vm_vaddr_t test_area_gva_private;
> +	uint64_t test_area_npages;
> +	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_upm_explicit);
> +
> +	vm_install_exception_handler(vm, VE_VECTOR, guest_ve_handler);
> +
> +	/*
> +	 * 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_area_npages = TDX_UPM_TEST_AREA_SIZE / vm->page_size;
> +	vm_userspace_mem_region_add(vm,
> +				    VM_MEM_SRC_ANONYMOUS, TDX_UPM_TEST_AREA_GPA,
> +				    3, test_area_npages, KVM_MEM_GUEST_MEMFD);
> +	vm->memslots[MEM_REGION_TEST_DATA] = 3;

I find it odd that one has to 'know' that slot 3 is the next one and that
it is just a magic number here based off of what td_initialize() did.

Sean already mentioned not defining MEM_REGION_TDX_BOOT_PARAMS.  Perhaps
this could be made more dynamic when that change is implemented?

> +
> +	test_area_gva_private = vm_vaddr_alloc_private(vm, TDX_UPM_TEST_AREA_SIZE,
> +						       TDX_UPM_TEST_AREA_GVA_PRIVATE,
> +						       TDX_UPM_TEST_AREA_GPA,
> +						       MEM_REGION_TEST_DATA);
> +	TEST_ASSERT_EQ(test_area_gva_private, TDX_UPM_TEST_AREA_GVA_PRIVATE);
> +
> +	test_area_gpa_private = (struct tdx_upm_test_area *)
> +		addr_gva2gpa(vm, test_area_gva_private);
> +	virt_map_shared(vm, TDX_UPM_TEST_AREA_GVA_SHARED,
> +			(uint64_t)test_area_gpa_private,
> +			test_area_npages);
> +	TEST_ASSERT_EQ(addr_gva2gpa(vm, TDX_UPM_TEST_AREA_GVA_SHARED),
> +		       (vm_paddr_t)test_area_gpa_private);
> +
> +	test_area_base_hva = addr_gva2hva(vm, TDX_UPM_TEST_AREA_GVA_PRIVATE);
> +
> +	TEST_ASSERT(fill_and_check(test_area_base_hva, PATTERN_CONFIDENCE_CHECK),
> +		    "Failed to mark memory intended as backing memory for TD shared memory");
> +
> +	sync_global_to_guest(vm, test_area_gpa_private);
> +	test_area_gpa_shared = (struct tdx_upm_test_area *)
> +		((uint64_t)test_area_gpa_private | vm->arch.s_bit);
> +	sync_global_to_guest(vm, test_area_gpa_shared);
> +
> +	td_finalize(vm);
> +
> +	printf("Verifying UPM functionality: explicit MapGPA\n");

Not sure if Sean's comment regarding printf applies here.

Personally, I don't mind the noise in the output.  But I am running things
by hand.  I can see how having no output on success is a good thing when
running a suite of tests.

Ira

[snip]

Powered by blists - more mailing lists