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)
        vmx->pml_pg = NULL;


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 <guangrong.x...@linux.intel.com>
        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 <guangrong.x...@linux.intel.com>
        [Rewrite to use vmcs_set_secondary_exec_control. - Paolo]
        Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>

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 <kai.hu...@linux.intel.com>


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 
           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);
        return 0;
-static void vmx_disable_pml(struct vcpu_vmx *vmx)
+static void vmx_destroy_pml_buffer(struct vcpu_vmx *vmx)
@@ -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);
@@ -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!

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

Reply via email to