2017-10-06 21:14 GMT+08:00 Radim Krčmář <rkrcmar@xxxxxxxxxx>:
> 2017-10-05 18:54-0700, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Cc: Radim Krčmář <rkrcmar@xxxxxxxxxx>
>> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>> ---
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> @@ -1458,6 +1458,36 @@ static void start_sw_period(struct kvm_lapic *apic)
>> HRTIMER_MODE_ABS_PINNED);
>> }
>>
>> +static bool update_target_expiration(struct kvm_lapic *apic, uint32_t old_divisor)
>> +{
>> + ktime_t now, remaining;
>> + u64 tscl = rdtsc(), delta;
>> +
>> + now = ktime_get();
>> + remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
>> + if (ktime_to_ns(remaining) < 0)
>> + remaining = 0;
>> + delta = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
>
> Hm, can this happen?
Yeah, when the hrtimer has already expired. I can catch it during testing.
>
>> + if (!delta)
>> + return false;
>> +
>> + apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
>> + * APIC_BUS_CYCLE_NS * apic->divide_count;
>
> I think that it would be safer to always modify the period.
Agreed.
>
>> + delta = delta * apic->divide_count / old_divisor;
>> +
>> + if (!apic->lapic_timer.period)
>> + return false;
>> +
>> + limit_periodic_timer_frequency(apic);
>> +
>> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
>> + nsec_to_cycles(apic->vcpu, delta);
>
> We could do that without rdtsc() for added precision and maybe
> performance:
Agreed.
>
> apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, delta) -
> nsec_to_cycles(apic->vcpu, remaining);
>
> // not sure how a negative operand would behave:
> // nsec_to_cycles(apic->vcpu, delta - remaining)
>
>> + apic->lapic_timer.target_expiration = ktime_add_ns(now, delta);
>> +
>> + return true;
>> +}
>> +
>> static bool set_target_expiration(struct kvm_lapic *apic)
>> {
>> ktime_t now;
>> @@ -1750,13 +1780,20 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>> start_apic_timer(apic);
>> break;
>>
>> - case APIC_TDCR:
>> + case APIC_TDCR: {
>> + uint32_t old_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);
>> + if (apic->divide_count != old_divisor) {
>> + hrtimer_cancel(&apic->lapic_timer.timer);
>> + if (update_target_expiration(apic, old_divisor))
>> + restart_apic_timer(apic);
>
> I think we can lose a timer here when we cancel a hrtimer whose
> expiration time passes before update_target_expiration(), so it never
> gets restarted.
>
> Doing restart_apic_timer() unconditionally seems better. It behaves
> well if we try to restart a timer that has already fired.
Agreed.
Regards,
Wanpeng Li
>
> Thanks.
>
>> + }
>> break;
>> -
>> + }
>> case APIC_ESR:
>> if (apic_x2apic_mode(apic) && val != 0) {
>> apic_debug("KVM_WRITE:ESR not zero %x\n", val);
>> --
>> 2.7.4
>>
