From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752238AbdJDNdq (ORCPT ); Wed, 4 Oct 2017 09:33:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35374 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752066AbdJDNdo (ORCPT ); Wed, 4 Oct 2017 09:33:44 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AAF6A81DEE Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=rkrcmar@redhat.com Date: Wed, 4 Oct 2017 15:33:41 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Wanpeng Li Cc: "linux-kernel@vger.kernel.org" , kvm , Paolo Bonzini , Wanpeng Li Subject: Re: [PATCH v2 2/4] KVM: LAPIC: Keep timer running when switching between one-shot and periodic mode Message-ID: <20171004133341.GG21107@flask> References: <1506647099-2688-1-git-send-email-wanpeng.li@hotmail.com> <1506647099-2688-3-git-send-email-wanpeng.li@hotmail.com> <20171003170655.GB21107@flask> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 04 Oct 2017 13:33:44 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-10-04 09:46+0800, Wanpeng Li: > 2017-10-04 1:06 GMT+08:00 Radim Krčmář : > > 2017-09-28 18:04-0700, Wanpeng Li: > >> From: Wanpeng Li > >> > >> 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. > > > > I think it does, please see comment under [v2 1/4]. > > As I replied to [v2 1/4]. Right, so we probably shouldn't disable the timer. > >> 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 > >> Cc: Radim Krčmář > >> Signed-off-by: Wanpeng Li > >> --- > >> arch/x86/kvm/lapic.c | 40 ++++++++++++++++++++++++++++------------ > >> 1 file changed, 28 insertions(+), 12 deletions(-) > >> > >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > >> index a739cbb..946c11b 100644 > >> --- a/arch/x86/kvm/lapic.c > >> +++ b/arch/x86/kvm/lapic.c > >> @@ -1301,7 +1301,7 @@ static void update_divide_count(struct kvm_lapic *apic) > >> apic->divide_count); > >> } > >> > >> -static void apic_update_lvtt(struct kvm_lapic *apic) > >> +static bool apic_update_lvtt(struct kvm_lapic *apic) > >> { > >> u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) & > >> apic->lapic_timer.timer_mode_mask; > >> @@ -1309,7 +1309,9 @@ static void apic_update_lvtt(struct kvm_lapic *apic) > >> if (apic->lapic_timer.timer_mode != timer_mode) { > >> apic->lapic_timer.timer_mode = timer_mode; > >> hrtimer_cancel(&apic->lapic_timer.timer); > >> + return true; > >> } > >> + return false; > >> } > >> > >> static void apic_timer_expired(struct kvm_lapic *apic) > >> @@ -1729,7 +1744,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); Changing the timer from one-shot to periodic doesn't change the expected expiration -- I think we could instead skip hrtimer_cancel() in apic_update_lvtt().