Re: [PATCH v2 03/16] KVM: MMU: properly assert spte on rmap walking path

2012-04-15 Thread Xiao Guangrong 
On 04/14/2012 10:15 AM, Takuya Yoshikawa wrote:

 On Fri, 13 Apr 2012 18:10:45 +0800
 Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote:
 
  static u64 *rmap_get_next(struct rmap_iterator *iter)
  {
 +u64 *sptep = NULL;
 +
  if (iter-desc) {
  if (iter-pos  PTE_LIST_EXT - 1) {
 -u64 *sptep;
 -
  ++iter-pos;
  sptep = iter-desc-sptes[iter-pos];
  if (sptep)
 -return sptep;
 +goto exit;
  }

  iter-desc = iter-desc-more;
 @@ -1028,11 +1036,14 @@ static u64 *rmap_get_next(struct rmap_iterator *iter)
  if (iter-desc) {
  iter-pos = 0;
  /* desc-sptes[0] cannot be NULL */
 -return iter-desc-sptes[iter-pos];
 +sptep = iter-desc-sptes[iter-pos];
 +goto exit;
  }
  }

 -return NULL;
 +exit:
 +WARN_ON(sptep  !is_shadow_present_pte(*sptep));
 +return sptep;
  }
 
 This will, probably, again force rmap_get_next function-call even with 
 EPT/NPT:
 CPU cannot skip it by branch prediction.
 

No, EPT/NPT also needs it.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 03/16] KVM: MMU: properly assert spte on rmap walking path

2012-04-13 Thread Xiao Guangrong 
Only test present bit is not enough since mmio spte is also set this
bit, use is_shadow_present_pte() instead of it

Also move the BUG_ONs to the common function to cleanup the code

Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
 arch/x86/kvm/mmu.c |   38 --
 1 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index aee13c6..91518b6 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -993,17 +993,25 @@ struct rmap_iterator {
  */
 static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter)
 {
+   u64 *sptep;
+
if (!rmap)
return NULL;

if (!(rmap  1)) {
iter-desc = NULL;
-   return (u64 *)rmap;
+   sptep = (u64 *)rmap;
+
+   goto exit;
}

iter-desc = (struct pte_list_desc *)(rmap  ~1ul);
iter-pos = 0;
-   return iter-desc-sptes[iter-pos];
+   sptep = iter-desc-sptes[iter-pos];
+
+exit:
+   WARN_ON(sptep  !is_shadow_present_pte(*sptep));
+   return sptep;
 }

 /*
@@ -1013,14 +1021,14 @@ static u64 *rmap_get_first(unsigned long rmap, struct 
rmap_iterator *iter)
  */
 static u64 *rmap_get_next(struct rmap_iterator *iter)
 {
+   u64 *sptep = NULL;
+
if (iter-desc) {
if (iter-pos  PTE_LIST_EXT - 1) {
-   u64 *sptep;
-
++iter-pos;
sptep = iter-desc-sptes[iter-pos];
if (sptep)
-   return sptep;
+   goto exit;
}

iter-desc = iter-desc-more;
@@ -1028,11 +1036,14 @@ static u64 *rmap_get_next(struct rmap_iterator *iter)
if (iter-desc) {
iter-pos = 0;
/* desc-sptes[0] cannot be NULL */
-   return iter-desc-sptes[iter-pos];
+   sptep = iter-desc-sptes[iter-pos];
+   goto exit;
}
}

-   return NULL;
+exit:
+   WARN_ON(sptep  !is_shadow_present_pte(*sptep));
+   return sptep;
 }

 static void drop_spte(struct kvm *kvm, u64 *sptep)
@@ -1048,7 +1059,6 @@ static int __rmap_write_protect(struct kvm *kvm, unsigned 
long *rmapp, int level
int write_protected = 0;

for (sptep = rmap_get_first(*rmapp, iter); sptep;) {
-   BUG_ON(!(*sptep  PT_PRESENT_MASK));
rmap_printk(rmap_write_protect: spte %p %llx\n, sptep, 
*sptep);

if (!is_writable_pte(*sptep)) {
@@ -1123,7 +1133,6 @@ static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long 
*rmapp,
int need_tlb_flush = 0;

while ((sptep = rmap_get_first(*rmapp, iter))) {
-   BUG_ON(!(*sptep  PT_PRESENT_MASK));
rmap_printk(kvm_rmap_unmap_hva: spte %p %llx\n, sptep, 
*sptep);

drop_spte(kvm, sptep);
@@ -1147,7 +1156,6 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned 
long *rmapp,
new_pfn = pte_pfn(*ptep);

for (sptep = rmap_get_first(*rmapp, iter); sptep;) {
-   BUG_ON(!is_shadow_present_pte(*sptep));
rmap_printk(kvm_set_pte_rmapp: spte %p %llx\n, sptep, *sptep);

need_flush = 1;
@@ -1242,14 +1250,11 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long 
*rmapp,
return kvm_unmap_rmapp(kvm, rmapp, data);

for (sptep = rmap_get_first(*rmapp, iter); sptep;
-sptep = rmap_get_next(iter)) {
-   BUG_ON(!(*sptep  PT_PRESENT_MASK));
-
+sptep = rmap_get_next(iter))
if (*sptep  PT_ACCESSED_MASK) {
young = 1;
clear_bit(PT_ACCESSED_SHIFT, (unsigned long *)sptep);
}
-   }

return young;
 }
@@ -1270,14 +1275,11 @@ static int kvm_test_age_rmapp(struct kvm *kvm, unsigned 
long *rmapp,
goto out;

for (sptep = rmap_get_first(*rmapp, iter); sptep;
-sptep = rmap_get_next(iter)) {
-   BUG_ON(!(*sptep  PT_PRESENT_MASK));
-
+sptep = rmap_get_next(iter))
if (*sptep  PT_ACCESSED_MASK) {
young = 1;
break;
}
-   }
 out:
return young;
 }
-- 
1.7.7.6

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/16] KVM: MMU: properly assert spte on rmap walking path

2012-04-13 Thread Takuya Yoshikawa 
On Fri, 13 Apr 2012 18:10:45 +0800
Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote:

  static u64 *rmap_get_next(struct rmap_iterator *iter)
  {
 + u64 *sptep = NULL;
 +
   if (iter-desc) {
   if (iter-pos  PTE_LIST_EXT - 1) {
 - u64 *sptep;
 -
   ++iter-pos;
   sptep = iter-desc-sptes[iter-pos];
   if (sptep)
 - return sptep;
 + goto exit;
   }
 
   iter-desc = iter-desc-more;
 @@ -1028,11 +1036,14 @@ static u64 *rmap_get_next(struct rmap_iterator *iter)
   if (iter-desc) {
   iter-pos = 0;
   /* desc-sptes[0] cannot be NULL */
 - return iter-desc-sptes[iter-pos];
 + sptep = iter-desc-sptes[iter-pos];
 + goto exit;
   }
   }
 
 - return NULL;
 +exit:
 + WARN_ON(sptep  !is_shadow_present_pte(*sptep));
 + return sptep;
  }

This will, probably, again force rmap_get_next function-call even with EPT/NPT:
CPU cannot skip it by branch prediction.

Takuya
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html