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

[Ackerley Tng <ackerleytng@google.com>]

David Hildenbrand <david@redhat.com> writes:

>>> I've been thinking long about this, and was wondering if we should instead
>>> clean up the code to decouple the "private" from gmem handling first.
>>>
>> 
>> Thank you for making this suggestion more concrete, I like the renaming!
>> 
>
> Thanks for the fast feedback!
>
>>> I know, this was already discussed a couple of times, but faking that
>>> shared memory is private looks odd.
>>>
>>> I played with the code to star cleaning this up. I ended up with the following
>>> gmem-terminology  cleanup patches (not even compile tested)
>>>
>>> KVM: rename CONFIG_KVM_GENERIC_PRIVATE_MEM to CONFIG_KVM_GENERIC_GMEM_POPULATE
>>> KVM: rename CONFIG_KVM_PRIVATE_MEM to CONFIG_KVM_GMEM
>>> KVM: rename kvm_arch_has_private_mem() to kvm_arch_supports_gmem()
>>> KVM: x86: rename kvm->arch.has_private_mem to kvm->arch.supports_gmem
>>> KVM: rename kvm_slot_can_be_private() to kvm_slot_has_gmem()
>> 
>> Perhaps zooming into this [1] can clarify a lot. In
>> kvm_mmu_max_mapping_level(), it was
>> 
>>    bool is_private = kvm_slot_has_gmem(slot) && kvm_mem_is_private(kvm, gfn);
>> 
>> and now it is
>> 
>>    bool is_gmem = kvm_slot_has_gmem(slot) && kvm_mem_from_gmem(kvm, gfn);
>> 
>> Is this actually saying that the mapping level is to be fully determined
>> from lpage_info as long as this memslot has gmem and
>
> With this change in particular I was not quite sure what to do, maybe it should
> stay specific to private memory only? But yeah the ideas was that
> kvm_mem_from_gmem() would express:
>
> (a) if guest_memfd only supports private memory, it would translate to
> kvm_mem_is_private() -> no change.
>
> (b) with guest_memfd having support for shared memory (+ support being enabled!),
> it would only rely on the slot, not gfn information. Because it will all be
> consumed from guest_memfd.
>
> This hunk was missing
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index d9616ee6acc70..cdcd7ac091b5c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -2514,6 +2514,12 @@ static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>   }
>   #endif /* CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES */
>   
> +static inline bool kvm_mem_from_gmem(struct kvm *kvm, gfn_t gfn)
> +{
> +       /* For now, only private memory gets consumed from guest_memfd. */
> +       return kvm_mem_is_private(kvm, gfn);
> +}
> +
>
>

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

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)

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

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)

>
> -- 
> Cheers,
>
> David / dhildenb