| Message ID | 1622710841-76604-2-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer | expand |
On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <kernellwp@gmail.com> wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > The value of current counter register after reset is 0 for both Intel > and AMD, let's do it in kvm. > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> How did we miss that? Reviewed-by: Jim Mattson <jmattson@google.com>
On Thu, Jun 03, 2021, Jim Mattson wrote: > On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <kernellwp@gmail.com> wrote: > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > The value of current counter register after reset is 0 for both Intel > > and AMD, let's do it in kvm. > > > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > How did we miss that? I suspect it's not actually a functional issue, and that writing '0' at reset is a glorified nop. The TMCCT is always computed on-demand and never directly readable. Is there an observable bug being fixed? If not, the changelog should state that this is a cosmetic change of sorts. static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset) { u32 val = 0; if (offset >= LAPIC_MMIO_LENGTH) return 0; switch (offset) { case APIC_ARBPRI: break; case APIC_TMCCT: /* Timer CCR */ if (apic_lvtt_tscdeadline(apic)) return 0; val = apic_get_tmcct(apic); break; ... } static u32 apic_get_tmcct(struct kvm_lapic *apic) { ktime_t remaining, now; s64 ns; u32 tmcct; ASSERT(apic != NULL); /* if initial count is 0, current count should also be 0 */ if (kvm_lapic_get_reg(apic, APIC_TMICT) == 0 || <------------ apic->lapic_timer.period == 0) return 0; now = ktime_get(); remaining = ktime_sub(apic->lapic_timer.target_expiration, now); if (ktime_to_ns(remaining) < 0) remaining = 0; ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period); tmcct = div64_u64(ns, (APIC_BUS_CYCLE_NS * apic->divide_count)); return tmcct; } int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s) { memcpy(s->regs, vcpu->arch.apic->regs, sizeof(*s)); /* * Get calculated timer current count for remaining timer period (if * any) and store it in the returned register set. */ __kvm_lapic_set_reg(s->regs, APIC_TMCCT, __apic_read(vcpu->arch.apic, APIC_TMCCT)); <---- return kvm_apic_state_fixup(vcpu, s, false); }
On Thu, 3 Jun 2021 at 23:34, Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Jun 03, 2021, Jim Mattson wrote: > > On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <kernellwp@gmail.com> wrote: > > > > > > From: Wanpeng Li <wanpengli@tencent.com> > > > > > > The value of current counter register after reset is 0 for both Intel > > > and AMD, let's do it in kvm. > > > > > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > > > How did we miss that? > > I suspect it's not actually a functional issue, and that writing '0' at reset is > a glorified nop. The TMCCT is always computed on-demand and never directly > readable. Update the patch description in v2, thanks. Wanpeng > > Is there an observable bug being fixed? If not, the changelog should state that > this is a cosmetic change of sorts. > > static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset) > { > u32 val = 0; > > if (offset >= LAPIC_MMIO_LENGTH) > return 0; > > switch (offset) { > case APIC_ARBPRI: > break; > > case APIC_TMCCT: /* Timer CCR */ > if (apic_lvtt_tscdeadline(apic)) > return 0; > > val = apic_get_tmcct(apic); > break; > ... > } > > > static u32 apic_get_tmcct(struct kvm_lapic *apic) > { > ktime_t remaining, now; > s64 ns; > u32 tmcct; > > ASSERT(apic != NULL); > > /* if initial count is 0, current count should also be 0 */ > if (kvm_lapic_get_reg(apic, APIC_TMICT) == 0 || <------------ > apic->lapic_timer.period == 0) > return 0; > > now = ktime_get(); > remaining = ktime_sub(apic->lapic_timer.target_expiration, now); > if (ktime_to_ns(remaining) < 0) > remaining = 0; > > ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period); > tmcct = div64_u64(ns, > (APIC_BUS_CYCLE_NS * apic->divide_count)); > > return tmcct; > } > > int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s) > { > memcpy(s->regs, vcpu->arch.apic->regs, sizeof(*s)); > > /* > * Get calculated timer current count for remaining timer period (if > * any) and store it in the returned register set. > */ > __kvm_lapic_set_reg(s->regs, APIC_TMCCT, > __apic_read(vcpu->arch.apic, APIC_TMCCT)); <---- > > return kvm_apic_state_fixup(vcpu, s, false); > } > > >
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 20dd2ae..9ba539b 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2352,6 +2352,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) kvm_lapic_set_reg(apic, APIC_ICR2, 0); kvm_lapic_set_reg(apic, APIC_TDCR, 0); kvm_lapic_set_reg(apic, APIC_TMICT, 0); + kvm_lapic_set_reg(apic, APIC_TMCCT, 0); for (i = 0; i < 8; i++) { kvm_lapic_set_reg(apic, APIC_IRR + 0x10 * i, 0); kvm_lapic_set_reg(apic, APIC_ISR + 0x10 * i, 0);