Re: [PATCH v7 4/9] KVM: guest_memfd: Handle in-place shared memory as guest_memfd backed memory

[David Hildenbrand <david@redhat.com>]

>>
> 
> I looked a little deeper and got help from James Houghton on
> understanding this too.

Great :)

> 
> Specifically for the usage of kvm_mem_is_private() in
> kvm_mmu_max_mapping_level(), the intention there is probably to skip
> querying userspace page tables in __kvm_mmu_max_mapping_level() since
> private memory will never be faulted into userspace, hence no need to
> check.
> 
> Hence kvm_mem_is_private() there is really meant to query the
> private-ness of the gfn rather than just whether kvm_mem_from_gmem().
> 
> But then again, if kvm_mem_from_gmem(), guest_memfd should be queried
> for max_mapping_level. guest_memfd would know, for both private and
> shared memory, what page size the page was split to, and what level
> it was faulted as. (Exception: if/when guest_memfd supports THP,
> depending on how that is done, querying userspace page tables might be
> necessary to determine the max_mapping_level)

Okay, so I assume my intuition was right: if we know we can go via the 
guest_memfd also for !private memory, then probably no need to consult 
the page tables.

Let's discuss that tomorrow in the meeting.

> 
>>>
>>> A. this specific gfn is backed by gmem, or
>>> B. if the specific gfn is private?
>>>
>>> I noticed some other places where kvm_mem_is_private() is left as-is
>>> [2], is that intentional? Are you not just renaming but splitting out
>>> the case two cases A and B?
>>
>> That was the idea, yes.
>>
>> If we get a private fault and !kvm_mem_is_private(), or a shared fault and
>> kvm_mem_is_private(), then we should handle it like today.
>>
>> That is the kvm_mmu_faultin_pfn() case, where we
>>
>> if (fault->is_private != kvm_mem_is_private(kvm, fault->gfn)) {
>> 	kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
>> 	return -EFAULT;
>> }
>>
>> which can be reached by arch/x86/kvm/svm/svm.c:npf_interception()
>>
>> if (sev_snp_guest(vcpu->kvm) && (error_code & PFERR_GUEST_ENC_MASK))
>> 	error_code |= PFERR_PRIVATE_ACCESS;
>>
>> In summary: the memory attribute mismatch will be handled as is, but not how
>> we obtain the gfn.
>>
>> At least that was the idea (-issues in the commit).
>>
>> What are your thoughts about that direction?
> 
> I still like the renaming. :)
> 
> I looked into kvm_mem_is_private() and I believe it has the following
> uses:
> 
> 1. Determining max_mapping_level (kvm_mmu_max_mapping_level() and
>     friends)
> 2. Querying the kernel's record of private/shared state, which is used
>     to handle (a) mismatch between fault->private and kernel's record
>     (handling implicit conversions) (b) how to prefaulting pages (c)
>     determining how to fault in KVM_X86_SW_PROTECTED_VMs
> 
> So perhaps we could leave kvm_mem_is_private() as not renamed, but as
> part of the series introducing mmap and conversions
> (CONFIG_KVM_GMEM_SHARED_MEM), we should also have kvm_mem_is_private()
> query guest_memfd for shareability status, and perhaps
> kvm_mmu_max_mapping_level() could query guest_memfd for page size (after
> splitting, etc).
> 

Right, that's why I opted to leave kvm_mem_is_private() here and really 
only indicate if memory is *actually* private.

> IIUC the maximum mapping level is determined by these factors:
> 
> 1. Attribute granularity (lpage_info)
> 2. Page size (guest_memfd for guest_memfd backed memory)
> 3. Size of mapping in host page table (for non-guest_memfd backed
>     memory, and important for THP if/when/depending on how guest_memfd
>     supports THP)

Right, once private+shared will come from guest_memfd, then likely 3
does not apply anymore.

See my reply to Patrick regarding that.

Thanks!

-- 
Cheers,

David / dhildenb