Re: [PATCH v5 3/7] kvm: Make init_rmode_identity_map() return 0 on success.

2014-09-11 Thread tangchen 


On 09/11/2014 05:17 PM, Paolo Bonzini wrote:

..
@@ -7645,7 +7642,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, 
unsigned int id)
kvm->arch.ept_identity_map_addr =
VMX_EPT_IDENTITY_PAGETABLE_ADDR;
err = -ENOMEM;
-   if (!init_rmode_identity_map(kvm))
+   if (init_rmode_identity_map(kvm))
Please add "< 0" here.  I would also consider setting err to the return
value of init_rmode_identity_map, and initializing it to -ENOMEM only
after the "if".


I'd like to move err = -ENOMEM to the following place:

vmx_create_vcpu()
{
..
err = kvm_vcpu_init(>vcpu, kvm, id);
if (err)
goto free_vcpu;

err = -ENOMEM;  -- move it here

vmx->guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL);

vmx->loaded_vmcs->vmcs = alloc_vmcs();

}

So that it can be used to handle the next two memory allocation error.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/7] kvm: Make init_rmode_identity_map() return 0 on success.

2014-09-11 Thread Paolo Bonzini 
Il 11/09/2014 07:38, Tang Chen ha scritto:
> In init_rmode_identity_map(), there two variables indicating the return
> value, r and ret, and it return 0 on error, 1 on success. The function
> is only called by vmx_create_vcpu(), and r is redundant.
> 
> This patch removes the redundant variable r, and make 
> init_rmode_identity_map()
> return 0 on success, -errno on failure.
> 
> Signed-off-by: Tang Chen 
> ---
>  arch/x86/kvm/vmx.c | 25 +++--
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 953d529..63c4c3e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3939,45 +3939,42 @@ out:
>  
>  static int init_rmode_identity_map(struct kvm *kvm)
>  {
> - int i, idx, r, ret = 0;
> + int i, idx, ret = 0;
>   pfn_t identity_map_pfn;
>   u32 tmp;
>  
>   if (!enable_ept)
> - return 1;
> + return 0;
>  
>   /* Protect kvm->arch.ept_identity_pagetable_done. */
>   mutex_lock(>slots_lock);
>  
> - if (likely(kvm->arch.ept_identity_pagetable_done)) {
> - ret = 1;
> + if (likely(kvm->arch.ept_identity_pagetable_done))
>   goto out2;
> - }
>  
>   identity_map_pfn = kvm->arch.ept_identity_map_addr >> PAGE_SHIFT;
>  
> - r = alloc_identity_pagetable(kvm);
> - if (r)
> + ret = alloc_identity_pagetable(kvm);
> + if (ret)
>   goto out2;
>  
>   idx = srcu_read_lock(>srcu);
> - r = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
> - if (r < 0)
> + ret = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
> + if (ret)
>   goto out;
>   /* Set up identity-mapping pagetable for EPT in real mode */
>   for (i = 0; i < PT32_ENT_PER_PAGE; i++) {
>   tmp = (i << 22) + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
>   _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
> - r = kvm_write_guest_page(kvm, identity_map_pfn,
> + ret = kvm_write_guest_page(kvm, identity_map_pfn,
>   , i * sizeof(tmp), sizeof(tmp));
> - if (r < 0)
> + if (ret)
>   goto out;
>   }
>   kvm->arch.ept_identity_pagetable_done = true;
> - ret = 1;
> +
>  out:
>   srcu_read_unlock(>srcu, idx);
> -
>  out2:
>   mutex_unlock(>slots_lock);
>   return ret;
> @@ -7645,7 +7642,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm 
> *kvm, unsigned int id)
>   kvm->arch.ept_identity_map_addr =
>   VMX_EPT_IDENTITY_PAGETABLE_ADDR;
>   err = -ENOMEM;
> - if (!init_rmode_identity_map(kvm))
> + if (init_rmode_identity_map(kvm))

Please add "< 0" here.  I would also consider setting err to the return
value of init_rmode_identity_map, and initializing it to -ENOMEM only
after the "if".

Paolo

>   goto free_vmcs;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/7] kvm: Make init_rmode_identity_map() return 0 on success.

2014-09-11 Thread Paolo Bonzini 
Il 11/09/2014 07:38, Tang Chen ha scritto:
 In init_rmode_identity_map(), there two variables indicating the return
 value, r and ret, and it return 0 on error, 1 on success. The function
 is only called by vmx_create_vcpu(), and r is redundant.
 
 This patch removes the redundant variable r, and make 
 init_rmode_identity_map()
 return 0 on success, -errno on failure.
 
 Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
 ---
  arch/x86/kvm/vmx.c | 25 +++--
  1 file changed, 11 insertions(+), 14 deletions(-)
 
 diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
 index 953d529..63c4c3e 100644
 --- a/arch/x86/kvm/vmx.c
 +++ b/arch/x86/kvm/vmx.c
 @@ -3939,45 +3939,42 @@ out:
  
  static int init_rmode_identity_map(struct kvm *kvm)
  {
 - int i, idx, r, ret = 0;
 + int i, idx, ret = 0;
   pfn_t identity_map_pfn;
   u32 tmp;
  
   if (!enable_ept)
 - return 1;
 + return 0;
  
   /* Protect kvm-arch.ept_identity_pagetable_done. */
   mutex_lock(kvm-slots_lock);
  
 - if (likely(kvm-arch.ept_identity_pagetable_done)) {
 - ret = 1;
 + if (likely(kvm-arch.ept_identity_pagetable_done))
   goto out2;
 - }
  
   identity_map_pfn = kvm-arch.ept_identity_map_addr  PAGE_SHIFT;
  
 - r = alloc_identity_pagetable(kvm);
 - if (r)
 + ret = alloc_identity_pagetable(kvm);
 + if (ret)
   goto out2;
  
   idx = srcu_read_lock(kvm-srcu);
 - r = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
 - if (r  0)
 + ret = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
 + if (ret)
   goto out;
   /* Set up identity-mapping pagetable for EPT in real mode */
   for (i = 0; i  PT32_ENT_PER_PAGE; i++) {
   tmp = (i  22) + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
   _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
 - r = kvm_write_guest_page(kvm, identity_map_pfn,
 + ret = kvm_write_guest_page(kvm, identity_map_pfn,
   tmp, i * sizeof(tmp), sizeof(tmp));
 - if (r  0)
 + if (ret)
   goto out;
   }
   kvm-arch.ept_identity_pagetable_done = true;
 - ret = 1;
 +
  out:
   srcu_read_unlock(kvm-srcu, idx);
 -
  out2:
   mutex_unlock(kvm-slots_lock);
   return ret;
 @@ -7645,7 +7642,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm 
 *kvm, unsigned int id)
   kvm-arch.ept_identity_map_addr =
   VMX_EPT_IDENTITY_PAGETABLE_ADDR;
   err = -ENOMEM;
 - if (!init_rmode_identity_map(kvm))
 + if (init_rmode_identity_map(kvm))

Please add  0 here.  I would also consider setting err to the return
value of init_rmode_identity_map, and initializing it to -ENOMEM only
after the if.

Paolo

   goto free_vmcs;

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/7] kvm: Make init_rmode_identity_map() return 0 on success.

2014-09-11 Thread tangchen 


On 09/11/2014 05:17 PM, Paolo Bonzini wrote:

..
@@ -7645,7 +7642,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, 
unsigned int id)
kvm-arch.ept_identity_map_addr =
VMX_EPT_IDENTITY_PAGETABLE_ADDR;
err = -ENOMEM;
-   if (!init_rmode_identity_map(kvm))
+   if (init_rmode_identity_map(kvm))
Please add  0 here.  I would also consider setting err to the return
value of init_rmode_identity_map, and initializing it to -ENOMEM only
after the if.


I'd like to move err = -ENOMEM to the following place:

vmx_create_vcpu()
{
..
err = kvm_vcpu_init(vmx-vcpu, kvm, id);
if (err)
goto free_vcpu;

err = -ENOMEM;  -- move it here

vmx-guest_msrs = kmalloc(PAGE_SIZE, GFP_KERNEL);

vmx-loaded_vmcs-vmcs = alloc_vmcs();

}

So that it can be used to handle the next two memory allocation error.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 3/7] kvm: Make init_rmode_identity_map() return 0 on success.

2014-09-10 Thread Tang Chen 
In init_rmode_identity_map(), there two variables indicating the return
value, r and ret, and it return 0 on error, 1 on success. The function
is only called by vmx_create_vcpu(), and r is redundant.

This patch removes the redundant variable r, and make init_rmode_identity_map()
return 0 on success, -errno on failure.

Signed-off-by: Tang Chen 
---
 arch/x86/kvm/vmx.c | 25 +++--
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 953d529..63c4c3e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3939,45 +3939,42 @@ out:
 
 static int init_rmode_identity_map(struct kvm *kvm)
 {
-   int i, idx, r, ret = 0;
+   int i, idx, ret = 0;
pfn_t identity_map_pfn;
u32 tmp;
 
if (!enable_ept)
-   return 1;
+   return 0;
 
/* Protect kvm->arch.ept_identity_pagetable_done. */
mutex_lock(>slots_lock);
 
-   if (likely(kvm->arch.ept_identity_pagetable_done)) {
-   ret = 1;
+   if (likely(kvm->arch.ept_identity_pagetable_done))
goto out2;
-   }
 
identity_map_pfn = kvm->arch.ept_identity_map_addr >> PAGE_SHIFT;
 
-   r = alloc_identity_pagetable(kvm);
-   if (r)
+   ret = alloc_identity_pagetable(kvm);
+   if (ret)
goto out2;
 
idx = srcu_read_lock(>srcu);
-   r = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
-   if (r < 0)
+   ret = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
+   if (ret)
goto out;
/* Set up identity-mapping pagetable for EPT in real mode */
for (i = 0; i < PT32_ENT_PER_PAGE; i++) {
tmp = (i << 22) + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
-   r = kvm_write_guest_page(kvm, identity_map_pfn,
+   ret = kvm_write_guest_page(kvm, identity_map_pfn,
, i * sizeof(tmp), sizeof(tmp));
-   if (r < 0)
+   if (ret)
goto out;
}
kvm->arch.ept_identity_pagetable_done = true;
-   ret = 1;
+
 out:
srcu_read_unlock(>srcu, idx);
-
 out2:
mutex_unlock(>slots_lock);
return ret;
@@ -7645,7 +7642,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, 
unsigned int id)
kvm->arch.ept_identity_map_addr =
VMX_EPT_IDENTITY_PAGETABLE_ADDR;
err = -ENOMEM;
-   if (!init_rmode_identity_map(kvm))
+   if (init_rmode_identity_map(kvm))
goto free_vmcs;
}
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v5 3/7] kvm: Make init_rmode_identity_map() return 0 on success.

2014-09-10 Thread Tang Chen 
In init_rmode_identity_map(), there two variables indicating the return
value, r and ret, and it return 0 on error, 1 on success. The function
is only called by vmx_create_vcpu(), and r is redundant.

This patch removes the redundant variable r, and make init_rmode_identity_map()
return 0 on success, -errno on failure.

Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
---
 arch/x86/kvm/vmx.c | 25 +++--
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 953d529..63c4c3e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3939,45 +3939,42 @@ out:
 
 static int init_rmode_identity_map(struct kvm *kvm)
 {
-   int i, idx, r, ret = 0;
+   int i, idx, ret = 0;
pfn_t identity_map_pfn;
u32 tmp;
 
if (!enable_ept)
-   return 1;
+   return 0;
 
/* Protect kvm-arch.ept_identity_pagetable_done. */
mutex_lock(kvm-slots_lock);
 
-   if (likely(kvm-arch.ept_identity_pagetable_done)) {
-   ret = 1;
+   if (likely(kvm-arch.ept_identity_pagetable_done))
goto out2;
-   }
 
identity_map_pfn = kvm-arch.ept_identity_map_addr  PAGE_SHIFT;
 
-   r = alloc_identity_pagetable(kvm);
-   if (r)
+   ret = alloc_identity_pagetable(kvm);
+   if (ret)
goto out2;
 
idx = srcu_read_lock(kvm-srcu);
-   r = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
-   if (r  0)
+   ret = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
+   if (ret)
goto out;
/* Set up identity-mapping pagetable for EPT in real mode */
for (i = 0; i  PT32_ENT_PER_PAGE; i++) {
tmp = (i  22) + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
-   r = kvm_write_guest_page(kvm, identity_map_pfn,
+   ret = kvm_write_guest_page(kvm, identity_map_pfn,
tmp, i * sizeof(tmp), sizeof(tmp));
-   if (r  0)
+   if (ret)
goto out;
}
kvm-arch.ept_identity_pagetable_done = true;
-   ret = 1;
+
 out:
srcu_read_unlock(kvm-srcu, idx);
-
 out2:
mutex_unlock(kvm-slots_lock);
return ret;
@@ -7645,7 +7642,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, 
unsigned int id)
kvm-arch.ept_identity_map_addr =
VMX_EPT_IDENTITY_PAGETABLE_ADDR;
err = -ENOMEM;
-   if (!init_rmode_identity_map(kvm))
+   if (init_rmode_identity_map(kvm))
goto free_vmcs;
}
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/