Received-SPF: pass (google.com: domain of linux-kernel+bounces-147759-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 16 Apr 2024 16:43:34 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Chao Gao <chao.gao@intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org, isaku.yamahata@gmail.com,
	linux-kernel@vger.kernel.org,
	Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Michael Roth <michael.roth@amd.com>,
	David Matlack <dmatlack@google.com>,
	Federico Parola <federico.parola@polito.it>,
	Kai Huang <kai.huang@intel.com>, isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v2 03/10] KVM: x86/mmu: Extract __kvm_mmu_do_page_fault()
Message-ID: <20240416234334.GA3039520@ls.amr.corp.intel.com>
References: <cover.1712785629.git.isaku.yamahata@intel.com>
 <ddf1d98420f562707b11e12c416cce8fdb986bb1.1712785629.git.isaku.yamahata@intel.com>
 <Zh41S8fh0IvXlKwX@chao-email>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Zh41S8fh0IvXlKwX@chao-email>

On Tue, Apr 16, 2024 at 04:22:35PM +0800,
Chao Gao <chao.gao@intel.com> wrote:

> 
> >This patch makes the emulation_type always set irrelevant to the return
> >code.  kvm_mmu_page_fault() is the only caller of kvm_mmu_do_page_fault(),
> >and references the value only when PF_RET_EMULATE is returned.  Therefore,
> >this adjustment doesn't affect functionality.
>
> This is benign. But what's the benefit of doing this?

To avoid increment vcpu->stat.  Because originally this was VM ioctl, I wanted
to avoid touch vCPU stat.  Now it's vCPU ioctl, it's fine to increment them.

Probably we can drop this patch and use kvm_mmu_do_page_fault().

> 
> >+static inline int __kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> >+					  u64 err, bool prefetch, int *emulation_type)
> > {
> > 	struct kvm_page_fault fault = {
> > 		.addr = cr2_or_gpa,
> >@@ -318,14 +318,6 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> > 		fault.slot = kvm_vcpu_gfn_to_memslot(vcpu, fault.gfn);
> > 	}
> > 
> >-	/*
> >-	 * Async #PF "faults", a.k.a. prefetch faults, are not faults from the
> >-	 * guest perspective and have already been counted at the time of the
> >-	 * original fault.
> >-	 */
> >-	if (!prefetch)
> >-		vcpu->stat.pf_taken++;
> >-
> > 	if (IS_ENABLED(CONFIG_MITIGATION_RETPOLINE) && fault.is_tdp)
> > 		r = kvm_tdp_page_fault(vcpu, &fault);
> > 	else
> >@@ -333,12 +325,30 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> > 
> > 	if (r == RET_PF_EMULATE && fault.is_private) {
> > 		kvm_mmu_prepare_memory_fault_exit(vcpu, &fault);
> >-		return -EFAULT;
> >+		r = -EFAULT;
> > 	}
> > 
> > 	if (fault.write_fault_to_shadow_pgtable && emulation_type)
> > 		*emulation_type |= EMULTYPE_WRITE_PF_TO_SP;
> > 
> >+	return r;
> >+}
> >+
> >+static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> >+					u64 err, bool prefetch, int *emulation_type)
> >+{
> >+	int r;
> >+
> >+	/*
> >+	 * Async #PF "faults", a.k.a. prefetch faults, are not faults from the
> >+	 * guest perspective and have already been counted at the time of the
> >+	 * original fault.
> >+	 */
> >+	if (!prefetch)
> >+		vcpu->stat.pf_taken++;
> >+
> >+	r = __kvm_mmu_do_page_fault(vcpu, cr2_or_gpa, err, prefetch, emulation_type);
> 
> bail out if r < 0?

The following if clauses checks RET_PF_xxx > 0. 
-- 
Isaku Yamahata <isaku.yamahata@intel.com>