Re: [v2] KVM: VMX: Fix commit which broke PML

2015-11-05 Thread Paolo Bonzini 


On 05/11/2015 03:04, Kai Huang wrote:
> 
> Thanks for applying! I am really sorry that I forgot to delete the line
> that clears SECONDARY_EXEC_ENABLE_PML bit in vmx_disable_pml, which is
> renamed to vmx_destroy_pml_buffer now.
> It won't impact functionality but to make the function consistent, would
> you also do below? Sorry for such negligence!
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 89f4fa2..ef4ca76 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -7826,8 +7826,6 @@ static void vmx_destroy_pml_buffer(struct vcpu_vmx
> *vmx)
> ASSERT(vmx->pml_pg);
> __free_page(vmx->pml_pg);
> vmx->pml_pg = NULL;
> -
> -   vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> SECONDARY_EXEC_ENABLE_PML);
>  }

No problem.  I haven't yet pushed to kvm/next, so I can change this commit.

Thanks for the quick response.

Paolo
--
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: [v2] KVM: VMX: Fix commit which broke PML

2015-11-04 Thread Kai Huang 

Hi Paolo,

Thanks for applying! I am really sorry that I forgot to delete the line 
that clears SECONDARY_EXEC_ENABLE_PML bit in vmx_disable_pml, which is 
renamed to vmx_destroy_pml_buffer now.
It won't impact functionality but to make the function consistent, would 
you also do below? Sorry for such negligence!


diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 89f4fa2..ef4ca76 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -7826,8 +7826,6 @@ static void vmx_destroy_pml_buffer(struct vcpu_vmx 
*vmx)

ASSERT(vmx->pml_pg);
__free_page(vmx->pml_pg);
vmx->pml_pg = NULL;
-
-   vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL, 
SECONDARY_EXEC_ENABLE_PML);

 }

Thanks,
-Kai

On 11/04/2015 08:00 PM, Paolo Bonzini wrote:


On 04/11/2015 06:46, Kai Huang wrote:

I found PML was broken since below commit:

commit feda805fe7c4ed9cf78158e73b1218752e3b4314
Author: Xiao Guangrong 
Date:   Wed Sep 9 14:05:55 2015 +0800

KVM: VMX: unify SECONDARY_VM_EXEC_CONTROL update

Unify the update in vmx_cpuid_update()

Signed-off-by: Xiao Guangrong 
[Rewrite to use vmcs_set_secondary_exec_control. - Paolo]
Signed-off-by: Paolo Bonzini 

The reason is in above commit vmx_cpuid_update calls vmx_secondary_exec_control,
in which currently SECONDARY_EXEC_ENABLE_PML bit is cleared unconditionally (as
PML is enabled in creating vcpu). Therefore if vcpu_cpuid_update is called after
vcpu is created, PML will be disabled unexpectedly while log-dirty code still
thinks PML is used.

Fix this by clearing SECONDARY_EXEC_ENABLE_PML in vmx_secondary_exec_control
only when PML is not supported or not enabled (!enable_pml). This is more
reasonable as PML is currently either always enabled or disabled. With this
explicit updating SECONDARY_EXEC_ENABLE_PML in vmx_enable{disable}_pml is not
needed so also rename vmx_enable{disable}_pml to vmx_create{destroy}_pml_buffer.

Signed-off-by: Kai Huang 

---

v1->v2: Fix this by following Paolo's suggestion. It's better to not to clear
SECONDARY_EXEC_ENABLE_PML in vmx_secondary_exec_control unconditionally but only
clear it when PML is not supported or enabled.

---
  arch/x86/kvm/vmx.c | 15 +++
  1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2ac11641..89f4fa2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4718,8 +4718,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx 
*vmx)
   a current VMCS12
*/
exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
-   /* PML is enabled/disabled in creating/destorying vcpu */
-   exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+
+   if (!enable_pml)
+   exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
  
  	/* Currently, we allow L1 guest to directly run pcommit instruction. */

exec_control &= ~SECONDARY_EXEC_PCOMMIT;
@@ -7804,7 +7805,7 @@ static void vmx_get_exit_info(struct kvm_vcpu *vcpu, u64 
*info1, u64 *info2)
*info2 = vmcs_read32(VM_EXIT_INTR_INFO);
  }
  
-static int vmx_enable_pml(struct vcpu_vmx *vmx)

+static int vmx_create_pml_buffer(struct vcpu_vmx *vmx)
  {
struct page *pml_pg;
  
@@ -7817,12 +7818,10 @@ static int vmx_enable_pml(struct vcpu_vmx *vmx)

vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
  
-	vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL, SECONDARY_EXEC_ENABLE_PML);

-
return 0;
  }
  
-static void vmx_disable_pml(struct vcpu_vmx *vmx)

+static void vmx_destroy_pml_buffer(struct vcpu_vmx *vmx)
  {
ASSERT(vmx->pml_pg);
__free_page(vmx->pml_pg);
@@ -8706,7 +8705,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
struct vcpu_vmx *vmx = to_vmx(vcpu);
  
  	if (enable_pml)

-   vmx_disable_pml(vmx);
+   vmx_destroy_pml_buffer(vmx);
free_vpid(vmx->vpid);
leave_guest_mode(vcpu);
vmx_load_vmcs01(vcpu);
@@ -8790,7 +8789,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, 
unsigned int id)
 * for the guest, etc.
 */
if (enable_pml) {
-   err = vmx_enable_pml(vmx);
+   err = vmx_create_pml_buffer(vmx);
if (err)
goto free_vmcs;
}



Applied, thanks!

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



--
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: [v2] KVM: VMX: Fix commit which broke PML

2015-11-04 Thread Paolo Bonzini 


On 04/11/2015 06:46, Kai Huang wrote:
> I found PML was broken since below commit:
> 
>   commit feda805fe7c4ed9cf78158e73b1218752e3b4314
>   Author: Xiao Guangrong 
>   Date:   Wed Sep 9 14:05:55 2015 +0800
> 
>   KVM: VMX: unify SECONDARY_VM_EXEC_CONTROL update
> 
>   Unify the update in vmx_cpuid_update()
> 
>   Signed-off-by: Xiao Guangrong 
>   [Rewrite to use vmcs_set_secondary_exec_control. - Paolo]
>   Signed-off-by: Paolo Bonzini 
> 
> The reason is in above commit vmx_cpuid_update calls 
> vmx_secondary_exec_control,
> in which currently SECONDARY_EXEC_ENABLE_PML bit is cleared unconditionally 
> (as
> PML is enabled in creating vcpu). Therefore if vcpu_cpuid_update is called 
> after
> vcpu is created, PML will be disabled unexpectedly while log-dirty code still
> thinks PML is used.
> 
> Fix this by clearing SECONDARY_EXEC_ENABLE_PML in vmx_secondary_exec_control
> only when PML is not supported or not enabled (!enable_pml). This is more
> reasonable as PML is currently either always enabled or disabled. With this
> explicit updating SECONDARY_EXEC_ENABLE_PML in vmx_enable{disable}_pml is not
> needed so also rename vmx_enable{disable}_pml to 
> vmx_create{destroy}_pml_buffer.
> 
> Signed-off-by: Kai Huang 
> 
> ---
> 
> v1->v2: Fix this by following Paolo's suggestion. It's better to not to clear
> SECONDARY_EXEC_ENABLE_PML in vmx_secondary_exec_control unconditionally but 
> only
> clear it when PML is not supported or enabled.
> 
> ---
>  arch/x86/kvm/vmx.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 2ac11641..89f4fa2 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4718,8 +4718,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx 
> *vmx)
>  a current VMCS12
>   */
>   exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
> - /* PML is enabled/disabled in creating/destorying vcpu */
> - exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
> +
> + if (!enable_pml)
> + exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>  
>   /* Currently, we allow L1 guest to directly run pcommit instruction. */
>   exec_control &= ~SECONDARY_EXEC_PCOMMIT;
> @@ -7804,7 +7805,7 @@ static void vmx_get_exit_info(struct kvm_vcpu *vcpu, 
> u64 *info1, u64 *info2)
>   *info2 = vmcs_read32(VM_EXIT_INTR_INFO);
>  }
>  
> -static int vmx_enable_pml(struct vcpu_vmx *vmx)
> +static int vmx_create_pml_buffer(struct vcpu_vmx *vmx)
>  {
>   struct page *pml_pg;
>  
> @@ -7817,12 +7818,10 @@ static int vmx_enable_pml(struct vcpu_vmx *vmx)
>   vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
>   vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
>  
> - vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL, SECONDARY_EXEC_ENABLE_PML);
> -
>   return 0;
>  }
>  
> -static void vmx_disable_pml(struct vcpu_vmx *vmx)
> +static void vmx_destroy_pml_buffer(struct vcpu_vmx *vmx)
>  {
>   ASSERT(vmx->pml_pg);
>   __free_page(vmx->pml_pg);
> @@ -8706,7 +8705,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
>   struct vcpu_vmx *vmx = to_vmx(vcpu);
>  
>   if (enable_pml)
> - vmx_disable_pml(vmx);
> + vmx_destroy_pml_buffer(vmx);
>   free_vpid(vmx->vpid);
>   leave_guest_mode(vcpu);
>   vmx_load_vmcs01(vcpu);
> @@ -8790,7 +8789,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm 
> *kvm, unsigned int id)
>* for the guest, etc.
>*/
>   if (enable_pml) {
> - err = vmx_enable_pml(vmx);
> + err = vmx_create_pml_buffer(vmx);
>   if (err)
>   goto free_vmcs;
>   }
> 


Applied, thanks!

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