Re: [PATCH v12 10/18] KVM: x86/mmu: Handle guest page faults for guest_memfd with shared memory

[Ackerley Tng <ackerleytng@google.com>]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jun 11, 2025, Fuad Tabba wrote:
>> From: Ackerley Tng <ackerleytng@google.com>
>> 
>> For memslots backed by guest_memfd with shared mem support, the KVM MMU
>> must always fault in pages from guest_memfd, and not from the host
>> userspace_addr. Update the fault handler to do so.
>
> And with a KVM_MEMSLOT_GUEST_MEMFD_ONLY flag, this becomes super obvious.
>
>> This patch also refactors related function names for accuracy:
>
> This patch.  And phrase changelogs as commands.
>
>> kvm_mem_is_private() returns true only when the current private/shared
>> state (in the CoCo sense) of the memory is private, and returns false if
>> the current state is shared explicitly or impicitly, e.g., belongs to a
>> non-CoCo VM.
>
> Again, state changes as commands.  For the above, it's not obvious if you're
> talking about the existing code versus the state of things after "this patch".
>
>

Will fix these, thanks!

>> kvm_mmu_faultin_pfn_gmem() is updated to indicate that it can be used to
>> fault in not just private memory, but more generally, from guest_memfd.
>
>> +static inline u8 kvm_max_level_for_order(int order)
>
> Do not use "inline" for functions that are visible only to the local compilation
> unit.  "inline" is just a hint, and modern compilers are smart enough to inline
> functions when appropriate without a hint.
>
> A longer explanation/rant here: https://lore.kernel.org/all/ZAdfX+S323JVWNZC@google.com
>

Will fix this!

>> +static inline int kvm_gmem_max_mapping_level(const struct kvm_memory_slot *slot,
>> +					     gfn_t gfn, int max_level)
>> +{
>> +	int max_order;
>>  
>>  	if (max_level == PG_LEVEL_4K)
>>  		return PG_LEVEL_4K;
>
> This is dead code, the one and only caller has *just* checked for this condition.
>>  
>> -	host_level = host_pfn_mapping_level(kvm, gfn, slot);
>> -	return min(host_level, max_level);
>> +	max_order = kvm_gmem_mapping_order(slot, gfn);
>> +	return min(max_level, kvm_max_level_for_order(max_order));
>>  }
>
> ...
>
>> -static u8 kvm_max_private_mapping_level(struct kvm *kvm, kvm_pfn_t pfn,
>> -					u8 max_level, int gmem_order)
>> +static u8 kvm_max_level_for_fault_and_order(struct kvm *kvm,
>
> This is comically verbose.  C ain't Java.  And having two separate helpers makes
> it *really* hard to (a) even see there are TWO helpers in the first place, and
> (b) understand how they differ.
>
> Gah, and not your bug, but completely ignoring the RMP in kvm_mmu_max_mapping_level()
> is wrong.  It "works" because guest_memfd doesn't (yet) support dirty logging,
> no one enables the NX hugepage mitigation on AMD hosts.
>
> We could plumb in the pfn and private info, but I don't really see the point,
> at least not at this time.
>
>> +					    struct kvm_page_fault *fault,
>> +					    int order)
>>  {
>> -	u8 req_max_level;
>> +	u8 max_level = fault->max_level;
>>  
>>  	if (max_level == PG_LEVEL_4K)
>>  		return PG_LEVEL_4K;
>>  
>> -	max_level = min(kvm_max_level_for_order(gmem_order), max_level);
>> +	max_level = min(kvm_max_level_for_order(order), max_level);
>>  	if (max_level == PG_LEVEL_4K)
>>  		return PG_LEVEL_4K;
>>  
>> -	req_max_level = kvm_x86_call(private_max_mapping_level)(kvm, pfn);
>> -	if (req_max_level)
>> -		max_level = min(max_level, req_max_level);
>> +	if (fault->is_private) {
>> +		u8 level = kvm_x86_call(private_max_mapping_level)(kvm, fault->pfn);
>
> Hmm, so the interesting thing here is that (IIRC) the RMP restrictions aren't
> just on the private pages, they also apply to the HYPERVISOR/SHARED pages.  (Don't
> quote me on that).
>
> Regardless, I'm leaning toward dropping the "private" part, and making SNP deal
> with the intricacies of the RMP:
>
> 	/* Some VM types have additional restrictions, e.g. SNP's RMP. */
> 	req_max_level = kvm_x86_call(max_mapping_level)(kvm, fault);
> 	if (req_max_level)
> 		max_level = min(max_level, req_max_level);
>
> Then we can get to something like:
>
> static int kvm_gmem_max_mapping_level(struct kvm *kvm, int order,
> 				      struct kvm_page_fault *fault)
> {
> 	int max_level, req_max_level;
>
> 	max_level = kvm_max_level_for_order(order);
> 	if (max_level == PG_LEVEL_4K)
> 		return PG_LEVEL_4K;
>
> 	req_max_level = kvm_x86_call(max_mapping_level)(kvm, fault);
> 	if (req_max_level)
> 		max_level = min(max_level, req_max_level);
>
> 	return max_level;
> }
>
> int kvm_mmu_max_mapping_level(struct kvm *kvm,
> 			      const struct kvm_memory_slot *slot, gfn_t gfn)
> {
> 	int max_level;
>
> 	max_level = kvm_lpage_info_max_mapping_level(kvm, slot, gfn, PG_LEVEL_NUM);
> 	if (max_level == PG_LEVEL_4K)
> 		return PG_LEVEL_4K;
>
> 	/* TODO: Comment goes here about KVM not supporting this path (yet). */

Which path does KVM not support?

> 	if (kvm_mem_is_private(kvm, gfn))
> 		return PG_LEVEL_4K;
>

Just making sure - this suggestion does take into account that
kvm_mem_is_private() will be querying guest_memfd for memory privacy
status, right? So the check below for kvm_is_memslot_gmem_only() will
only be handling the cases where the memory is shared, and only
guest_memfd is used for this gfn?

> 	if (kvm_is_memslot_gmem_only(slot)) {
> 		int order = kvm_gmem_mapping_order(slot, gfn);
>
> 		return min(max_level, kvm_gmem_max_mapping_level(kvm, order, NULL));
> 	}
>
> 	return min(max_level, host_pfn_mapping_level(kvm, gfn, slot));
> }
>
> static int kvm_mmu_faultin_pfn_gmem(struct kvm_vcpu *vcpu,
> 				    struct kvm_page_fault *fault)
> {
> 	struct kvm *kvm = vcpu->kvm;
> 	int order, r;
>
> 	if (!kvm_slot_has_gmem(fault->slot)) {
> 		kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
> 		return -EFAULT;
> 	}
>
> 	r = kvm_gmem_get_pfn(kvm, fault->slot, fault->gfn, &fault->pfn,
> 			     &fault->refcounted_page, &order);
> 	if (r) {
> 		kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
> 		return r;
> 	}
>
> 	fault->map_writable = !(fault->slot->flags & KVM_MEM_READONLY);
> 	fault->max_level = kvm_gmem_max_mapping_level(kvm, order, fault);
>
> 	return RET_PF_CONTINUE;
> }
>
> int sev_max_mapping_level(struct kvm *kvm, struct kvm_page_fault *fault)
> {
> 	int level, rc;
> 	bool assigned;
>
> 	if (!sev_snp_guest(kvm))
> 		return 0;
>
> 	if (WARN_ON_ONCE(!fault) || !fault->is_private)
> 		return 0;
>
> 	rc = snp_lookup_rmpentry(fault->pfn, &assigned, &level);
> 	if (rc || !assigned)
> 		return PG_LEVEL_4K;
>
> 	return level;
> }

I like this. Thanks for the suggestion, I'll pass Fuad some patch(es)
for v13.

>> +/*
>> + * Returns true if the given gfn's private/shared status (in the CoCo sense) is
>> + * private.
>> + *
>> + * A return value of false indicates that the gfn is explicitly or implicitly
>> + * shared (i.e., non-CoCo VMs).
>> + */
>>  static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>>  {
>> -	return IS_ENABLED(CONFIG_KVM_GMEM) &&
>> -	       kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE;
>> +	struct kvm_memory_slot *slot;
>> +
>> +	if (!IS_ENABLED(CONFIG_KVM_GMEM))
>> +		return false;
>> +
>> +	slot = gfn_to_memslot(kvm, gfn);
>> +	if (kvm_slot_has_gmem(slot) && kvm_gmem_memslot_supports_shared(slot)) {
>> +		/*
>> +		 * Without in-place conversion support, if a guest_memfd memslot
>> +		 * supports shared memory, then all the slot's memory is
>> +		 * considered not private, i.e., implicitly shared.
>> +		 */
>> +		return false;
>
> Why!?!?  Just make sure KVM_MEMORY_ATTRIBUTE_PRIVATE is mutually exclusive with
> mappable guest_memfd.  You need to do that no matter what. 

Thanks, I agree that setting KVM_MEMORY_ATTRIBUTE_PRIVATE should be
disallowed for gfn ranges whose slot is guest_memfd-only. Missed that
out. Where do people think we should check the mutual exclusivity?

In kvm_supported_mem_attributes() I'm thiking that we should still allow
the use of KVM_MEMORY_ATTRIBUTE_PRIVATE for other non-guest_memfd-only
gfn ranges. Or do people think we should just disallow
KVM_MEMORY_ATTRIBUTE_PRIVATE for the entire VM as long as one memslot is
a guest_memfd-only memslot?

If we check mutually exclusivity when handling
kvm_vm_set_memory_attributes(), as long as part of the range where
KVM_MEMORY_ATTRIBUTE_PRIVATE is requested to be set intersects a range
whose slot is guest_memfd-only, the ioctl will return EINVAL.

> Then you don't need
> to sprinkle special case code all over the place.
>

That's true, thanks.

I guess the special-casing will come back when guest_memfd supports
conversions (and stores shareability). After guest_memfd supports
conversions, if guest_memfd-only memslot, check with guest_memfd. Else,
look up memory attributes with kvm_get_memory_attributes().

>> +	}
>> +
>> +	return kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE;
>>  }
>>  #else
>>  static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>> -- 
>> 2.50.0.rc0.642.g800a2b2222-goog
>>