[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANRm+CzQtB99=+OG1ZbQkd1s=kUMQO=PM5QRqK6f_7Y6U+_7Fw@mail.gmail.com>
Date: Fri, 6 Oct 2017 07:14:04 +0800
From: Wanpeng Li <kernellwp@...il.com>
To: Radim Krčmář <rkrcmar@...hat.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
kvm <kvm@...r.kernel.org>, Paolo Bonzini <pbonzini@...hat.com>,
Wanpeng Li <wanpeng.li@...mail.com>
Subject: Re: [PATCH v5 3/3] KVM: LAPIC: Apply change to TDCR right away to the timer
2017-10-06 2:14 GMT+08:00 Radim Krčmář <rkrcmar@...hat.com>:
> 2017-10-05 07:35-0700, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@...mail.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@...hat.com>
>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@...mail.com>
>> ---
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> @@ -1474,9 +1474,24 @@ static bool set_target_expiration(struct kvm_lapic *apic)
>> ktime_to_ns(ktime_add_ns(now,
>> apic->lapic_timer.period)));
>>
>> + delta = apic->lapic_timer.period;
>> + if (apic->divide_count != old_divisor) {
>
> Hm, nothing should happen if the guest writes the same value TDCR, but
> we'll reset the timer. (An extra argument would solve it, but maybe it
> would be nicer to add a new function for updating the expiration.)
Agreed.
>
>> + 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 false;
>> +
>> + apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
>> + * APIC_BUS_CYCLE_NS * apic->divide_count;
>
> I'd prefer to apply the rate limiting (done earlier in this function) to
> the period. This version allows the guest to configure 128 times more
> frequent interrupts in the host.
> (And thinking about it, the version of [2/3] I proposed has similar
> problem when switching from one-shot to periodic, only there it is
> unpredictably limited by the speed of KVM.)
We didn't stop and restart the timer, why the rate will influence us for [2/3]?
Regards,
Wanpeng Li
>
> Thanks.
>
>> + delta = delta * apic->divide_count / old_divisor;
>> + }
>> +
>> apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
>> - nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
>> - apic->lapic_timer.target_expiration = ktime_add_ns(now, apic->lapic_timer.period);
>> + nsec_to_cycles(apic->vcpu, delta);
>> + apic->lapic_timer.target_expiration = ktime_add_ns(now, delta);
>>
>> return true;
>> }
Powered by blists - more mailing lists