Message ID | 1506647099-2688-4-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
2017-09-28 18:04-0700, Wanpeng Li: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > The description in the Intel SDM of how the divide configuration > register is used: "The APIC timer frequency will be the processor's bus > clock or core crystal clock frequency divided by the value specified in > the divide configuration register." > > Observation of baremetal shown that when the TDCR is change, the TMCCT > does not change or make a big jump in value, but the rate at which it > count down change. > > The patch update the emulation to APIC timer to so that a change to the > divide configuration would be reflected in the value of the counter and > when the next interrupt is triggered. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- Why do we need to do more than just restart the timer? The TMCCT should remain roughly at the same level -- changing divide count modifies target_expiration and it looks like apic_get_tmcct() would get the same result like before changing divide count. Thanks.
2017-10-04 1:28 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2017-09-28 18:04-0700, Wanpeng Li: >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> The description in the Intel SDM of how the divide configuration >> register is used: "The APIC timer frequency will be the processor's bus >> clock or core crystal clock frequency divided by the value specified in >> the divide configuration register." >> >> Observation of baremetal shown that when the TDCR is change, the TMCCT >> does not change or make a big jump in value, but the rate at which it >> count down change. >> >> The patch update the emulation to APIC timer to so that a change to the >> divide configuration would be reflected in the value of the counter and >> when the next interrupt is triggered. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >> --- > > Why do we need to do more than just restart the timer? Because the current timer (hv or sw) are still running. I think the goal of this commit is to runtime update the rate of the current timer which is running. Our restart_apic_timer() implementation just cancels the current timer when switch between preemption timer and hrtimer. Regards, Wanpeng Li > > The TMCCT should remain roughly at the same level -- changing divide > count modifies target_expiration and it looks like apic_get_tmcct() > would get the same result like before changing divide count. > > Thanks.
2017-10-04 09:59+0800, Wanpeng Li: > 2017-10-04 1:28 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > 2017-09-28 18:04-0700, Wanpeng Li: > >> From: Wanpeng Li <wanpeng.li@hotmail.com> > >> > >> The description in the Intel SDM of how the divide configuration > >> register is used: "The APIC timer frequency will be the processor's bus > >> clock or core crystal clock frequency divided by the value specified in > >> the divide configuration register." > >> > >> Observation of baremetal shown that when the TDCR is change, the TMCCT > >> does not change or make a big jump in value, but the rate at which it > >> count down change. > >> > >> The patch update the emulation to APIC timer to so that a change to the > >> divide configuration would be reflected in the value of the counter and > >> when the next interrupt is triggered. > >> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: Radim Krčmář <rkrcmar@redhat.com> > >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > >> --- > > > > Why do we need to do more than just restart the timer? > > Because the current timer (hv or sw) are still running. I think the > goal of this commit is to runtime update the rate of the current timer > which is running. Our restart_apic_timer() implementation just cancels > the current timer when switch between preemption timer and hrtimer. I see ... we do need to know both divisors in order to make it work, thanks.
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 946c11b..6bafd06 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1432,7 +1432,7 @@ static void start_sw_period(struct kvm_lapic *apic) HRTIMER_MODE_ABS_PINNED); } -static bool set_target_expiration(struct kvm_lapic *apic, bool timer_update) +static bool set_target_expiration(struct kvm_lapic *apic, bool timer_update, uint32_t old_divisor) { ktime_t now, remaining; u64 tscl = rdtsc(), delta; @@ -1440,7 +1440,7 @@ static bool set_target_expiration(struct kvm_lapic *apic, bool timer_update) /* Calculate the next time the timer should trigger an interrupt */ now = ktime_get(); apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT) - * APIC_BUS_CYCLE_NS * apic->divide_count; + * APIC_BUS_CYCLE_NS * old_divisor; if (!apic->lapic_timer.period) return false; @@ -1485,6 +1485,12 @@ static bool set_target_expiration(struct kvm_lapic *apic, bool timer_update) if (!delta) return false; + if (apic->divide_count != old_divisor) { + apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT) + * APIC_BUS_CYCLE_NS * apic->divide_count; + delta = delta * apic->divide_count / old_divisor; + } + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) + nsec_to_cycles(apic->vcpu, delta); apic->lapic_timer.target_expiration = ktime_add_ns(now, delta); @@ -1624,12 +1630,13 @@ void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu) restart_apic_timer(apic); } -static void start_apic_timer(struct kvm_lapic *apic, bool timer_update) +static void start_apic_timer(struct kvm_lapic *apic, bool timer_update, + uint32_t old_divisor) { atomic_set(&apic->lapic_timer.pending, 0); if ((apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) - && !set_target_expiration(apic, timer_update)) + && !set_target_expiration(apic, timer_update, old_divisor)) return; restart_apic_timer(apic); @@ -1745,7 +1752,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask); kvm_lapic_set_reg(apic, APIC_LVTT, val); if (apic_update_lvtt(apic) && !apic_lvtt_tscdeadline(apic)) - start_apic_timer(apic, true); + start_apic_timer(apic, true, apic->divide_count); break; case APIC_TMICT: @@ -1754,16 +1761,20 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) hrtimer_cancel(&apic->lapic_timer.timer); kvm_lapic_set_reg(apic, APIC_TMICT, val); - start_apic_timer(apic, false); + start_apic_timer(apic, false, apic->divide_count); break; - case APIC_TDCR: + case APIC_TDCR: { + uint32_t current_divisor = apic->divide_count; + if (val & 4) apic_debug("KVM_WRITE:TDCR %x\n", val); kvm_lapic_set_reg(apic, APIC_TDCR, val); update_divide_count(apic); + hrtimer_cancel(&apic->lapic_timer.timer); + start_apic_timer(apic, true, current_divisor); break; - + } case APIC_ESR: if (apic_x2apic_mode(apic) && val != 0) { apic_debug("KVM_WRITE:ESR not zero %x\n", val); @@ -1888,7 +1899,7 @@ void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data) hrtimer_cancel(&apic->lapic_timer.timer); apic->lapic_timer.tscdeadline = data; - start_apic_timer(apic, false); + start_apic_timer(apic, false, apic->divide_count); } void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8) @@ -2254,7 +2265,7 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s) apic_update_lvtt(apic); apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0)); update_divide_count(apic); - start_apic_timer(apic, false); + start_apic_timer(apic, false, apic->divide_count); apic->irr_pending = true; apic->isr_count = vcpu->arch.apicv_active ? 1 : count_vectors(apic->regs + APIC_ISR);