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
