Re: [PATCH v8 06/13] KVM: x86: Generalize private fault lookups to guest_memfd fault lookups

[Ackerley Tng <ackerleytng@google.com>]

David Hildenbrand <david@redhat.com> writes:

> On 06.05.25 15:58, Sean Christopherson wrote:
>> On Mon, May 05, 2025, Vishal Annapurve wrote:
>>> On Mon, May 5, 2025 at 10:17 PM Vishal Annapurve <vannapurve@google.com> wrote:
>>>>
>>>> On Mon, May 5, 2025 at 3:57 PM Sean Christopherson <seanjc@google.com> wrote:
>>>>>> ...
>>>>>> And not worry about lpage_infor for the time being, until we actually do
>>>>>> support larger pages.
>>>>>
>>>>> I don't want to completely punt on this, because if it gets messy, then I want
>>>>> to know now and have a solution in hand, not find out N months from now.
>>>>>
>>>>> That said, I don't expect it to be difficult.  What we could punt on is
>>>>> performance of the lookups, which is the real reason KVM maintains the rather
>>>>> expensive disallow_lpage array.
>>>>>
>>>>> And that said, memslots can only bind to one guest_memfd instance, so I don't
>>>>> immediately see any reason why the guest_memfd ioctl() couldn't process the
>>>>> slots that are bound to it.  I.e. why not update KVM_LPAGE_MIXED_FLAG from the
>>>>> guest_memfd ioctl() instead of from KVM_SET_MEMORY_ATTRIBUTES?
>>>>
>>>> I am missing the point here to update KVM_LPAGE_MIXED_FLAG for the
>>>> scenarios where in-place memory conversion will be supported with
>>>> guest_memfd. As guest_memfd support for hugepages comes with the
>>>> design that hugepages can't have mixed attributes. i.e. max_order
>>>> returned by get_pfn will always have the same attributes for the folio
>>>> range.
>> 
>> Oh, if this will naturally be handled by guest_memfd, then do that.  I was purely
>> reacting to David's suggestion to "not worry about lpage_infor for the time being,
>> until we actually do support larger pages".
>> 
>>>> Is your suggestion around using guest_memfd ioctl() to also toggle
>>>> memory attributes for the scenarios where guest_memfd instance doesn't
>>>> have in-place memory conversion feature enabled?
>>>
>>> Reading more into your response, I guess your suggestion is about
>>> covering different usecases present today and new usecases which may
>>> land in future, that rely on kvm_lpage_info for faster lookup. If so,
>>> then it should be easy to modify guest_memfd ioctl to update
>>> kvm_lpage_info as you suggested.
>> 
>> Nah, I just missed/forgot that using a single guest_memfd for private and shared
>> would naturally need to split the folio and thus this would Just Work.

Sean, David, I'm circling back to make sure I'm following the discussion
correctly before Fuad sends out the next revision of this series.

>
> Yeah, I ignored that fact as well. So essentially, this patch should be 
> mostly good for now.
>

From here [1], these changes will make it to v9

+ kvm_max_private_mapping_level renaming to kvm_max_gmem_mapping_level
+ kvm_mmu_faultin_pfn_private renaming to kvm_mmu_faultin_pfn_gmem

> Only kvm_mmu_hugepage_adjust() must be taught to not rely on 
> fault->is_private.
>

I think fault->is_private should contribute to determining the max
mapping level.

By the time kvm_mmu_hugepage_adjust() is called,

* For Coco VMs using guest_memfd only for private memory,
  * fault->is_private would have been checked to align with
    kvm->mem_attr_array, so 
* For Coco VMs using guest_memfd for both private/shared memory,
  * fault->is_private would have been checked to align with
    guest_memfd's shareability
* For non-Coco VMs using guest_memfd
  * fault->is_private would be false

Hence fault->is_private can be relied on when calling
kvm_mmu_hugepage_adjust().

If fault->is_private, there will be no host userspace mapping to check,
hence in __kvm_mmu_max_mapping_level(), we should skip querying host
page tables.

If !fault->is_private, for shared memory ranges, if the VM uses
guest_memfd only for shared memory, we should query host page tables.

If !fault->is_private, for shared memory ranges, if the VM uses
guest_memfd for both shared/private memory, we should not query host
page tables.

If !fault->is_private, for non-Coco VMs, we should not query host page
tables.

I propose to rename the parameter is_private to skip_host_page_tables,
so

- if (is_private)
+ if (skip_host_page_tables)
	return max_level;

and pass

skip_host_page_tables = fault->is_private ||
			kvm_gmem_memslot_supports_shared(fault->slot);

where kvm_gmem_memslot_supports_shared() checks the inode in the memslot
for GUEST_MEMFD_FLAG_SUPPORT_SHARED.

For recover_huge_pages_range(), the other user of
__kvm_mmu_max_mapping_level(), currently there's no prior call to
kvm_gmem_get_pfn() to get max_order or max_level, so I propose to call
__kvm_mmu_max_mapping_level() with

if (kvm_gmem_memslot_supports_shared(slot)) {
	max_level = kvm_gmem_max_mapping_level(slot, gfn);
	skip_host_page_tables = true;
} else {
	max_level = PG_LEVEL_NUM;
        skip_host_page_tables = kvm_slot_has_gmem(slot) &&
				kvm_mem_is_private(kvm, gfn);
}

Without 1G support, kvm_gmem_max_mapping_level(slot, gfn) would always
return 4K.

With 1G support, kvm_gmem_max_mapping_level(slot, gfn) would return the
level for the page's order, at the offset corresponding to the gfn.

> Once we support large folios in guest_memfd, only the "alignment" 
> consideration might have to be taken into account.
>

I'll be handling this alignment as part of the 1G page support series
(won't be part of Fuad's first stage series) [2]

> Anything else?
>
> -- 
> Cheers,
>
> David / dhildenb


[1] https://lore.kernel.org/all/20250430165655.605595-7-tabba@google.com/
[2] https://lore.kernel.org/all/diqz1pt1sfw8.fsf@ackerleytng-ctop.c.googlers.com/