Re: [PATCH v8 28/30] KVM: selftests: TDX: Add TDX UPM selftest

Sagi Shahar wrote:

From: Ackerley Tng ackerleytng@google.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]