[PATCH 0/5] KVM: x86/mmu: Clean up is_tdp_mmu_root and root_hpa checks

[PATCH 0/5] KVM: x86/mmu: Clean up is_tdp_mmu_root and root_hpa checks

[David Matlack]

This series is spun off from Sean's suggestions on [PATCH 1/8] of my TDP
MMU Fast Page Fault series [1].

Patches 1-2 and 4 cleans up some redundant code in the page fault handling path:
 - Redundant checks for TDP MMU enablement.
 - Redundant checks for root_hpa validity.

Patch 3 refactors is_tdp_mmu_root into a simpler function prototype.

Note to reviewers: I purposely opted not to remove the root_hpa check
from is_tdp_mmu even though it is theoretically redundant in the current
code. My rational is that it could be called from outside the page fault
handling code in the future where root_hpa can be invalid. This seems
more likely to happen than with the other functions since is_tdp_mmu()
is not inherently tied to page fault handling.

The cost of getting this wrong is high since the result would be we end
up calling executing pfn_to_page(-1 >> PAGE_SHIFT)->private in
to_shadow_page. A better solution might be to move the VALID_PAGE check
into to_shadow_page but I did not want to expand the scope of this
series.

To test this series I ran all kvm-unit-tests and KVM selftests on an
Intel Cascade Lake machine.

[1] https://lore.kernel.org/kvm/YMepDK40DLkD4DSy@google.com/

David Matlack (4):
  KVM: x86/mmu: Remove redundant is_tdp_mmu_root check
  KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check
  KVM: x86/mmu: Refactor is_tdp_mmu_root into is_tdp_mmu
  KVM: x86/mmu: Remove redundant root_hpa checks

 arch/x86/kvm/mmu/mmu.c     | 19 ++++++-------------
 arch/x86/kvm/mmu/tdp_mmu.c |  5 -----
 arch/x86/kvm/mmu/tdp_mmu.h |  5 ++---
 3 files changed, 8 insertions(+), 21 deletions(-)

-- 
2.32.0.288.g62a8d224e6-goog

[PATCH 1/4] KVM: x86/mmu: Remove redundant is_tdp_mmu_root check

[David Matlack]

The check for is_tdp_mmu_root in kvm_tdp_mmu_map is redundant because
kvm_tdp_mmu_map's only caller (direct_page_fault) already checks
is_tdp_mmu_root.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 95eeb5ac6a8a..5888f2ba417c 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -979,8 +979,6 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 
 	if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root_hpa)))
 		return RET_PF_RETRY;
-	if (WARN_ON(!is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa)))
-		return RET_PF_RETRY;
 
 	level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn,
 					huge_page_disallowed, &req_level);
-- 
2.32.0.288.g62a8d224e6-goog

[PATCH 2/4] KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check

[David Matlack]

This check is redundant because the root shadow page will only be a TDP
MMU page if is_tdp_mmu_enabled() returns true, and is_tdp_mmu_enabled()
never changes for the lifetime of a VM.

It's possible that this check was added for performance reasons but it
is unlikely that it is useful in practice since to_shadow_page() is
cheap. That being said, this patch also caches the return value of
is_tdp_mmu_root() in direct_page_fault() since there's no reason to
duplicate the call so many times, so performance is not a concern.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/mmu.c     | 11 ++++++-----
 arch/x86/kvm/mmu/tdp_mmu.h |  4 +---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 0144c40d09c7..1e6bf2e207f6 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3545,7 +3545,7 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
 		return reserved;
 	}
 
-	if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
+	if (is_tdp_mmu_root(vcpu->arch.mmu->root_hpa))
 		leaf = kvm_tdp_mmu_get_walk(vcpu, addr, sptes, &root);
 	else
 		leaf = get_walk(vcpu, addr, sptes, &root);
@@ -3717,6 +3717,7 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 			     bool prefault, int max_level, bool is_tdp)
 {
+	bool is_tdp_mmu_fault = is_tdp_mmu_root(vcpu->arch.mmu->root_hpa);
 	bool write = error_code & PFERR_WRITE_MASK;
 	bool map_writable;
 
@@ -3729,7 +3730,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 	if (page_fault_handle_page_track(vcpu, error_code, gfn))
 		return RET_PF_EMULATE;
 
-	if (!is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa)) {
+	if (!is_tdp_mmu_fault) {
 		r = fast_page_fault(vcpu, gpa, error_code);
 		if (r != RET_PF_INVALID)
 			return r;
@@ -3751,7 +3752,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 
 	r = RET_PF_RETRY;
 
-	if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
+	if (is_tdp_mmu_fault)
 		read_lock(&vcpu->kvm->mmu_lock);
 	else
 		write_lock(&vcpu->kvm->mmu_lock);
@@ -3762,7 +3763,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 	if (r)
 		goto out_unlock;
 
-	if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
+	if (is_tdp_mmu_fault)
 		r = kvm_tdp_mmu_map(vcpu, gpa, error_code, map_writable, max_level,
 				    pfn, prefault);
 	else
@@ -3770,7 +3771,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 				 prefault, is_tdp);
 
 out_unlock:
-	if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
+	if (is_tdp_mmu_fault)
 		read_unlock(&vcpu->kvm->mmu_lock);
 	else
 		write_unlock(&vcpu->kvm->mmu_lock);
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index 5fdf63090451..843ca2127faf 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -91,12 +91,10 @@ static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
 static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
 #endif
 
-static inline bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa)
+static inline bool is_tdp_mmu_root(hpa_t hpa)
 {
 	struct kvm_mmu_page *sp;
 
-	if (!is_tdp_mmu_enabled(kvm))
-		return false;
 	if (WARN_ON(!VALID_PAGE(hpa)))
 		return false;
 
-- 
2.32.0.288.g62a8d224e6-goog

Re: [PATCH 2/4] KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7776 bytes --]

Hi David,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/queue]
[also build test ERROR on vhost/linux-next v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Matlack/KVM-x86-mmu-Remove-redundant-is_tdp_mmu_root-check/20210618-082018
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
config: i386-randconfig-a016-20210618 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/6ab060f3cf9061da492b1eb89808eb2da5406781
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review David-Matlack/KVM-x86-mmu-Remove-redundant-is_tdp_mmu_root-check/20210618-082018
        git checkout 6ab060f3cf9061da492b1eb89808eb2da5406781
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: arch/x86/kvm/mmu/mmu.o: in function `get_mmio_spte':
>> arch/x86/kvm/mmu/mmu.c:3612: undefined reference to `kvm_tdp_mmu_get_walk'
   ld: arch/x86/kvm/mmu/mmu.o: in function `direct_page_fault':
>> arch/x86/kvm/mmu/mmu.c:3830: undefined reference to `kvm_tdp_mmu_map'


vim +3612 arch/x86/kvm/mmu/mmu.c

95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3597  
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3598  /* return true if reserved bit(s) are detected on a valid, non-MMIO SPTE. */
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3599  static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3600  {
dde81f9477d018 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3601  	u64 sptes[PT64_ROOT_MAX_LEVEL + 1];
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3602  	struct rsvd_bits_validate *rsvd_check;
39b4d43e6003ce arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3603  	int root, leaf, level;
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3604  	bool reserved = false;
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3605  
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3606  	if (!VALID_PAGE(vcpu->arch.mmu->root_hpa)) {
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3607  		*sptep = 0ull;
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3608  		return reserved;
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3609  	}
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3610  
6ab060f3cf9061 arch/x86/kvm/mmu/mmu.c David Matlack       2021-06-17  3611  	if (is_tdp_mmu_root(vcpu->arch.mmu->root_hpa))
39b4d43e6003ce arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17 @3612  		leaf = kvm_tdp_mmu_get_walk(vcpu, addr, sptes, &root);
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3613  	else
39b4d43e6003ce arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3614  		leaf = get_walk(vcpu, addr, sptes, &root);
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3615  
2aa078932ff6c6 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3616  	if (unlikely(leaf < 0)) {
2aa078932ff6c6 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3617  		*sptep = 0ull;
2aa078932ff6c6 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3618  		return reserved;
2aa078932ff6c6 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3619  	}
2aa078932ff6c6 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3620  
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3621  	*sptep = sptes[leaf];
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3622  
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3623  	/*
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3624  	 * Skip reserved bits checks on the terminal leaf if it's not a valid
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3625  	 * SPTE.  Note, this also (intentionally) skips MMIO SPTEs, which, by
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3626  	 * design, always have reserved bits set.  The purpose of the checks is
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3627  	 * to detect reserved bits on non-MMIO SPTEs. i.e. buggy SPTEs.
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3628  	 */
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3629  	if (!is_shadow_present_pte(sptes[leaf]))
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3630  		leaf++;
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3631  
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3632  	rsvd_check = &vcpu->arch.mmu->shadow_zero_check;
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3633  
9aa418792f5f11 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3634  	for (level = root; level >= leaf; level--)
b5c3c1b3c6e95c arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-01-09  3635  		/*
b5c3c1b3c6e95c arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-01-09  3636  		 * Use a bitwise-OR instead of a logical-OR to aggregate the
b5c3c1b3c6e95c arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-01-09  3637  		 * reserved bit and EPT's invalid memtype/XWR checks to avoid
b5c3c1b3c6e95c arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-01-09  3638  		 * adding a Jcc in the loop.
b5c3c1b3c6e95c arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-01-09  3639  		 */
dde81f9477d018 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3640  		reserved |= __is_bad_mt_xwr(rsvd_check, sptes[level]) |
dde81f9477d018 arch/x86/kvm/mmu/mmu.c Sean Christopherson 2020-12-17  3641  			    __is_rsvd_bits_set(rsvd_check, sptes[level], level);
47ab8751695f71 arch/x86/kvm/mmu.c     Xiao Guangrong      2015-08-05  3642  
47ab8751695f71 arch/x86/kvm/mmu.c     Xiao Guangrong      2015-08-05  3643  	if (reserved) {
bb4cdf3af9395d arch/x86/kvm/mmu/mmu.c Sean Christopherson 2021-02-25  3644  		pr_err("%s: reserved bits set on MMU-present spte, addr 0x%llx, hierarchy:\n",
47ab8751695f71 arch/x86/kvm/mmu.c     Xiao Guangrong      2015-08-05  3645  		       __func__, addr);
95fb5b0258b7bd arch/x86/kvm/mmu/mmu.c Ben Gardon          2020-10-14  3646  		for (level = root; level >= leaf; level--)
bb4cdf3af9395d arch/x86/kvm/mmu/mmu.c Sean Christopherson 2021-02-25  3647  			pr_err("------ spte = 0x%llx level = %d, rsvd bits = 0x%llx",
bb4cdf3af9395d arch/x86/kvm/mmu/mmu.c Sean Christopherson 2021-02-25  3648  			       sptes[level], level,
bb4cdf3af9395d arch/x86/kvm/mmu/mmu.c Sean Christopherson 2021-02-25  3649  			       rsvd_check->rsvd_bits_mask[(sptes[level] >> 7) & 1][level-1]);
47ab8751695f71 arch/x86/kvm/mmu.c     Xiao Guangrong      2015-08-05  3650  	}
ddce6208217c1a arch/x86/kvm/mmu/mmu.c Sean Christopherson 2019-12-06  3651  
47ab8751695f71 arch/x86/kvm/mmu.c     Xiao Guangrong      2015-08-05  3652  	return reserved;
ce88decffd17bf arch/x86/kvm/mmu.c     Xiao Guangrong      2011-07-12  3653  }
ce88decffd17bf arch/x86/kvm/mmu.c     Xiao Guangrong      2011-07-12  3654  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34530 bytes --]

Re: [PATCH 2/4] KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check

[Paolo Bonzini]

On 18/06/21 09:17, kernel test robot wrote:
> Hi David,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kvm/queue]
> [also build test ERROR on vhost/linux-next v5.13-rc6]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:https://github.com/0day-ci/linux/commits/David-Matlack/KVM-x86-mmu-Remove-redundant-is_tdp_mmu_root-check/20210618-082018
> base:https://git.kernel.org/pub/scm/virt/kvm/kvm.git  queue
> config: i386-randconfig-a016-20210618 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
>          #https://github.com/0day-ci/linux/commit/6ab060f3cf9061da492b1eb89808eb2da5406781
>          git remote add linux-reviewhttps://github.com/0day-ci/linux
>          git fetch --no-tags linux-review David-Matlack/KVM-x86-mmu-Remove-redundant-is_tdp_mmu_root-check/20210618-082018
>          git checkout 6ab060f3cf9061da492b1eb89808eb2da5406781
>          # save the attached .config to linux build tree
>          make W=1 ARCH=i386
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot<lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     ld: arch/x86/kvm/mmu/mmu.o: in function `get_mmio_spte':
>>> arch/x86/kvm/mmu/mmu.c:3612: undefined reference to `kvm_tdp_mmu_get_walk'
>     ld: arch/x86/kvm/mmu/mmu.o: in function `direct_page_fault':
>>> arch/x86/kvm/mmu/mmu.c:3830: undefined reference to `kvm_tdp_mmu_map'

Turns out sometimes is_tdp_mmu_root is not inlined after this patch.
Fixed thusly:

--------- 8< -----------
Subject: [PATCH] KVM: x86: Stub out is_tdp_mmu_root on 32-bit hosts

If is_tdp_mmu_root is not inlined, the elimination of TDP MMU calls as dead
code might not work out.  To avoid this, explicitly declare the stubbed
is_tdp_mmu_root on 32-bit hosts.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index fabfea947e46..f6e0667cf4b6 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -85,12 +85,6 @@ bool kvm_mmu_init_tdp_mmu(struct kvm *kvm);
  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
  static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return kvm->arch.tdp_mmu_enabled; }
  static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return sp->tdp_mmu_page; }
-#else
-static inline bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return false; }
-static inline void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) {}
-static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
-static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
-#endif
  
  static inline bool is_tdp_mmu_root(hpa_t hpa)
  {
@@ -105,5 +99,12 @@ static inline bool is_tdp_mmu_root(hpa_t hpa)
  
         return is_tdp_mmu_page(sp) && sp->root_count;
  }
+#else
+static inline bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return false; }
+static inline void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) {}
+static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
+static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
+static inline bool is_tdp_mmu_root(hpa_t hpa) { return false; }
+#endif
  
  #endif /* __KVM_X86_MMU_TDP_MMU_H */

Re: [PATCH 2/4] KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check

[David Matlack]

On Fri, Jun 18, 2021 at 12:42:56PM +0200, Paolo Bonzini wrote:
> On 18/06/21 09:17, kernel test robot wrote:
> > Hi David,
> > 
> > Thank you for the patch! Yet something to improve:
> > 
> > [auto build test ERROR on kvm/queue]
> > [also build test ERROR on vhost/linux-next v5.13-rc6]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> > 
> > url:https://github.com/0day-ci/linux/commits/David-Matlack/KVM-x86-mmu-Remove-redundant-is_tdp_mmu_root-check/20210618-082018
> > base:https://git.kernel.org/pub/scm/virt/kvm/kvm.git  queue
> > config: i386-randconfig-a016-20210618 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> > reproduce (this is a W=1 build):
> >          #https://github.com/0day-ci/linux/commit/6ab060f3cf9061da492b1eb89808eb2da5406781
> >          git remote add linux-reviewhttps://github.com/0day-ci/linux
> >          git fetch --no-tags linux-review David-Matlack/KVM-x86-mmu-Remove-redundant-is_tdp_mmu_root-check/20210618-082018
> >          git checkout 6ab060f3cf9061da492b1eb89808eb2da5406781
> >          # save the attached .config to linux build tree
> >          make W=1 ARCH=i386
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot<lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> >     ld: arch/x86/kvm/mmu/mmu.o: in function `get_mmio_spte':
> > > > arch/x86/kvm/mmu/mmu.c:3612: undefined reference to `kvm_tdp_mmu_get_walk'
> >     ld: arch/x86/kvm/mmu/mmu.o: in function `direct_page_fault':
> > > > arch/x86/kvm/mmu/mmu.c:3830: undefined reference to `kvm_tdp_mmu_map'
> 
> Turns out sometimes is_tdp_mmu_root is not inlined after this patch.
> Fixed thusly:

Thanks for the fix. I guess after I removed the is_tdp_mmu_enabled()
check the compiler couldn't determine what is_tdp_mmu_root() would
return on 32-bit builds anymore.

Pretty nice of the compiler to throw out kvm_tdp_mmu_get_walk() and
kvm_tdp_mmu_map() once it knows they are not reachable so we don't have
to implement stubs for those functions that BUG_ON and pray they never
get called!

> 
> --------- 8< -----------
> Subject: [PATCH] KVM: x86: Stub out is_tdp_mmu_root on 32-bit hosts
> 
> If is_tdp_mmu_root is not inlined, the elimination of TDP MMU calls as dead
> code might not work out.  To avoid this, explicitly declare the stubbed
> is_tdp_mmu_root on 32-bit hosts.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> index fabfea947e46..f6e0667cf4b6 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.h
> +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> @@ -85,12 +85,6 @@ bool kvm_mmu_init_tdp_mmu(struct kvm *kvm);
>  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
>  static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return kvm->arch.tdp_mmu_enabled; }
>  static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return sp->tdp_mmu_page; }
> -#else
> -static inline bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return false; }
> -static inline void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) {}
> -static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
> -static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
> -#endif
>  static inline bool is_tdp_mmu_root(hpa_t hpa)
>  {
> @@ -105,5 +99,12 @@ static inline bool is_tdp_mmu_root(hpa_t hpa)
>         return is_tdp_mmu_page(sp) && sp->root_count;
>  }
> +#else
> +static inline bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return false; }
> +static inline void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) {}
> +static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
> +static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
> +static inline bool is_tdp_mmu_root(hpa_t hpa) { return false; }
> +#endif
>  #endif /* __KVM_X86_MMU_TDP_MMU_H */
> 

Re: [PATCH 2/4] KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check

[Paolo Bonzini]

On 18/06/21 18:47, David Matlack wrote:
> On Fri, Jun 18, 2021 at 12:42:56PM +0200, Paolo Bonzini wrote:
>> On 18/06/21 09:17, kernel test robot wrote:
>>>      ld: arch/x86/kvm/mmu/mmu.o: in function `get_mmio_spte':
>>>>> arch/x86/kvm/mmu/mmu.c:3612: undefined reference to `kvm_tdp_mmu_get_walk'
>>>      ld: arch/x86/kvm/mmu/mmu.o: in function `direct_page_fault':
>>>>> arch/x86/kvm/mmu/mmu.c:3830: undefined reference to `kvm_tdp_mmu_map'
>>
>> Turns out sometimes is_tdp_mmu_root is not inlined after this patch.
>> Fixed thusly:
> 
> Thanks for the fix. I guess after I removed the is_tdp_mmu_enabled()
> check the compiler couldn't determine what is_tdp_mmu_root() would
> return on 32-bit builds anymore.

It still could, but only as long as is_tdp_mmu_root was inlined.  Your 
patch caused the final "return false" to only appear after a few WARN_ON 
checks that obviously can't be removed, and therefore tipped the balance 
towards not inlining it!

Paolo

[PATCH 3/4] KVM: x86/mmu: Refactor is_tdp_mmu_root into is_tdp_mmu

[David Matlack]

This change simplifies the call sites slightly and also abstracts away
the implementation detail of looking at root_hpa as the mechanism for
determining if the mmu is the TDP MMU.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/mmu.c     | 4 ++--
 arch/x86/kvm/mmu/tdp_mmu.h | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 1e6bf2e207f6..4a4580243210 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3545,7 +3545,7 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
 		return reserved;
 	}
 
-	if (is_tdp_mmu_root(vcpu->arch.mmu->root_hpa))
+	if (is_tdp_mmu(vcpu->arch.mmu))
 		leaf = kvm_tdp_mmu_get_walk(vcpu, addr, sptes, &root);
 	else
 		leaf = get_walk(vcpu, addr, sptes, &root);
@@ -3717,7 +3717,7 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 			     bool prefault, int max_level, bool is_tdp)
 {
-	bool is_tdp_mmu_fault = is_tdp_mmu_root(vcpu->arch.mmu->root_hpa);
+	bool is_tdp_mmu_fault = is_tdp_mmu(vcpu->arch.mmu);
 	bool write = error_code & PFERR_WRITE_MASK;
 	bool map_writable;
 
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index 843ca2127faf..a63e35378e43 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -91,9 +91,10 @@ static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
 static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
 #endif
 
-static inline bool is_tdp_mmu_root(hpa_t hpa)
+static inline bool is_tdp_mmu(struct kvm_mmu *mmu)
 {
 	struct kvm_mmu_page *sp;
+	hpa_t hpa = mmu->root_hpa;
 
 	if (WARN_ON(!VALID_PAGE(hpa)))
 		return false;
-- 
2.32.0.288.g62a8d224e6-goog

[PATCH 4/4] KVM: x86/mmu: Remove redundant root_hpa checks

[David Matlack]

The root_hpa checks below the top-level check in kvm_mmu_page_fault are
theoretically redundant since there is no longer a way for the root_hpa
to be reset during a page fault. The details of why are described in
commit ddce6208217c ("KVM: x86/mmu: Move root_hpa validity checks to top
of page fault handler")

__direct_map, kvm_tdp_mmu_map, and get_mmio_spte are all only reachable
through kvm_mmu_page_fault, therefore their root_hpa checks are
redundant.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/mmu.c     | 8 --------
 arch/x86/kvm/mmu/tdp_mmu.c | 3 ---
 2 files changed, 11 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 4a4580243210..2e36430e54e6 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2827,9 +2827,6 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 	gfn_t gfn = gpa >> PAGE_SHIFT;
 	gfn_t base_gfn = gfn;
 
-	if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root_hpa)))
-		return RET_PF_RETRY;
-
 	level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn,
 					huge_page_disallowed, &req_level);
 
@@ -3540,11 +3537,6 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
 	int root, leaf, level;
 	bool reserved = false;
 
-	if (!VALID_PAGE(vcpu->arch.mmu->root_hpa)) {
-		*sptep = 0ull;
-		return reserved;
-	}
-
 	if (is_tdp_mmu(vcpu->arch.mmu))
 		leaf = kvm_tdp_mmu_get_walk(vcpu, addr, sptes, &root);
 	else
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 5888f2ba417c..fa436a4ae01d 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -977,9 +977,6 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
 	int level;
 	int req_level;
 
-	if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root_hpa)))
-		return RET_PF_RETRY;
-
 	level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn,
 					huge_page_disallowed, &req_level);
 
-- 
2.32.0.288.g62a8d224e6-goog

Re: [PATCH 0/5] KVM: x86/mmu: Clean up is_tdp_mmu_root and root_hpa checks

[Paolo Bonzini]

On 18/06/21 01:19, David Matlack wrote:
> This series is spun off from Sean's suggestions on [PATCH 1/8] of my TDP
> MMU Fast Page Fault series [1].
> 
> Patches 1-2 and 4 cleans up some redundant code in the page fault handling path:
>   - Redundant checks for TDP MMU enablement.
>   - Redundant checks for root_hpa validity.
> 
> Patch 3 refactors is_tdp_mmu_root into a simpler function prototype.
> 
> Note to reviewers: I purposely opted not to remove the root_hpa check
> from is_tdp_mmu even though it is theoretically redundant in the current
> code. My rational is that it could be called from outside the page fault
> handling code in the future where root_hpa can be invalid. This seems
> more likely to happen than with the other functions since is_tdp_mmu()
> is not inherently tied to page fault handling.
> 
> The cost of getting this wrong is high since the result would be we end
> up calling executing pfn_to_page(-1 >> PAGE_SHIFT)->private in
> to_shadow_page. A better solution might be to move the VALID_PAGE check
> into to_shadow_page but I did not want to expand the scope of this
> series.
> 
> To test this series I ran all kvm-unit-tests and KVM selftests on an
> Intel Cascade Lake machine.
> 
> [1] https://lore.kernel.org/kvm/YMepDK40DLkD4DSy@google.com/
> 
> David Matlack (4):
>    KVM: x86/mmu: Remove redundant is_tdp_mmu_root check
>    KVM: x86/mmu: Remove redundant is_tdp_mmu_enabled check
>    KVM: x86/mmu: Refactor is_tdp_mmu_root into is_tdp_mmu
>    KVM: x86/mmu: Remove redundant root_hpa checks
> 
>   arch/x86/kvm/mmu/mmu.c     | 19 ++++++-------------
>   arch/x86/kvm/mmu/tdp_mmu.c |  5 -----
>   arch/x86/kvm/mmu/tdp_mmu.h |  5 ++---
>   3 files changed, 8 insertions(+), 21 deletions(-)
> 

Queued, thanks.

Paolo