[v7] KVM: LAPIC: Apply change to TDCR right away to the timer

Commit Message

Wanpeng Li Oct. 6, 2017, 2:01 p.m. UTC

From: Wanpeng Li <[e-mail]>

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 <[e-mail]>
Cc: Radim Krčmář <[e-mail]>
Signed-off-by: Wanpeng Li <[e-mail]>
---
v6 -> v7:
 * always modify the period
 * added precision and maybe performance
 * doing restart_apic_timer() unconditionally

 arch/x86/kvm/lapic.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Oct. 6, 2017, 2:21 p.m. UTC | #1

On 06/10/2017 16:01, Wanpeng Li wrote:
> +	if (!apic->lapic_timer.period)
> +		return;
> +
> +	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);

I think this shouldn't be happening.  If it does, I'm not sure the mod 
is the right thing to do, so I'd just use ktime_to_ns(remaining).

So perhaps let's simplify all this to:

	ns_remaining_old = ktime_to_ns(remaining);
	ns_remaining_new = mul_u64_u32_div(ns_remaining_old,
                                           apic->divide_count, old_divisor);

because below you're calling nsec_to_cycles but remaining is not expressed
in nanoseconds.

> +	if (!delta)
> +		return;
> +
> +	delta = delta * apic->divide_count / old_divisor;
> +
> +	limit_periodic_timer_frequency(apic);

This should be done before all the "if"s (which should not be there in v8,
but you should still call it before "now = ktime_get();").

Paolo

> +	apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, delta) -
> +                                    nsec_to_cycles(apic->vcpu, remaining);
> +	apic->lapic_timer.target_expiration = ktime_add_ns(now, delta);
> +}

Wanpeng Li Oct. 6, 2017, 2:40 p.m. UTC | #2

2017-10-06 22:21 GMT+08:00 Paolo Bonzini <[e-mail]>:
> On 06/10/2017 16:01, Wanpeng Li wrote:
>> +     if (!apic->lapic_timer.period)
>> +             return;
>> +
>> +     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);
>
> I think this shouldn't be happening.  If it does, I'm not sure the mod
> is the right thing to do, so I'd just use ktime_to_ns(remaining).
>
> So perhaps let's simplify all this to:
>
>         ns_remaining_old = ktime_to_ns(remaining);
>         ns_remaining_new = mul_u64_u32_div(ns_remaining_old,
>                                            apic->divide_count, old_divisor);
>
> because below you're calling nsec_to_cycles but remaining is not expressed
> in nanoseconds.
>
>> +     if (!delta)
>> +             return;
>> +
>> +     delta = delta * apic->divide_count / old_divisor;
>> +
>> +     limit_periodic_timer_frequency(apic);
>
> This should be done before all the "if"s (which should not be there in v8,
> but you should still call it before "now = ktime_get();").

Just sent out v8 to handle all the comments.

Regards,
Wanpeng Li

>
> Paolo
>
>> +     apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, delta) -
>> +                                    nsec_to_cycles(apic->vcpu, remaining);
>> +     apic->lapic_timer.target_expiration = ktime_add_ns(now, delta);
>> +}
>

Patch

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 14f63b3..bf047e3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1458,6 +1458,35 @@  static void start_sw_period(struct kvm_lapic *apic)
 		HRTIMER_MODE_ABS_PINNED);
 }
 
+static void update_target_expiration(struct kvm_lapic *apic, uint32_t old_divisor)
+{
+	ktime_t now, remaining;
+	u64 delta;
+
+	apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
+		* APIC_BUS_CYCLE_NS * apic->divide_count;
+
+	if (!apic->lapic_timer.period)
+		return;
+
+	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);
+
+	if (!delta)
+		return;
+
+	delta = delta * apic->divide_count / old_divisor;
+
+	limit_periodic_timer_frequency(apic);
+
+	apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, delta) -
+                                    nsec_to_cycles(apic->vcpu, remaining);
+	apic->lapic_timer.target_expiration = ktime_add_ns(now, delta);
+}
+
 static bool set_target_expiration(struct kvm_lapic *apic)
 {
 	ktime_t now;
@@ -1750,13 +1779,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);
+			update_target_expiration(apic, old_divisor);
+			restart_apic_timer(apic);
+		}
 		break;
-
+	}
 	case APIC_ESR:
 		if (apic_x2apic_mode(apic) && val != 0) {
 			apic_debug("KVM_WRITE:ESR not zero %x\n", val);