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

[David Hildenbrand <david@redhat.com>]

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


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

-- 
Cheers,

David / dhildenb