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

[David Hildenbrand <david@redhat.com>]

On 30.04.25 20:58, Ackerley Tng wrote:
> Fuad Tabba <tabba@google.com> writes:
> 
>> Until now, faults to private memory backed by guest_memfd are always
>> consumed from guest_memfd whereas faults to shared memory are consumed
>> from anonymous memory. Subsequent patches will allow sharing guest_memfd
>> backed memory in-place, and mapping it by the host. Faults to in-place
>> shared memory should be consumed from guest_memfd as well.
>>
>> In order to facilitate that, generalize the fault lookups. Currently,
>> only private memory is consumed from guest_memfd and therefore as it
>> stands, this patch does not change the behavior.
>>
>> Co-developed-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Fuad Tabba <tabba@google.com>
>> ---
>>   arch/x86/kvm/mmu/mmu.c   | 19 +++++++++----------
>>   include/linux/kvm_host.h |  6 ++++++
>>   2 files changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>> index 6d5dd869c890..08eebd24a0e1 100644
>> --- a/arch/x86/kvm/mmu/mmu.c
>> +++ b/arch/x86/kvm/mmu/mmu.c
>> @@ -3258,7 +3258,7 @@ static int host_pfn_mapping_level(struct kvm *kvm, gfn_t gfn,
>>
>>   static int __kvm_mmu_max_mapping_level(struct kvm *kvm,
>>   				       const struct kvm_memory_slot *slot,
>> -				       gfn_t gfn, int max_level, bool is_private)
>> +				       gfn_t gfn, int max_level, bool is_gmem)
>>   {
>>   	struct kvm_lpage_info *linfo;
>>   	int host_level;
>> @@ -3270,7 +3270,7 @@ static int __kvm_mmu_max_mapping_level(struct kvm *kvm,
>>   			break;
>>   	}
>>
>> -	if (is_private)
>> +	if (is_gmem)
>>   		return max_level;
> 
 > I think this renaming isn't quite accurate.

After our discussion yesterday, does that still hold true?

> 
> IIUC in __kvm_mmu_max_mapping_level(), we skip considering
> host_pfn_mapping_level() if the gfn is private because private memory
> will not be mapped to userspace, so there's no need to query userspace
> page tables in host_pfn_mapping_level().

I think the reason was that: for private we won't be walking the user 
space pages tables.

Once guest_memfd is also responsible for the shared part, why should 
this here still be private-only, and why should we consider querying a 
user space mapping that might not even exist?

> 
> Renaming is_private to is_gmem in this function implies that as long as
> gmem is used, especially for shared pages from gmem, lpage_info will
> always be updated and there's no need to query userspace page tables.

I'm missing the point why shared memory from gmem should be treated 
differently here. Can you maybe clarify which issue you see?

> 
>>
>>   	if (max_level == PG_LEVEL_4K)
>> @@ -3283,10 +3283,9 @@ static int __kvm_mmu_max_mapping_level(struct kvm *kvm,
>>   int kvm_mmu_max_mapping_level(struct kvm *kvm,
>>   			      const struct kvm_memory_slot *slot, gfn_t gfn)
>>   {
>> -	bool is_private = kvm_slot_has_gmem(slot) &&
>> -			  kvm_mem_is_private(kvm, gfn);
>> +	bool is_gmem = kvm_slot_has_gmem(slot) && kvm_mem_from_gmem(kvm, gfn);
> 
> This renaming should probably be undone too.
> 
>>
>> -	return __kvm_mmu_max_mapping_level(kvm, slot, gfn, PG_LEVEL_NUM, is_private);
>> +	return __kvm_mmu_max_mapping_level(kvm, slot, gfn, PG_LEVEL_NUM, is_gmem);
>>   }


-- 
Cheers,

David / dhildenb