> On 23 Apr 2019, at 0:13, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
>
> On Tue, Apr 16, 2019 at 08:36:34PM +0300, Liran Alon wrote:
>> If guest sets MSR_IA32_TSCDEADLINE to value such that in host
>> time-domain it's shorter than lapic_timer_advance_ns, we can
>> reach a case that we call hrtimer_start() with expiration time set at
>> the past.
>>
>> Because lapic_timer.timer is init with HRTIMER_MODE_ABS_PINNED, it
>> is not allowed to run in softirq and therefore will never expire.
>>
>> To avoid such a scenario, verify that deadline expiration time is set on
>> host time-domain further than (now + lapic_timer_advance_ns).
>>
>> A future patch can also consider adding a min_timer_deadline_ns module parameter,
>> similar to min_timer_period_us to avoid races that amount of ns it takes
>> to run logic could still call hrtimer_start() with expiration timer set
>> at the past.
>>
>> Reviewed-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
>> ---
>> arch/x86/kvm/lapic.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 9f089e2e09d0..fbd37e07e90d 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1538,9 +1538,12 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
>>
>> now = ktime_get();
>> guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
>> - if (likely(tscdeadline > guest_tsc)) {
>> - ns = (tscdeadline - guest_tsc) * 1000000ULL;
>> - do_div(ns, this_tsc_khz);
>> +
>> + ns = (tscdeadline - guest_tsc) * 1000000ULL;
>> + do_div(ns, this_tsc_khz);
>
> This will do the division even if 'tscdeadline <= guest_tsc’.
So? I don’t see an issue with that…
Seems like a neglectable optimisation and code is very readable in my opinion as proposed in this patch.
> I don't
> see any reason to use ktime_sub_ns() since neither 'ns' or
> 'lapic_timer_advance_ns' is a ktime_t, e.g.:
>
> if (likely(tscdeadline > guest_tsc)) {
> ns = (tscdeadline - guest_tsc) * 1000000ULL;
> do_div(ns, this_tsc_khz);
> if (likely(ns > lapic_timer_advance_ns))
> ns -= lapic_timer_advance_ns;
> else
> ns = 0;
> } else {
> ns = 0;
> }
> if (ns) {
> now = ktime_get();
> expire = ktime_add_ns(now, ns);
> hrtimer_start(&apic->lapic_timer.timer,
> expire, HRTIMER_MODE_ABS_PINNED);
> } else
> apic_timer_expired(apic);
This would obviously work.
It’s a matter of taste of course. I personally think it makes code unnecessarily less readable.
I would let Paolo decide of course :)
-Liran
>
>> +
>> + if (likely(tscdeadline > guest_tsc) &&
>> + likely(ns > lapic_timer_advance_ns)) {
>> expire = ktime_add_ns(now, ns);
>> expire = ktime_sub_ns(expire, lapic_timer_advance_ns);
>> hrtimer_start(&apic->lapic_timer.timer,
>> --
>> 2.20.1
>>
