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

[Ackerley Tng]

Xiaoyao Li <xiaoyao.li@xxxxxxxxx> writes:

> On 8/17/2026 9:52 PM, Ackerley Tng wrote:
>> Xiaoyao Li <xiaoyao.li@xxxxxxxxx> 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.
>
> __vm_phy_pages_alloc() takes a parameter @protected, which is used to
> tell the allocated physical pages need to be protected(private) or not.
> It looks weird that a page is allocated as protected but actually it is
> still shared.
>

I think there are a few different things here:

1. Finding some range of guest physical addresses for a page. This is
   __vm_phy_pages_alloc().

2. Tracking the guest physical addresses as shared or private in the
   userspace VMM. This is setting region->protected_phy_pages.

3. Creating private vs shared mappings in guest page tables. This is
   done in __virt_pg_map().

4. Transferring region->protected_phy_pages into the kernel. This is
   either the VM or guest_memfd conversion ioctl.

5. Loading private memory based on region->protected_phy_pages. This is
   done in tdx_load_private_memory() and the SNP counterpart.

So protected_phy_pages tracks all the pages that will be initialized as
private, not the pages that are private (since vm_mem_set_private()
doesn't update protected_phy_pages).

(2) is coupled to whether the VM has protected memory in
vm_phy_pages_alloc(). This is kind of an awkward assumption since a CoCo
VM can have shared memory too, and especially for selftests there can be
shared pages right from the start.

However, setting protected_phy_pages in (2) and referencing in it (3),
(4) and (5) makes sense since that's the common path, and we want the
common path to be easy for the selftest writer.

I think there could be some refactoring so that the different components
1 through 5 are opened up for easier usage but I haven't thought about
how it should look like and anyway I think that belongs in a separate
series.


Imo the direct response to looking allocated as protected but is actually
shared is that the page was allocated as "will be loaded as private",
and doesn't make any statement on whether it is shared or private.

Does seeing protected_phy_pages as "will be loaded as private" help in
explaining why vm_mem_set_private() is not called in
__vm_phy_pages_alloc() but is done only in tdx_load_private_memory()?

>>> 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 based on the assumption of "in-place conversion". However the
> TDX selftests should be able to run without "in-place conversion". There
> is no hard dependency on it.
>
> I think this patch implements what patch 13[1] of this series says "For
> CoCo VMs, pages that need to be private are explicitly set to private
> before executing the VM."
>
> But all of this is for the case of "in-place conversion". When "in-place
> conversion" is not supported/enabled by the kernel,
> GUEST_MEMFD_FLAG_INIT_SHARED for Coco VMs is *not* allowed and all the
> memory are private by default and no need to call vm_mem_set_private()
> here at all.
>
> [1]
> https://lore.kernel.org/all/20260722-tdx-selftests-v14-13-15ad654a50db@xxxxxxxxxx/
>

Doing (3) as part of (5) isn't just for in-place conversion. Without
in-place conversion, source_pages should be the address mmap()-ed from
some non-guest_memfd memory, which by the time of
tdx_load_private_memory(), would already be populated with whatever
needs to be loaded. Populate needs to be called on private memory
regardless of whether we use in-place conversions.

>>> 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);
>>>> +}
>>>>