Re: [PATCH v8 13/46] KVM: guest_memfd: Add base support for KVM_SET_MEMORY_ATTRIBUTES2

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 06/07/2026 19:17, Ackerley Tng wrote:
> Suzuki K Poulose <suzuki.poulose@arm.com> writes:
> 
>>
>> [...snip...]
>>
>>> +static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
>>> +				     size_t nr_pages, uint64_t attrs)
>>> +{
>>> +	struct address_space *mapping = inode->i_mapping;
>>> +	struct gmem_inode *gi = GMEM_I(inode);
>>> +	pgoff_t end = start + nr_pages;
>>> +	struct maple_tree *mt;
>>> +	struct ma_state mas;
>>> +	int r;
>>> +
>>> +	mt = &gi->attributes;
>>> +
>>> +	filemap_invalidate_lock(mapping);
>>> +
>>> +	mas_init(&mas, mt, start);
>>> +	r = kvm_gmem_mas_preallocate(&mas, attrs, start, nr_pages);
>>> +	if (r)
>>> +		goto out;
>>> +
>>> +	/*
>>> +	 * From this point on guest_memfd has performed necessary
>>> +	 * checks and can proceed to do guest-breaking changes.
>>> +	 */
>>> +
>>> +	kvm_gmem_invalidate_start(inode, start, end);
>>
>> I added support for Arm CCA KVM patches with the inplace conversion and
>> I am hitting the following issue.
>>
>> 1. I am supporting INIT_SHARED + MMAP flags.
>> 2. VMM creates the Gmem_fd with both the flags above.
>> 3. Uses the shared gmem-mmap to load the initial payloads (kernel, dtb).
>> 4. At the VM finalization time, Populate the loaded regions one by one
>>      by
>>       a) copying the images to a temparory buffer - Since CCA can't really
>>          load the contents in-place.
> 
> Sounds good :). I see that you blocked this in the kernel by returning
> -EOPNOTSUPP if (!src_page) [0].

We could do the copy in kernel with src_page == dst_page, but that would
affect the batching of Granule delegation (and at which point we might
need a temparory buffer in the kernel as big as the vma_pagesize)

> 
>>       b) Set the "region" to Private in the gmem_fd (via
>> SET_MEMORY_ATTRIBUTES2)
>>       c) Invoke CCA backend to populate the private memory via
>>          ioctl(KVM_ARM_RMI_POPULATE,..) [0]
>>
> 
> This flow sounds right.
> 
>> [0]
>> https://lore.kernel.org/all/20260513131757.116630-27-steven.price@arm.com/
>>
>>
>> 5. Additionally, VMM can mark the entire RAM to be private before the VM
>>      starts running, again via SET_MEMORY_ATTRIBUTES2. On CCA, this
>> action is measured and doesn't require the Host to "commit" memory to
>> the VM.
>> Instead the host can lazily donate memory on a fault.
>>
> 
> For both TDX and SNP, the host can also lazily donate memory,
> guest_memfd supports this.
> 
>> But step (5) triggers the invalidation of both private and shared
>> mappings of the gmem area, from the kvm_gmem_invalidate_start()
>> above.
>>
>> This is because, the entire DRAM now has, some portions PRIVATE (the
>> loaded regions) and the rest are SHARED (from the Gmem_fd creation).
>>    Thus, kvm_gmem_get_invalidate_filter(Dram_start, Dram_end) causes the
>> invalidation of both "PRIVATE" and "SHARED" regions, which results
>> in the destruction of the already loaded data and things go south.
>>
> 
> This destruction will happen for TDX as well. I think we managed to get
> around this because we didn't apply conversion on the already-private
> ranges.
> 
> IIUC on SNP, zapping pages in the stage 2 page tables doesn't destroy
> the data, so that's probably why it has been fine for SNP.

Additionally, the Guest at boot, will try to mark the entire DRAM
as Private (RIPAS_RAM in CCA), which would trigger this anyways.

Suzuki


> 
>> When we know that the kvm_gmem_invalidate_xx is triggered by a
>> conversion, we don't need to invalidate the existing pages that
>> are in the requested state. i.e., the following patch on top of
>> this series does the trick for me :
>>
>>
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index a97fcac34a0e..62e0427a49f4 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -250,16 +250,23 @@ static void __kvm_gmem_invalidate_start(struct
>> gmem_file *f, pgoff_t start,
>>                   KVM_MMU_UNLOCK(kvm);
>>    }
>>
>> +static void kvm_gmem_invalidate_start_filter(struct inode *inode,
>> pgoff_t start,
>> +                                            pgoff_t end,
>> +                                            enum kvm_gfn_range_filter
>> attr_filter)
>> +{
>> +       struct gmem_file *f;
>> +
>> +       kvm_gmem_for_each_file(f, inode)
>> +               __kvm_gmem_invalidate_start(f, start, end, attr_filter);
>> +}
>> +
>>    static void kvm_gmem_invalidate_start(struct inode *inode, pgoff_t start,
>>                                         pgoff_t end)
>>    {
>>           enum kvm_gfn_range_filter attr_filter;
>> -       struct gmem_file *f;
>> -
>>           attr_filter = kvm_gmem_get_invalidate_filter(inode, start, end);
>>
>> -       kvm_gmem_for_each_file(f, inode)
>> -               __kvm_gmem_invalidate_start(f, start, end, attr_filter);
>> +       kvm_gmem_invalidate_start_filter(inode, start, end, attr_filter);
>>    }
>>
>>    static void __kvm_gmem_invalidate_end(struct gmem_file *f, pgoff_t start,
>> @@ -724,9 +731,14 @@ static int __kvm_gmem_set_attributes(struct inode
>> *inode, pgoff_t start,
>>           /*
>>            * From this point on guest_memfd has performed necessary
>>            * checks and can proceed to do guest-breaking changes.
>> +        * Also, we don't have to invalidate the regions that
>> +        * may already be in the requested state. Hence, we could
>> +        * explicitly filter the invalidations to the opposite
>> +        * state.
>>            */
>>
>> -       kvm_gmem_invalidate_start(inode, start, end);
>> +       kvm_gmem_invalidate_start_filter(inode, start, end,
>> +                                       to_private ? KVM_FILTER_SHARED :
>> KVM_FILTER_PRIVATE);
>>
> 
> I think this makes sense. Thanks for catching this.
> 
>>           if (!to_private)
>>                   kvm_gmem_invalidate(inode, start, end);
>>
>>
>> Thoughts ?
>>
>> Suzuki
>>
>>
>>>
>>> [...snip...]
>>>