Re: [PATCH v3 3/6] KVM: MMU: make return value of mmio page fault handler more readable

2013-06-11 Thread Gleb Natapov 
On Mon, Jun 10, 2013 at 10:16:04PM +0900, Takuya Yoshikawa wrote:
 On Mon, 10 Jun 2013 10:57:50 +0300
 Gleb Natapov g...@redhat.com wrote:
 
  On Fri, Jun 07, 2013 at 04:51:25PM +0800, Xiao Guangrong wrote:
 
   +
   +/*
   + * Return values of handle_mmio_page_fault_common:
   + * RET_MMIO_PF_EMULATE: it is a real mmio page fault, emulate the 
   instruction
   + *directly.
   + * RET_MMIO_PF_RETRY: let CPU fault again on the address.
   + * RET_MMIO_PF_BUG: bug is detected.
   + */
   +enum {
   + RET_MMIO_PF_EMULATE = 1,
   + RET_MMIO_PF_RETRY = 0,
   + RET_MMIO_PF_BUG = -1
   +};
  I would order them from -1 to 1 and rename RET_MMIO_PF_BUG to
  RET_MMIO_PF_ERROR, but no need to resend just for that.
 
 Why not just let compilers select the values? -- It's an enum.
 Any reason to make it start from -1?
 
I am fine with this too as an additional patch. It makes sense to preserve
original values like Xiao did for initial patch, since it is easier to
verify that the patch is just a mechanical change.

--
Gleb.
--
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 v3 3/6] KVM: MMU: make return value of mmio page fault handler more readable

2013-06-10 Thread Gleb Natapov 
On Fri, Jun 07, 2013 at 04:51:25PM +0800, Xiao Guangrong wrote:
 Define some meaningful names instead of raw code
 
 Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
 ---
  arch/x86/kvm/mmu.c | 15 +--
  arch/x86/kvm/mmu.h | 14 ++
  arch/x86/kvm/vmx.c |  4 ++--
  3 files changed, 21 insertions(+), 12 deletions(-)
 
 diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
 index eca91bd..044d8c0 100644
 --- a/arch/x86/kvm/mmu.c
 +++ b/arch/x86/kvm/mmu.c
 @@ -3222,17 +3222,12 @@ static u64 walk_shadow_page_get_mmio_spte(struct 
 kvm_vcpu *vcpu, u64 addr)
   return spte;
  }
  
 -/*
 - * If it is a real mmio page fault, return 1 and emulat the instruction
 - * directly, return 0 to let CPU fault again on the address, -1 is
 - * returned if bug is detected.
 - */
  int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool 
 direct)
  {
   u64 spte;
  
   if (quickly_check_mmio_pf(vcpu, addr, direct))
 - return 1;
 + return RET_MMIO_PF_EMULATE;
  
   spte = walk_shadow_page_get_mmio_spte(vcpu, addr);
  
 @@ -3245,7 +3240,7 @@ int handle_mmio_page_fault_common(struct kvm_vcpu 
 *vcpu, u64 addr, bool direct)
  
   trace_handle_mmio_page_fault(addr, gfn, access);
   vcpu_cache_mmio_info(vcpu, addr, gfn, access);
 - return 1;
 + return RET_MMIO_PF_EMULATE;
   }
  
   /*
 @@ -3253,13 +3248,13 @@ int handle_mmio_page_fault_common(struct kvm_vcpu 
 *vcpu, u64 addr, bool direct)
* it's a BUG if the gfn is not a mmio page.
*/
   if (direct  !check_direct_spte_mmio_pf(spte))
 - return -1;
 + return RET_MMIO_PF_BUG;
  
   /*
* If the page table is zapped by other cpus, let CPU fault again on
* the address.
*/
 - return 0;
 + return RET_MMIO_PF_RETRY;
  }
  EXPORT_SYMBOL_GPL(handle_mmio_page_fault_common);
  
 @@ -3269,7 +3264,7 @@ static int handle_mmio_page_fault(struct kvm_vcpu 
 *vcpu, u64 addr,
   int ret;
  
   ret = handle_mmio_page_fault_common(vcpu, addr, direct);
 - WARN_ON(ret  0);
 + WARN_ON(ret == RET_MMIO_PF_BUG);
   return ret;
  }
  
 diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
 index 922bfae..ba6a19c 100644
 --- a/arch/x86/kvm/mmu.h
 +++ b/arch/x86/kvm/mmu.h
 @@ -52,6 +52,20 @@
  
  int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 
 sptes[4]);
  void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask);
 +
 +/*
 + * Return values of handle_mmio_page_fault_common:
 + * RET_MMIO_PF_EMULATE: it is a real mmio page fault, emulate the instruction
 + *directly.
 + * RET_MMIO_PF_RETRY: let CPU fault again on the address.
 + * RET_MMIO_PF_BUG: bug is detected.
 + */
 +enum {
 + RET_MMIO_PF_EMULATE = 1,
 + RET_MMIO_PF_RETRY = 0,
 + RET_MMIO_PF_BUG = -1
 +};
I would order them from -1 to 1 and rename RET_MMIO_PF_BUG to
RET_MMIO_PF_ERROR, but no need to resend just for that.

 +
  int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool 
 direct);
  int kvm_init_shadow_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *context);
  
 diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
 index 78ee123..85c8d51 100644
 --- a/arch/x86/kvm/vmx.c
 +++ b/arch/x86/kvm/vmx.c
 @@ -5366,10 +5366,10 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
   gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
  
   ret = handle_mmio_page_fault_common(vcpu, gpa, true);
 - if (likely(ret == 1))
 + if (likely(ret == RET_MMIO_PF_EMULATE))
   return x86_emulate_instruction(vcpu, gpa, 0, NULL, 0) ==
 EMULATE_DONE;
 - if (unlikely(!ret))
 + if (unlikely(ret == RET_MMIO_PF_RETRY))
   return 1;
  
   /* It is the real ept misconfig */
 -- 
 1.8.1.4

--
Gleb.
--
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 v3 3/6] KVM: MMU: make return value of mmio page fault handler more readable

2013-06-10 Thread Xiao Guangrong 
On 06/10/2013 03:57 PM, Gleb Natapov wrote:
 On Fri, Jun 07, 2013 at 04:51:25PM +0800, Xiao Guangrong wrote:
 Define some meaningful names instead of raw code

 Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
 ---
  arch/x86/kvm/mmu.c | 15 +--
  arch/x86/kvm/mmu.h | 14 ++
  arch/x86/kvm/vmx.c |  4 ++--
  3 files changed, 21 insertions(+), 12 deletions(-)

 diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
 index eca91bd..044d8c0 100644
 --- a/arch/x86/kvm/mmu.c
 +++ b/arch/x86/kvm/mmu.c
 @@ -3222,17 +3222,12 @@ static u64 walk_shadow_page_get_mmio_spte(struct 
 kvm_vcpu *vcpu, u64 addr)
  return spte;
  }
  
 -/*
 - * If it is a real mmio page fault, return 1 and emulat the instruction
 - * directly, return 0 to let CPU fault again on the address, -1 is
 - * returned if bug is detected.
 - */
  int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool 
 direct)
  {
  u64 spte;
  
  if (quickly_check_mmio_pf(vcpu, addr, direct))
 -return 1;
 +return RET_MMIO_PF_EMULATE;
  
  spte = walk_shadow_page_get_mmio_spte(vcpu, addr);
  
 @@ -3245,7 +3240,7 @@ int handle_mmio_page_fault_common(struct kvm_vcpu 
 *vcpu, u64 addr, bool direct)
  
  trace_handle_mmio_page_fault(addr, gfn, access);
  vcpu_cache_mmio_info(vcpu, addr, gfn, access);
 -return 1;
 +return RET_MMIO_PF_EMULATE;
  }
  
  /*
 @@ -3253,13 +3248,13 @@ int handle_mmio_page_fault_common(struct kvm_vcpu 
 *vcpu, u64 addr, bool direct)
   * it's a BUG if the gfn is not a mmio page.
   */
  if (direct  !check_direct_spte_mmio_pf(spte))
 -return -1;
 +return RET_MMIO_PF_BUG;
  
  /*
   * If the page table is zapped by other cpus, let CPU fault again on
   * the address.
   */
 -return 0;
 +return RET_MMIO_PF_RETRY;
  }
  EXPORT_SYMBOL_GPL(handle_mmio_page_fault_common);
  
 @@ -3269,7 +3264,7 @@ static int handle_mmio_page_fault(struct kvm_vcpu 
 *vcpu, u64 addr,
  int ret;
  
  ret = handle_mmio_page_fault_common(vcpu, addr, direct);
 -WARN_ON(ret  0);
 +WARN_ON(ret == RET_MMIO_PF_BUG);
  return ret;
  }
  
 diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
 index 922bfae..ba6a19c 100644
 --- a/arch/x86/kvm/mmu.h
 +++ b/arch/x86/kvm/mmu.h
 @@ -52,6 +52,20 @@
  
  int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 
 sptes[4]);
  void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask);
 +
 +/*
 + * Return values of handle_mmio_page_fault_common:
 + * RET_MMIO_PF_EMULATE: it is a real mmio page fault, emulate the 
 instruction
 + *   directly.
 + * RET_MMIO_PF_RETRY: let CPU fault again on the address.
 + * RET_MMIO_PF_BUG: bug is detected.
 + */
 +enum {
 +RET_MMIO_PF_EMULATE = 1,
 +RET_MMIO_PF_RETRY = 0,
 +RET_MMIO_PF_BUG = -1
 +};
 I would order them from -1 to 1 and rename RET_MMIO_PF_BUG to
 RET_MMIO_PF_ERROR, but no need to resend just for that.

Fine to me. Will make a separate patch to update it later. Thanks for your 
review!
--
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 v3 3/6] KVM: MMU: make return value of mmio page fault handler more readable

2013-06-10 Thread Takuya Yoshikawa 
On Mon, 10 Jun 2013 10:57:50 +0300
Gleb Natapov g...@redhat.com wrote:

 On Fri, Jun 07, 2013 at 04:51:25PM +0800, Xiao Guangrong wrote:

  +
  +/*
  + * Return values of handle_mmio_page_fault_common:
  + * RET_MMIO_PF_EMULATE: it is a real mmio page fault, emulate the 
  instruction
  + *  directly.
  + * RET_MMIO_PF_RETRY: let CPU fault again on the address.
  + * RET_MMIO_PF_BUG: bug is detected.
  + */
  +enum {
  +   RET_MMIO_PF_EMULATE = 1,
  +   RET_MMIO_PF_RETRY = 0,
  +   RET_MMIO_PF_BUG = -1
  +};
 I would order them from -1 to 1 and rename RET_MMIO_PF_BUG to
 RET_MMIO_PF_ERROR, but no need to resend just for that.

Why not just let compilers select the values? -- It's an enum.
Any reason to make it start from -1?

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

[PATCH v3 3/6] KVM: MMU: make return value of mmio page fault handler more readable

2013-06-07 Thread Xiao Guangrong 
Define some meaningful names instead of raw code

Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
 arch/x86/kvm/mmu.c | 15 +--
 arch/x86/kvm/mmu.h | 14 ++
 arch/x86/kvm/vmx.c |  4 ++--
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index eca91bd..044d8c0 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3222,17 +3222,12 @@ static u64 walk_shadow_page_get_mmio_spte(struct 
kvm_vcpu *vcpu, u64 addr)
return spte;
 }
 
-/*
- * If it is a real mmio page fault, return 1 and emulat the instruction
- * directly, return 0 to let CPU fault again on the address, -1 is
- * returned if bug is detected.
- */
 int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool direct)
 {
u64 spte;
 
if (quickly_check_mmio_pf(vcpu, addr, direct))
-   return 1;
+   return RET_MMIO_PF_EMULATE;
 
spte = walk_shadow_page_get_mmio_spte(vcpu, addr);
 
@@ -3245,7 +3240,7 @@ int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, 
u64 addr, bool direct)
 
trace_handle_mmio_page_fault(addr, gfn, access);
vcpu_cache_mmio_info(vcpu, addr, gfn, access);
-   return 1;
+   return RET_MMIO_PF_EMULATE;
}
 
/*
@@ -3253,13 +3248,13 @@ int handle_mmio_page_fault_common(struct kvm_vcpu 
*vcpu, u64 addr, bool direct)
 * it's a BUG if the gfn is not a mmio page.
 */
if (direct  !check_direct_spte_mmio_pf(spte))
-   return -1;
+   return RET_MMIO_PF_BUG;
 
/*
 * If the page table is zapped by other cpus, let CPU fault again on
 * the address.
 */
-   return 0;
+   return RET_MMIO_PF_RETRY;
 }
 EXPORT_SYMBOL_GPL(handle_mmio_page_fault_common);
 
@@ -3269,7 +3264,7 @@ static int handle_mmio_page_fault(struct kvm_vcpu *vcpu, 
u64 addr,
int ret;
 
ret = handle_mmio_page_fault_common(vcpu, addr, direct);
-   WARN_ON(ret  0);
+   WARN_ON(ret == RET_MMIO_PF_BUG);
return ret;
 }
 
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 922bfae..ba6a19c 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -52,6 +52,20 @@
 
 int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 sptes[4]);
 void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask);
+
+/*
+ * Return values of handle_mmio_page_fault_common:
+ * RET_MMIO_PF_EMULATE: it is a real mmio page fault, emulate the instruction
+ *  directly.
+ * RET_MMIO_PF_RETRY: let CPU fault again on the address.
+ * RET_MMIO_PF_BUG: bug is detected.
+ */
+enum {
+   RET_MMIO_PF_EMULATE = 1,
+   RET_MMIO_PF_RETRY = 0,
+   RET_MMIO_PF_BUG = -1
+};
+
 int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool 
direct);
 int kvm_init_shadow_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *context);
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 78ee123..85c8d51 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5366,10 +5366,10 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
 
ret = handle_mmio_page_fault_common(vcpu, gpa, true);
-   if (likely(ret == 1))
+   if (likely(ret == RET_MMIO_PF_EMULATE))
return x86_emulate_instruction(vcpu, gpa, 0, NULL, 0) ==
  EMULATE_DONE;
-   if (unlikely(!ret))
+   if (unlikely(ret == RET_MMIO_PF_RETRY))
return 1;
 
/* It is the real ept misconfig */
-- 
1.8.1.4

--
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