On 16/04/19 22:32, Sean Christopherson wrote:
> Refactor kvm_x86_ops->set_hv_timer to use an explicit parameter for
> stating that the timer has expired. Overloading the return value is
> unnecessarily clever, e.g. can lead to confusion over the proper return
> value from start_hv_timer() when r==1.
I guess it's also in the eye of the beholder; I don't like out
parameters very much in general but this time I think I don't disagree
enough to argue. ;)
Paolo
> Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> Cc: Liran Alon <liran.alon@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 3 ++-
> arch/x86/kvm/lapic.c | 10 +++++-----
> arch/x86/kvm/vmx/vmx.c | 6 ++++--
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a9d03af34030..501a8209285b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1167,7 +1167,8 @@ struct kvm_x86_ops {
> uint32_t guest_irq, bool set);
> void (*apicv_post_state_restore)(struct kvm_vcpu *vcpu);
>
> - int (*set_hv_timer)(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc);
> + int (*set_hv_timer)(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
> + bool *expired);
> void (*cancel_hv_timer)(struct kvm_vcpu *vcpu);
>
> void (*setup_mce)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index bdcf04689a80..9a09a41000c3 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1681,7 +1681,8 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
> static bool start_hv_timer(struct kvm_lapic *apic)
> {
> struct kvm_timer *ktimer = &apic->lapic_timer;
> - int r;
> + struct kvm_vcpu *vcpu = apic->vcpu;
> + bool expired;
>
> WARN_ON(preemptible());
> if (!kvm_x86_ops->set_hv_timer)
> @@ -1693,8 +1694,7 @@ static bool start_hv_timer(struct kvm_lapic *apic)
> if (!ktimer->tscdeadline)
> return false;
>
> - r = kvm_x86_ops->set_hv_timer(apic->vcpu, ktimer->tscdeadline);
> - if (r < 0)
> + if (kvm_x86_ops->set_hv_timer(vcpu, ktimer->tscdeadline, &expired))
> return false;
>
> ktimer->hv_timer_in_use = true;
> @@ -1712,13 +1712,13 @@ static bool start_hv_timer(struct kvm_lapic *apic)
> */
> if (atomic_read(&ktimer->pending)) {
> cancel_hv_timer(apic);
> - } else if (r) {
> + } else if (expired) {
> apic_timer_expired(apic);
> cancel_hv_timer(apic);
> }
> }
>
> - trace_kvm_hv_timer_state(apic->vcpu->vcpu_id, ktimer->hv_timer_in_use);
> + trace_kvm_hv_timer_state(vcpu->vcpu_id, ktimer->hv_timer_in_use);
>
> return true;
> }
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index a3211e3ee679..f3c326e3c324 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7027,7 +7027,8 @@ static inline int u64_shl_div_u64(u64 a, unsigned int shift,
> return 0;
> }
>
> -static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc)
> +static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
> + bool *expired)
> {
> struct vcpu_vmx *vmx;
> u64 tscl, guest_tscl, delta_tsc, lapic_timer_advance_cycles;
> @@ -7066,7 +7067,8 @@ static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc)
> return -ERANGE;
>
> vmx->hv_deadline_tsc = tscl + delta_tsc;
> - return delta_tsc == 0;
> + *expired = !delta_tsc;
> + return 0;
> }
>
> static void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu)
>
