| Message ID | 20190416203248.29429-8-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: lapic: Fix a variety of timer adv issues | expand |
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@tencent.com> > Cc: Liran Alon <liran.alon@oracle.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > 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) >
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)
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. Cc: Wanpeng Li <wanpengli@tencent.com> Cc: Liran Alon <liran.alon@oracle.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- 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(-)