On 03/11/2015 06:49, 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 PML after above commit vmx_cpuid_update calls
> vmx_secondary_exec_control, in which PML is disabled 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 think 
> PML
> is used. Actually looks calling vmx_secondary_exec_control in vmx_cpuid_update
> is likely to break any VMX features that is enabled/disabled on demand by
> updating SECONDARY_VM_EXEC_CONTROL, if vmx_cpuid_update is called between the
> feature is enabled and disabled.
> 
> Fix this by calling vmcs_read32 to read out SECONDARY_VM_EXEC_CONTROL 
> directly.

vmx_cpuid_update() is meant to be mostly idempotent; the parts that
depend on the current VMCS configuration are hidden in
vmcs_set_secondary_control.  So a better fix would be to add
SECONDARY_EXEC_ENABLE_PML to vmcs_set_secondary_exec_control's
"mask" variable.  However, you can see from the comment:

        /*
         * These bits in the secondary execution controls field
         * are dynamic, the others are mostly based on the hypervisor
         * architecture and the guest's CPUID. Do not touch the
         * dynamic bits.
         */

that even this is not the optimal fix.  SECONDARY_EXEC_ENABLE_PML is
either always set or always clear, so it shouldn't be in "mask".

Instead, it should be in vmcs_config.cpu_based_2nd_exec_ctrl.  It isn't
because my review didn't notice this remnant of your original
implementation, which dynamically enabled/disabled PML.

In fact, cpu_has_vmx_pml() expects SECONDARY_EXEC_ENABLE_PML to be set
in vmcs_config.cpu_based_2nd_exec_ctrl, so it is a bit confusing to
remove the bit unconditionally in vmx_secondary_exec_control!

So I think SECONDARY_EXEC_ENABLE_PML should not be removed unconditionally
from exec_control in vmx_secondary_exec_control; the removal should be
conditional on !enable_pml, like we do for e.g. EPT or VPID.  If you do this,
vmx_enable_pml and vmx_disable_pml need not touch SECONDARY_VM_EXEC_CONTROL
anymore.  Do you agree?  If so, can you prepare a patch along these lines?

(Since you are at it, perhaps you can rename vmx_enable_pml and
vmx_disable_pml, since they will only allocate and free the PML page).

Thanks for reporting the issue!

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

Reply via email to