| 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
| ||
|
Message-ID: <CANRm+CwhSXXAtbPn_etTg0_6ik7_5Tte7SRA4L7TJhoSRbyF7w@mail.gmail.com>
Date: Thu, 20 Oct 2016 06:57:26 +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>,
Yunhong Jiang <yunhong.jiang@...el.com>,
Wanpeng Li <wanpeng.li@...mail.com>
Subject: Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX
preemption timer support
2016-10-20 3:28 GMT+08:00 Radim Krčmář <rkrcmar@...hat.com>:
> 2016-10-17 15:45+0800, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@...mail.com>
>>
>> Most windows guests still utilize APIC Timer periodic/oneshot mode
>> instead of tsc-deadline mode, and the APIC Timer periodic/oneshot
>> mode are still emulated by high overhead hrtimer on host. This patch
>> converts the expected expire time of the periodic/oneshot mode to
>> guest deadline tsc in order to leverage VMX preemption timer logic
>> for APIC Timer tsc-deadline mode. After each preemption timer vmexit
>> preemption timer is restarted to emulate LVTT current-count register
>> is automatically reloaded from the initial-count register when the
>> count reaches 0. This patch reduces ~3800 cycles for each APIC Timer
>> periodic mode operation virtualization.
>>
>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>> Cc: Yunhong Jiang <yunhong.jiang@...el.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@...mail.com>
>> ---
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> @@ -1090,7 +1090,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
>>
>> static u32 apic_get_tmcct(struct kvm_lapic *apic)
>> {
>> - ktime_t remaining;
>> + ktime_t remaining, now;
>> s64 ns;
>> u32 tmcct;
>>
>> @@ -1101,7 +1101,8 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>> apic->lapic_timer.period == 0)
>> return 0;
>>
>> - remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
>> + now = apic->lapic_timer.timer.base->get_time();
>> + remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
>
> Periodic timer does not advance apic->lapic_timer.target_expiration,
> when rearming the hrtimer, so this would incorrectly return 0 in
> subsequent periods.
Agreed.
>
>> if (ktime_to_ns(remaining) < 0)
>> remaining = ktime_set(0, 0);
>>
>> @@ -1438,14 +1447,42 @@ static bool start_hv_timer(struct kvm_lapic *apic)
>> return apic->lapic_timer.hv_timer_in_use;
>> }
>>
>> +void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
>> +{
>> + struct kvm_lapic *apic = vcpu->arch.apic;
>> +
>> + WARN_ON(!apic->lapic_timer.hv_timer_in_use);
>> + WARN_ON(swait_active(&vcpu->wq));
>> + cancel_hv_timer(apic);
>> + apic_timer_expired(apic);
>> +
>> + if (apic_lvtt_period(apic) &&
>> + set_target_expiration(apic) &&
>> + !start_hv_timer(apic))
>> + start_sw_period(apic);
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
>> +
>> void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
>> {
>> struct kvm_lapic *apic = vcpu->arch.apic;
>>
>> WARN_ON(apic->lapic_timer.hv_timer_in_use);
>>
>> - if (apic_lvtt_tscdeadline(apic))
>> - start_hv_timer(apic);
>> + if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
>> + ktime_t remaining, now;
>> + u64 tscl = rdtsc();
>> +
>> + now = apic->lapic_timer.timer.base->get_time();
>> + remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
>> + if (ktime_to_ns(remaining) < 0)
>> + remaining = ktime_set(0, 0);
>> +
>> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
>> + nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining));
>> + apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining));
>
> For oneshot timer, there is no need to hrtimer_get_remaining(), because
> apic->lapic_timer.tscdeadline and apic->lapic_timer.target_expiration
> are already correct, so we could just use them.
Agreed.
>
> The same could be true for the periodic timer as well, but
> apic->lapic_timer.target_expiration nor apic->lapic_timer.tscdeadline is
> advanced in apic_timer_fn(), so they are soon incorrect.
Yeah, I catch this when testing.
>
> I think it would be better to add a function to advance the periodic
> timer and use it in kvm_lapic_expired_hv_timer() and in apic_timer_fn().
>
> The function can be simpler than set_target_expiration(), because it
> just adds the period to an existing timer. Periodic timer will also be
> better then, because the period will not depend on KVM's latency when
> rearming.
Good point, what's the function name do you like? How about
advance_target_expiration()?
>
>> + }
>> + start_hv_timer(apic);
>> }
>> EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
>>
>> @@ -1462,7 +1499,10 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
>> if (atomic_read(&apic->lapic_timer.pending))
>> return;
>>
>> - start_sw_tscdeadline(apic);
>> + if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
>> + start_sw_period(apic);
>> + else if (apic_lvtt_tscdeadline(apic))
>> + start_sw_tscdeadline(apic);
>> }
>> EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
>>
>> @@ -1470,9 +1510,11 @@ static void start_apic_timer(struct kvm_lapic *apic)
>> {
>> atomic_set(&apic->lapic_timer.pending, 0);
>>
>> - if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
>> - start_sw_period(apic);
>> - else if (apic_lvtt_tscdeadline(apic)) {
>> + if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
>> + if (set_target_expiration(apic) &&
>> + !(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>> + start_sw_period(apic);
>> + } else if (apic_lvtt_tscdeadline(apic)) {
>> if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>> start_sw_tscdeadline(apic);
>> }
>> @@ -2005,8 +2047,11 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>>
>> if (atomic_read(&apic->lapic_timer.pending) > 0) {
>> kvm_apic_local_deliver(apic, APIC_LVTT);
>> - if (apic_lvtt_tscdeadline(apic))
>> + if (!(apic_lvtt_period(apic) &&
>> + kvm_lapic_hv_timer_in_use(vcpu))) {
>
> This would zero apic->lapic_timer.target_expiration of
> apic_lvtt_period() when !kvm_lapic_hv_timer_in_use().
>
> I think we don't want to ever do that, so we want
>
> if (!(apic_lvtt_period(apic)) {
>
> or maybe even better
>
> if (apic_lvtt_tscdeadline(apic))
> apic->lapic_timer.tscdeadline = 0;
>
> if (apic_lvtt_oneshot(apic)) {
> apic->lapic_timer.tscdeadline = 0;
> apic->lapic_timer.target_expiration = ktime_set(0, 0);
> }
>
>> apic->lapic_timer.tscdeadline = 0;
>> + apic->lapic_timer.target_expiration = ktime_set(0, 0);
>> + }
Agreed, thanks for your review. :)
Regards,
Wanpeng Li
Powered by blists - more mailing lists