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

[David Hildenbrand <david@redhat.com>]

>>
>> 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

But as Sean said, looking at the code might be easier.

Maybe just send the resulting diff of the patch here real quick?

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

How is that better than calling it "is_gmem" / "from_gmem" etc? :) 
Anyhow, no strong opinion, spelling out that something is from gmem 
implies that we don't care about page tables.

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

Makes sense.

-- 
Cheers,

David / dhildenb