linux-kernel - Re: [PATCH v5 3/3] KVM: LAPIC: Apply change to TDCR right away to the timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANRm+CxT1D+2aBACDs7D98mt9UrPEMoctcUeOBov1n3Ta_bWcA@mail.gmail.com>
Date:   Fri, 6 Oct 2017 20:55:40 +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 7:14 GMT+08:00 Wanpeng Li <kernellwp@...il.com>:
> 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]?

Have already done in v6, please have a review. :)

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;
>>>  }