From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751710AbdJFN7h (ORCPT ); Fri, 6 Oct 2017 09:59:37 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:56556 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750883AbdJFN7f (ORCPT ); Fri, 6 Oct 2017 09:59:35 -0400 X-Google-Smtp-Source: AOwi7QAFQy+kbzRDnq3992EO9yNP4tyeIkVlYeBoHnCSLVnfAArsLZA/fsYEclx/2vG/ec3Ap+I2wsrmbp+XEmMO3DY= MIME-Version: 1.0 In-Reply-To: <20171006131453.GB16466@flask> References: <1507254866-5020-1-git-send-email-wanpeng.li@hotmail.com> <1507254866-5020-4-git-send-email-wanpeng.li@hotmail.com> <20171006131453.GB16466@flask> From: Wanpeng Li Date: Fri, 6 Oct 2017 21:59:33 +0800 Message-ID: Subject: Re: [PATCH v6 3/3] KVM: LAPIC: Apply change to TDCR right away to the timer To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Cc: "linux-kernel@vger.kernel.org" , kvm , Paolo Bonzini , Wanpeng Li Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v96DxgEB031100 2017-10-06 21:14 GMT+08:00 Radim Krčmář : > 2017-10-05 18:54-0700, Wanpeng Li: >> From: Wanpeng Li >> >> 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 >> Cc: Radim Krčmář >> Signed-off-by: Wanpeng Li >> --- >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> @@ -1458,6 +1458,36 @@ static void start_sw_period(struct kvm_lapic *apic) >> HRTIMER_MODE_ABS_PINNED); >> } >> >> +static bool update_target_expiration(struct kvm_lapic *apic, uint32_t old_divisor) >> +{ >> + ktime_t now, remaining; >> + u64 tscl = rdtsc(), delta; >> + >> + now = ktime_get(); >> + 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); > > Hm, can this happen? Yeah, when the hrtimer has already expired. I can catch it during testing. > >> + if (!delta) >> + return false; >> + >> + apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT) >> + * APIC_BUS_CYCLE_NS * apic->divide_count; > > I think that it would be safer to always modify the period. Agreed. > >> + delta = delta * apic->divide_count / old_divisor; >> + >> + if (!apic->lapic_timer.period) >> + return false; >> + >> + limit_periodic_timer_frequency(apic); >> + >> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) + >> + nsec_to_cycles(apic->vcpu, delta); > > We could do that without rdtsc() for added precision and maybe > performance: Agreed. > > apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, delta) - > nsec_to_cycles(apic->vcpu, remaining); > > // not sure how a negative operand would behave: > // nsec_to_cycles(apic->vcpu, delta - remaining) > >> + apic->lapic_timer.target_expiration = ktime_add_ns(now, delta); >> + >> + return true; >> +} >> + >> static bool set_target_expiration(struct kvm_lapic *apic) >> { >> ktime_t now; >> @@ -1750,13 +1780,20 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) >> start_apic_timer(apic); >> break; >> >> - case APIC_TDCR: >> + case APIC_TDCR: { >> + uint32_t old_divisor = apic->divide_count; >> + >> if (val & 4) >> apic_debug("KVM_WRITE:TDCR %x\n", val); >> kvm_lapic_set_reg(apic, APIC_TDCR, val); >> update_divide_count(apic); >> + if (apic->divide_count != old_divisor) { >> + hrtimer_cancel(&apic->lapic_timer.timer); >> + if (update_target_expiration(apic, old_divisor)) >> + restart_apic_timer(apic); > > I think we can lose a timer here when we cancel a hrtimer whose > expiration time passes before update_target_expiration(), so it never > gets restarted. > > Doing restart_apic_timer() unconditionally seems better. It behaves > well if we try to restart a timer that has already fired. Agreed. Regards, Wanpeng Li > > Thanks. > >> + } >> break; >> - >> + } >> case APIC_ESR: >> if (apic_x2apic_mode(apic) && val != 0) { >> apic_debug("KVM_WRITE:ESR not zero %x\n", val); >> -- >> 2.7.4 >>