Re: [PATCH Part2 RFC v4 37/40] KVM: SVM: Add support to handle the RMP nested page fault

[Sean Christopherson <seanjc@google.com>]

On Wed, Jul 07, 2021, Brijesh Singh wrote:
> Follow the recommendation from APM2 section 15.36.10 and 15.36.11 to
> resolve the RMP violation encountered during the NPT table walk.

Heh, please elaborate on exactly what that recommendation is.  A recommendation
isn't exactly architectural, i.e. is subject to change :-)

And, do we have to follow the APM's recommendation?  Specifically, can KVM treat
#NPF RMP violations as guest errors, or is that not allowed by the GHCB spec?
I.e. can we mandate accesses be preceded by page state change requests?  It would
simplify KVM (albeit not much of a simplificiation) and would also make debugging
easier since transitions would require an explicit guest request and guest bugs
would result in errors instead of random corruption/weirdness.

> index 46323af09995..117e2e08d7ed 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1399,6 +1399,9 @@ struct kvm_x86_ops {
>  
>  	void (*write_page_begin)(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn);
>  	void (*write_page_end)(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn);
> +
> +	int (*handle_rmp_page_fault)(struct kvm_vcpu *vcpu, gpa_t gpa, kvm_pfn_t pfn,
> +			int level, u64 error_code);
>  };
>  
>  struct kvm_x86_nested_ops {
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index e60f54455cdc..b6a676ba1862 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5096,6 +5096,18 @@ static void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
>  	write_unlock(&vcpu->kvm->mmu_lock);
>  }
>  
> +static int handle_rmp_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
> +{
> +	kvm_pfn_t pfn;
> +	int level;
> +
> +	if (unlikely(!kvm_mmu_get_tdp_walk(vcpu, gpa, &pfn, &level)))
> +		return RET_PF_RETRY;
> +
> +	kvm_x86_ops.handle_rmp_page_fault(vcpu, gpa, pfn, level, error_code);
> +	return RET_PF_RETRY;
> +}
> +
>  int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
>  		       void *insn, int insn_len)
>  {
> @@ -5112,6 +5124,14 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
>  			goto emulate;
>  	}
>  
> +	if (unlikely(error_code & PFERR_GUEST_RMP_MASK)) {
> +		r = handle_rmp_page_fault(vcpu, cr2_or_gpa, error_code);

Adding a kvm_x86_ops hook is silly, there's literally one path, npf_interception()
that can encounter RMP violations.  Just invoke snp_handle_rmp_page_fault() from
there.  That works even if kvm_mmu_get_tdp_walk() stays around since it was
exported earlier.

> +		if (r == RET_PF_RETRY)
> +			return 1;
> +		else
> +			return r;
> +	}
> +
>  	if (r == RET_PF_INVALID) {
>  		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa,
>  					  lower_32_bits(error_code), false);
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 839cf321c6dd..53a60edc810e 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3519,3 +3519,60 @@ void sev_snp_write_page_begin(struct kvm *kvm, struct kvm_memory_slot *slot, gfn
>  		BUG_ON(rc != 0);
>  	}
>  }
> +
> +int snp_handle_rmp_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, kvm_pfn_t pfn,
> +			      int level, u64 error_code)
> +{
> +	struct rmpentry *e;
> +	int rlevel, rc = 0;
> +	bool private;
> +	gfn_t gfn;
> +
> +	e = snp_lookup_page_in_rmptable(pfn_to_page(pfn), &rlevel);
> +	if (!e)
> +		return 1;
> +
> +	private = !!(error_code & PFERR_GUEST_ENC_MASK);
> +
> +	/*
> +	 * See APM section 15.36.11 on how to handle the RMP fault for the large pages.

Please do not punt the reader to the APM for things like this.  It's ok when there
are gory details about CPU behavior that aren't worth commenting, but under no
circumstance should KVM's software implementation be "documented" in a CPU spec.

> +	 *
> +	 *  npt	     rmp    access      action
> +	 *  --------------------------------------------------
> +	 *  4k       2M     C=1       psmash
> +	 *  x        x      C=1       if page is not private then add a new RMP entry
> +	 *  x        x      C=0       if page is private then make it shared
> +	 *  2M       4k     C=x       zap
> +	 */
> +	if ((error_code & PFERR_GUEST_SIZEM_MASK) ||
> +	    ((level == PG_LEVEL_4K) && (rlevel == PG_LEVEL_2M) && private)) {
> +		rc = snp_rmptable_psmash(vcpu, pfn);
> +		goto zap_gfn;
> +	}
> +
> +	/*
> +	 * If it's a private access, and the page is not assigned in the RMP table, create a
> +	 * new private RMP entry.
> +	 */
> +	if (!rmpentry_assigned(e) && private) {
> +		rc = snp_make_page_private(vcpu, gpa, pfn, PG_LEVEL_4K);
> +		goto zap_gfn;
> +	}
> +
> +	/*
> +	 * If it's a shared access, then make the page shared in the RMP table.
> +	 */
> +	if (rmpentry_assigned(e) && !private)
> +		rc = snp_make_page_shared(vcpu, gpa, pfn, PG_LEVEL_4K);

Hrm, this really feels like it needs to be protected by mmu_lock.  Functionally,
it might all work out in the end after enough RMP violations, but it's extremely
difficult to reason about and probably even more difficult if multiple vCPUs end
up fighting over a gfn.

My gut reaction is that this is also backwards, i.e. KVM should update the RMP
to match its TDP SPTEs, not the other way around.

The one big complication is that the TDP MMU only takes mmu_lock for read.  A few
options come to mind but none of them are all that pretty.  I'll wait to hear back
on whether or not we can make PSC request mandatory before thinking too hard on
this one.

> +zap_gfn:
> +	/*
> +	 * Now that we have updated the RMP pagesize, zap the existing rmaps for
> +	 * large entry ranges so that nested page table gets rebuilt with the updated RMP
> +	 * pagesize.
> +	 */
> +	gfn = gpa_to_gfn(gpa) & ~(KVM_PAGES_PER_HPAGE(PG_LEVEL_2M) - 1);
> +	kvm_zap_gfn_range(vcpu->kvm, gfn, gfn + 512);
> +
> +	return 0;
> +}