Re: [PATCH v14 18/22] KVM: selftests: Add helpers to init TDX memory and finalize VM

Xiaoyao Li <[email protected]> writes:

>
> [...snip...]
>
>> +static void tdx_load_private_memory(struct kvm_vm *vm)
>> +{
>> +	struct userspace_mem_region *region;
>> +	int ctr;
>> +
>> +	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node) {
>> +		const struct sparsebit *protected_pages = region->protected_phy_pages;
>> +		const gpa_t gpa_base = region->region.guest_phys_addr;
>> +		const u64 hva_base = region->region.userspace_addr;
>> +		const sparsebit_idx_t lowest_page_in_region = gpa_base >> vm->page_shift;
>> +		void *source_pages = NULL;
>> +		sparsebit_idx_t i, j;
>> +
>> +		if (!sparsebit_any_set(protected_pages))
>
> sparebit_any_set() doens't check if the input is NULL. So we need to
> check it here.
>
>> +			continue;
>> +
>> +		TEST_ASSERT(region->region.guest_memfd != -1,
>> +			    "TD private memory must be backed by guest_memfd");
>> +
>> +		sparsebit_for_each_set_range(protected_pages, i, j) {
>> +			const u64 size_to_load = (j - i + 1) * vm->page_size;
>> +			const u64 offset =
>> +				(i - lowest_page_in_region) * vm->page_size;
>> +			const u64 hva = hva_base + offset;
>> +			const u64 gpa = gpa_base + offset;
>> +
>> +			if (!kvm_has_gmem_attributes)
>> +				source_pages = (void *)hva;
>> +
>
>> +			vm_mem_set_private(vm, gpa, size_to_load);
>
> So vm_mem_set_private() has to be called at this late stage when run
> with in-place gmem. But for non in-place gmem, we can actually call
> vm_mem_set_private() in __vm_phy_pages_alloc().
>
> Calling vm_mem_set_private() here instead of in __vm_phy_pages_alloc()
> looks like a trick to me.

I thought this is fine because __vm_phy_pages_alloc() seems to be a
rather low-level function, where the responsibility of the function is
just to allocate (for find some physical pages). Calling
vm_mem_set_private() in there seems to be doing too much.

> That is, we cannot set the page as private
> when allocating a guest physical page as protected because if doing so,
> we cannot write the initial content to it.
>

I think calling it here isn't a trick, it's a good way to reuse all the
existing code that builds up the guest image in place. In-place
conversion allows you to set stuff up in shared memory and then convert
everything when you're done and also populate the memory.

> This is the topic about how to implement the infras for in-place gmem,
> not the issue of this series. Let me go read the selftest patches of
> gmem in-place series and we can discuss there.
>
>> +			tdx_init_mem_region(vm, source_pages, gpa, size_to_load);
>> +		}
>> +	}
>> +}
>> +
>> +void tdx_vm_finalize(struct kvm_vm *vm)
>> +{
>> +	tdx_load_private_memory(vm);
>> +	tdx_vm_ioctl(vm, KVM_TDX_FINALIZE_VM, 0, NULL);
>> +}
>>