On Thu, Mar 10, 2022, Chao Peng wrote: > @@ -3890,7 +3893,59 @@ static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch); > } > > -static bool kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault, int *r) > +static bool kvm_vcpu_is_private_gfn(struct kvm_vcpu *vcpu, gfn_t gfn) > +{ > + /* > + * At this time private gfn has not been supported yet. Other patch > + * that enables it should change this. > + */ > + return false; > +} > + > +static bool kvm_faultin_pfn_private(struct kvm_vcpu *vcpu, > + struct kvm_page_fault *fault, > + bool *is_private_pfn, int *r) @is_private_pfn should be a field in @fault, not a separate parameter, and it should be a const property set by the original caller. I would also name it "is_private", because if KVM proceeds past this point, it will be a property of the fault/access _and_ the pfn I say it's a property of the fault because the below kvm_vcpu_is_private_gfn() should instead be: if (fault->is_private) The kvm_vcpu_is_private_gfn() check is TDX centric. For SNP, private vs. shared is communicated via error code. For software-only (I'm being optimistic ;-) ), we'd probably need to track private vs. shared internally in KVM, I don't think we'd want to force it to be a property of the gfn. Then you can also move the fault->is_private waiver into is_page_fault_stale(), and drop the local is_private_pfn in direct_page_fault(). > +{ > + int order; > + unsigned int flags = 0; > + struct kvm_memory_slot *slot = fault->slot; > + long pfn = kvm_memfile_get_pfn(slot, fault->gfn, &order); If get_lock_pfn() and thus kvm_memfile_get_pfn() returns a pure error code instead of multiplexing the pfn, then this can be: bool is_private_pfn; is_private_pfn = !!kvm_memfile_get_pfn(slot, fault->gfn, &fault->pfn, &order); That self-documents the "pfn < 0" == shared logic. > + > + if (kvm_vcpu_is_private_gfn(vcpu, fault->addr >> PAGE_SHIFT)) { > + if (pfn < 0) > + flags |= KVM_MEMORY_EXIT_FLAG_PRIVATE; > + else { > + fault->pfn = pfn; > + if (slot->flags & KVM_MEM_READONLY) > + fault->map_writable = false; > + else > + fault->map_writable = true; > + > + if (order == 0) > + fault->max_level = PG_LEVEL_4K; This doesn't correctly handle order > 0, but less than the next page size, in which case max_level needs to be PG_LEVEL_4k. It also doesn't handle the case where max_level > PG_LEVEL_2M. That said, I think the proper fix is to have the get_lock_pfn() API return the max mapping level, not the order. KVM, and presumably any other secondary MMU that might use these APIs, doesn't care about the order of the struct page, KVM cares about the max size/level of page it can map into the guest. And similar to the previous patch, "order" is specific to struct page, which we are trying to avoid. > + *is_private_pfn = true; This is where KVM guarantees that is_private_pfn == fault->is_private. > + *r = RET_PF_FIXED; > + return true; Ewww. This is super confusing. Ditto for the "*r = -1" magic number. I totally understand why you took this approach, it's just hard to follow because it kinda follows the kvm_faultin_pfn() semantics, but then inverts true and false in this one case. I think the least awful option is to forego the helper and open code everything. If we ever refactor kvm_faultin_pfn() to be less weird then we can maybe move this to a helper. Open coding isn't too bad if you reorganize things so that the exit-to-userspace path is a dedicated, early check. IMO, it's a lot easier to read this way, open coded or not. I think this is correct? "is_private_pfn" and "level" are locals, everything else is in @fault. if (kvm_slot_is_private(slot)) { is_private_pfn = !!kvm_memfile_get_pfn(slot, fault->gfn, &fault->pfn, &level); if (fault->is_private != is_private_pfn) { if (is_private_pfn) kvm_memfile_put_pfn(slot, fault->pfn); vcpu->run->exit_reason = KVM_EXIT_MEMORY_ERROR; if (fault->is_private) vcpu->run->memory.flags = KVM_MEMORY_EXIT_FLAG_PRIVATE; else vcpu->run->memory.flags = 0; vcpu->run->memory.padding = 0; vcpu->run->memory.gpa = fault->gfn << PAGE_SHIFT; vcpu->run->memory.size = PAGE_SIZE; *r = 0; return true; } /* * fault->pfn is all set if the fault is for a private pfn, just * need to update other metadata. */ if (fault->is_private) { fault->max_level = min(fault->max_level, level); fault->map_writable = !(slot->flags & KVM_MEM_READONLY); return false; } /* Fault is shared, fallthrough to the standard path. */ } async = false; > @@ -4016,7 +4076,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > else > write_lock(&vcpu->kvm->mmu_lock); > > - if (is_page_fault_stale(vcpu, fault, mmu_seq)) > + if (!is_private_pfn && is_page_fault_stale(vcpu, fault, mmu_seq)) As above, I'd prefer this check go in is_page_fault_stale(). It means shadow MMUs will suffer a pointless check, but I don't think that's a big issue. Oooh, unless we support software-only, which would play nice with nested and probably even legacy shadow paging. Fun :-) > goto out_unlock; > > r = make_mmu_pages_available(vcpu); > @@ -4033,7 +4093,12 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > read_unlock(&vcpu->kvm->mmu_lock); > else > write_unlock(&vcpu->kvm->mmu_lock); > - kvm_release_pfn_clean(fault->pfn); > + > + if (is_private_pfn) And this can be if (fault->is_private) Same feedback for paging_tmpl.h.