Re: [PATCH v5 1/2] KVM: guest_memfd: add generic population via write

[Nikita Kalyazin <kalyazin@amazon.com>]

On 13/09/2025 01:32, Vishal Annapurve wrote:
> On Fri, Sep 12, 2025 at 3:35 PM James Houghton <jthoughton@google.com> wrote:
>>
>>>>> +
>>>>> +       if (folio_test_uptodate(folio)) {
>>>>> +               folio_unlock(folio);
>>>>> +               folio_put(folio);
>>>>> +               return -ENOSPC;
>>>>
>>>> Does it actually matter for the folio not to be uptodate? It seems
>>>> unnecessarily restrictive not to be able to overwrite data if we're
>>>> saying that this is only usable for unencrypted memory anyway.
>>>
>>> In the context of direct map removal [1] it does actually because when
>>> we mark a folio as prepared, we remove it from the direct map making it
>>> inaccessible to the way write() performs the copy.  It does not matter
>>> if direct map removal isn't enabled though.  Do you think it should be
>>> conditional?
>>
>> Oh, good point. It's simpler (both to implement and to describe) to
>> disallow a second write() call in all cases (no matter if the direct
>> map for the page has been removed or if the contents have been
>> encrypted), so I'm all for leaving it unconditional like you have now.
>> Thanks!
> 
> Are we deviating from the way read/write semantics work for the other
> filesystems? I don't think other filesystems carry this restriction of
> one-time-write only. Do we strictly need the differing semantics?

Yes, I believe we are deviating from other "regular" filesystems, but I 
don't think what we propose is too uncommon as in "special" filesystems 
such as sysfs subsequent calls to attributes like "remove" will likely 
fail as well (not due to up-to-date flag though).

> Maybe it would be simpler to not overload uptodate flag and just not
> allow read/write if folio is not mapped in the direct map for non-conf
> VMs (assuming there could be other ways to deduce that information).

The only such interface I'm aware of is kernel_page_present() so the 
check may look like:

	for (i = 0; i < folio_nr_pages(folio); i++) {
		struct page *page = folio_page(folio, i);
		if (!kernel_page_present(page)) {
			folio_unlock(folio);
			folio_put(folio);
			return -ENOSPC;
		}
	}

However, kernel_page_present() is not currently exported to modules.

Alternatively, the same effect can be achieved via checking for both 
kvm_gmem_test_no_direct_map(inode) [1] and folio_test_uptodate(folio). 
It would be the "conditional" check I mentioned earlier in the thread.

[1]: https://lore.kernel.org/kvm/20250912091708.17502-6-roypat@amazon.co.uk/

> Can there be users who want to populate the file ranges multiple times
> as it seems more performant?

Yes, you are right, there may be use cases like that.  At the same time, 
I think they are much less common because it's more typical for the 
initial population to cover larger memory ranges and be sensitive to 
performance.

> 
>>
>>>
>>> [1]: https://lore.kernel.org/kvm/20250828093902.2719-1-roypat@amazon.co.uk
>>>
>>>>