linux-kernel - Re: [PATCH v3 2/3] KVM: LAPIC: Keep timer running when switching between one-shot and periodic mode

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 5 Oct 2017 20:54:01 +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 v3 2/3] KVM: LAPIC: Keep timer running when switching
 between one-shot and periodic mode

2017-10-05 20:40 GMT+08:00 Radim Krčmář <rkrcmar@...hat.com>:
> 2017-10-05 03:53-0700, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@...mail.com>
>>
>> If we take TSC-deadline mode timer out of the picture, the Intel SDM
>> does not say that the timer is disable when the timer mode is change,
>> either from one-shot to periodic or vice versa.
>>
>> After this patch, the timer is no longer disarmed on change of mode, so
>> the counter (TMCCT) keeps counting down.
>>
>> So what does a write to LVTT changes ? On baremetal, the change of mode
>> is probably taken into account only when the counter reach 0. When this
>> happen, LVTT is use to figure out if the counter should restard counting
>> down from TMICT (so periodic mode) or stop counting (if one-shot mode).
>>
>> This patch is based on observation of the behavior of the APIC timer on
>> baremetal as well as check that they does not go against the description
>> written in the Intel SDM.
>>
>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@...mail.com>
>> ---
>>  arch/x86/kvm/lapic.c | 45 +++++++++++++++++++++++++++++++--------------
>>  1 file changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> @@ -1729,7 +1745,8 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>>                       val |= APIC_LVT_MASKED;
>>               val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask);
>>               kvm_lapic_set_reg(apic, APIC_LVTT, val);
>> -             apic_update_lvtt(apic);
>> +             if (apic_update_lvtt(apic) && !apic_lvtt_tscdeadline(apic))
>> +                     start_apic_timer(apic, true);
>
> start_apic_timer() is not needed here: when switching from
> apic_lvtt_tscdeadline(), we have TMICT = 0, so the timer is disabled.
> When switching between one-shot and periodic, the timer is running with
> a correct expiration time.
>
> This will bring us close to bare-metal behavior wrt. races and also
> allows us to get rid of apic_update_lvtt() return value and an argument
> to start_apic_timer().

I see, what this patch should do is to skip hrtimer_cancel() in
apic_update_lvtt() when either from one-shot mode to periodic or vice
versa.

Regards,
Wanpeng Li