On Fri, Sep 14, 2018 at 07:46:48PM +0200, Paolo Bonzini wrote:
> On 28/08/2018 00:21, Sean Christopherson wrote:
> > Provide a singular location where the VMX preemption timer bit is
> > set/cleared so that future usages of the preemption timer can ensure
> > the VMCS bit is up-to-date without having to modify unrelated code
> > paths. For example, the preemption timer can be used to force an
> > immediate VMExit. Cache the status of the timer to avoid redundant
> > VMREAD and VMWRITE, e.g. if the timer stays armed across multiple
> > VMEnters/VMExits.
>
> It's mostly aesthetic, but I slightly prefer this:
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 8f8aa6391a47..7a66a7e817af 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -10596,10 +10596,9 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
> msrs[i].host, false);
> }
>
> -static void vmx_arm_hv_timer(struct kvm_vcpu *vcpu)
> +static void vmx_update_hv_timer(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> - bool arm_timer;
> u64 tscl;
> u32 delta_tsc;
>
> @@ -10613,16 +10612,16 @@ static void vmx_arm_hv_timer(struct kvm_vcpu *vcpu)
> delta_tsc = 0;
>
> vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, delta_tsc);
> + if (!vmx->hv_timer_armed)
> + vmcs_set_bits(PIN_BASED_VM_EXEC_CONTROL,
> + PIN_BASED_VMX_PREEMPTION_TIMER);
> + vmx->hv_timer_armed = true;
> + } else {
> + if (vmx->hv_timer_armed)
> + vmcs_clear_bits(PIN_BASED_VM_EXEC_CONTROL,
> + PIN_BASED_VMX_PREEMPTION_TIMER);
> + vmx->hv_timer_armed = false;
> }
> -
> - arm_timer = (vmx->hv_deadline_tsc != -1);
> - if (arm_timer && !vmx->hv_timer_armed)
> - vmcs_set_bits(PIN_BASED_VM_EXEC_CONTROL,
> - PIN_BASED_VMX_PREEMPTION_TIMER);
> - else if (!arm_timer && vmx->hv_timer_armed)
> - vmcs_clear_bits(PIN_BASED_VM_EXEC_CONTROL,
> - PIN_BASED_VMX_PREEMPTION_TIMER);
> - vmx->hv_timer_armed = arm_timer;
> }
>
> static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -10682,7 +10681,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>
> atomic_switch_perf_msrs(vmx);
>
> - vmx_arm_hv_timer(vcpu);
> + vmx_update_hv_timer(vcpu);
>
> /*
> * If this vCPU has touched SPEC_CTRL, restore the guest's value if
>
> The generated code is probably pretty much the same, since the compiler
> can figure out most of this.
Works for me, I all but literally flipped a coin when choosing between
the two approaches. Do you want me to send a v2 with this change and
hv_timer_armed moved to loaded_vmcs?
