| Message ID | 1506647168-2846-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 28, 2017 at 6:06 PM, Wanpeng Li <kernellwp@gmail.com> wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > + /* > + * After the change of mode, the counter should not be reset and continue > + * counting down from where it was > + */ This seems to contradict the Intel SDM, volume 3, section 10.5.4.1: A write to the LVT Timer Register that changes the timer mode disarms the local APIC timer.
2018-04-05 7:12 GMT+08:00 Jim Mattson <jmattson@google.com>: > On Thu, Sep 28, 2017 at 6:06 PM, Wanpeng Li <kernellwp@gmail.com> wrote: >> From: Wanpeng Li <wanpeng.li@hotmail.com> > >> + /* >> + * After the change of mode, the counter should not be reset and continue >> + * counting down from where it was >> + */ > > This seems to contradict the Intel SDM, volume 3, section 10.5.4.1: > > A write to the LVT Timer Register that changes the timer mode disarms > the local APIC timer. The statement in SDM just against TSC-Deadline Mode. Regards, Wanpeng Li
I agree that the manual is confusing in that it makes the statement under the subsection about the TSC-Deadline mode. But the statement is also used to introduce Table 10-2, which discusses all three APIC timer modes (i.e., One-shot, Periodic, and TSC-Deadline). Regardless, I found that this test fails on my system. When I dug into it, in addition to observing what the manual says, I also observed that in the KVM source code, the timer is armed when the APIC_TMICTT is written [1], whereas it is NOT armed when the APIC_LVTT register is written [2] to change the APIC timer's mode. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/x86/kvm/lapic.c#n1801 [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/x86/kvm/lapic.c#n1787 Thanks, Marc On Wed, Apr 4, 2018 at 5:10 PM Wanpeng Li <kernellwp@gmail.com> wrote: > 2018-04-05 7:12 GMT+08:00 Jim Mattson <jmattson@google.com>: > > On Thu, Sep 28, 2017 at 6:06 PM, Wanpeng Li <kernellwp@gmail.com> wrote: > >> From: Wanpeng Li <wanpeng.li@hotmail.com> > > > >> + /* > >> + * After the change of mode, the counter should not be reset and continue > >> + * counting down from where it was > >> + */ > > > > This seems to contradict the Intel SDM, volume 3, section 10.5.4.1: > > > > A write to the LVT Timer Register that changes the timer mode disarms > > the local APIC timer. > The statement in SDM just against TSC-Deadline Mode. > Regards, > Wanpeng Li
2018-04-05 8:25 GMT+08:00 Marc Orr <marcorr@google.com>: > I agree that the manual is confusing in that it makes the statement under > the subsection about the TSC-Deadline mode. But the statement is also used > to introduce Table 10-2, which discusses all three APIC timer modes (i.e., > One-shot, Periodic, and TSC-Deadline). > > Regardless, I found that this test fails on my system. When I dug into it, > in addition to observing what the manual says, I also observed that in the > KVM source code, the timer is armed when the APIC_TMICTT is written, whereas > it is NOT armed when the APIC_LVTT register is written to change the APIC > timer's mode. Xen guys verify on the bare-metal, the statement just influences TSC-Deadline mode. Then, both Xen and KVM rework the lapic timer emulation logic to behave more like real-hardware. Regards, Wanpeng Li
The sentence I quoted from the SDM is unequivocal. Are you saying that this is an error in the SDM, and that it should actually read: A write to the LVT Timer Register that changes the timer mode *to TSC-Deadline mode* disarms the local APIC timer. Has anyone reported this error to Intel, so that the manual can be corrected? On Wed, Apr 4, 2018 at 5:33 PM, Wanpeng Li <kernellwp@gmail.com> wrote: > 2018-04-05 8:25 GMT+08:00 Marc Orr <marcorr@google.com>: >> I agree that the manual is confusing in that it makes the statement under >> the subsection about the TSC-Deadline mode. But the statement is also used >> to introduce Table 10-2, which discusses all three APIC timer modes (i.e., >> One-shot, Periodic, and TSC-Deadline). >> >> Regardless, I found that this test fails on my system. When I dug into it, >> in addition to observing what the manual says, I also observed that in the >> KVM source code, the timer is armed when the APIC_TMICTT is written, whereas >> it is NOT armed when the APIC_LVTT register is written to change the APIC >> timer's mode. > > Xen guys verify on the bare-metal, the statement just influences > TSC-Deadline mode. Then, both Xen and KVM rework the lapic timer > emulation logic to behave more like real-hardware. > > Regards, > Wanpeng Li
On Thu, 2018-04-05, Jim Mattson wrote: > The sentence I quoted from the SDM is unequivocal. Are you saying that > this is an error in the SDM, and that it should actually read: > > A write to the LVT Timer Register that changes the timer mode *to > TSC-Deadline mode* disarms the local APIC timer. > > Has anyone reported this error to Intel, so that the manual can be corrected? I suspect that Wanpeng's analysis is correct and that the timer is disarmed only when changing to/from deadline mode, i.e. this is an SDM bug. I'll hunt down the actual behavior and report back (and file an SDM bug if that's indeed the issue). I have a copy of the SDM circa 2010, before deadline mode was added, and there is no mention of the timer being disarmed when transitioning between one-shot and periodic. And further down in 10.5.4.1 of the current SDM, it has a second blurb about disarming the timer when switching between TSC-deadline mode and other timer modes, which seems to imply that switching between one-shot and periodic does not disarm the timer. In TSC-deadline mode, writing 0 to the IA32_TSC_DEADLINE MSR disarms the local-APIC timer. Transitioning between TSC-deadline mode and other timer modes also disarms the timer. > On Wed, Apr 4, 2018 at 5:33 PM, Wanpeng Li <kernellwp@gmail.com> wrote: > > 2018-04-05 8:25 GMT+08:00 Marc Orr <marcorr@google.com>: > >> I agree that the manual is confusing in that it makes the statement under > >> the subsection about the TSC-Deadline mode. But the statement is also used > >> to introduce Table 10-2, which discusses all three APIC timer modes (i.e., > >> One-shot, Periodic, and TSC-Deadline). > >> > >> Regardless, I found that this test fails on my system. When I dug into it, > >> in addition to observing what the manual says, I also observed that in the > >> KVM source code, the timer is armed when the APIC_TMICTT is written, whereas > >> it is NOT armed when the APIC_LVTT register is written to change the APIC > >> timer's mode. > > > > Xen guys verify on the bare-metal, the statement just influences > > TSC-Deadline mode. Then, both Xen and KVM rework the lapic timer > > emulation logic to behave more like real-hardware. > > > > Regards, > > Wanpeng Li
On Thu, Apr 05, 2018 at 07:25:15PM +0000, Christopherson, Sean J wrote: > On Thu, 2018-04-05, Jim Mattson wrote: > > The sentence I quoted from the SDM is unequivocal. Are you saying that > > this is an error in the SDM, and that it should actually read: > > > > A write to the LVT Timer Register that changes the timer mode *to > > TSC-Deadline mode* disarms the local APIC timer. > > > > Has anyone reported this error to Intel, so that the manual can be corrected? > > I suspect that Wanpeng's analysis is correct and that the timer is > disarmed only when changing to/from deadline mode, i.e. this is an > SDM bug. I'll hunt down the actual behavior and report back (and > file an SDM bug if that's indeed the issue). Confirmed SDM bug, the timer is only disarmed when switching between TSC-Deadline and other modes. > I have a copy of the SDM circa 2010, before deadline mode was added, > and there is no mention of the timer being disarmed when transitioning > between one-shot and periodic. And further down in 10.5.4.1 of the > current SDM, it has a second blurb about disarming the timer when > switching between TSC-deadline mode and other timer modes, which seems > to imply that switching between one-shot and periodic does not disarm > the timer. > > In TSC-deadline mode, writing 0 to the IA32_TSC_DEADLINE MSR > disarms the local-APIC timer. Transitioning between TSC-deadline > mode and other timer modes also disarms the timer. > > > On Wed, Apr 4, 2018 at 5:33 PM, Wanpeng Li <kernellwp@gmail.com> wrote: > > > 2018-04-05 8:25 GMT+08:00 Marc Orr <marcorr@google.com>: > > >> I agree that the manual is confusing in that it makes the statement under > > >> the subsection about the TSC-Deadline mode. But the statement is also used > > >> to introduce Table 10-2, which discusses all three APIC timer modes (i.e., > > >> One-shot, Periodic, and TSC-Deadline). > > >> > > >> Regardless, I found that this test fails on my system. When I dug into it, > > >> in addition to observing what the manual says, I also observed that in the > > >> KVM source code, the timer is armed when the APIC_TMICTT is written, whereas > > >> it is NOT armed when the APIC_LVTT register is written to change the APIC > > >> timer's mode. > > > > > > Xen guys verify on the bare-metal, the statement just influences > > > TSC-Deadline mode. Then, both Xen and KVM rework the lapic timer > > > emulation logic to behave more like real-hardware. > > > > > > Regards, > > > Wanpeng Li >
2018-05-11 23:25 GMT+08:00 Sean Christopherson <sean.j.christopherson@intel.com>: > On Thu, Apr 05, 2018 at 07:25:15PM +0000, Christopherson, Sean J wrote: >> On Thu, 2018-04-05, Jim Mattson wrote: >> > The sentence I quoted from the SDM is unequivocal. Are you saying that >> > this is an error in the SDM, and that it should actually read: >> > >> > A write to the LVT Timer Register that changes the timer mode *to >> > TSC-Deadline mode* disarms the local APIC timer. >> > >> > Has anyone reported this error to Intel, so that the manual can be corrected? >> >> I suspect that Wanpeng's analysis is correct and that the timer is >> disarmed only when changing to/from deadline mode, i.e. this is an >> SDM bug. I'll hunt down the actual behavior and report back (and >> file an SDM bug if that's indeed the issue). > > Confirmed SDM bug, the timer is only disarmed when switching between > TSC-Deadline and other modes. Great, as expected, thanks Christopherson! Regards, Wanpeng Li
diff --git a/lib/x86/apic-defs.h b/lib/x86/apic-defs.h index e0c3cca..a7bc6a0 100644 --- a/lib/x86/apic-defs.h +++ b/lib/x86/apic-defs.h @@ -91,6 +91,7 @@ #define APIC_TIMER_BASE_CLKIN 0x0 #define APIC_TIMER_BASE_TMBASE 0x1 #define APIC_TIMER_BASE_DIV 0x2 +#define APIC_LVT_TIMER_MASK (3 << 17) #define APIC_LVT_TIMER_ONESHOT (0 << 17) #define APIC_LVT_TIMER_PERIODIC (1 << 17) #define APIC_LVT_TIMER_TSCDEADLINE (2 << 17) diff --git a/x86/apic.c b/x86/apic.c index eb78579..6cfb52d 100644 --- a/x86/apic.c +++ b/x86/apic.c @@ -458,6 +458,83 @@ static void test_physical_broadcast(void) report("APIC physical broadcast shorthand", broadcast_received(ncpus)); } +void wait_until_tmcct_is_zero(uint32_t initial_count, bool stop_when_half) +{ + uint32_t tmcct = apic_read(APIC_TMCCT); + + if (tmcct) { + while (tmcct > (initial_count / 2)) + tmcct = apic_read(APIC_TMCCT); + + if ( stop_when_half ) + return; + + /* Wait until the counter reach 0 or wrap-around */ + while ( tmcct <= (initial_count / 2) && tmcct > 0 ) + tmcct = apic_read(APIC_TMCCT); + } +} + +static inline void apic_change_mode(unsigned long new_mode) +{ + uint32_t lvtt; + + lvtt = apic_read(APIC_LVTT); + apic_write(APIC_LVTT, (lvtt & ~APIC_LVT_TIMER_MASK) | new_mode); +} + +static void test_apic_change_mode() +{ + uint32_t tmict = 0x999999; + + printf("starting apic change mode\n"); + + apic_write(APIC_TMICT, tmict); + + apic_change_mode(APIC_LVT_TIMER_PERIODIC); + + report("TMICT value reset", apic_read(APIC_TMICT) == tmict); + + /* Testing one-shot */ + apic_change_mode(APIC_LVT_TIMER_ONESHOT); + apic_write(APIC_TMICT, tmict); + report("TMCCT should have a non-zero value", apic_read(APIC_TMCCT)); + + wait_until_tmcct_is_zero(tmict, false); + report("TMCCT should have reached 0", !apic_read(APIC_TMCCT)); + + /* + * Write TMICT before changing mode from one-shot to periodic TMCCT should + * be reset to TMICT periodicly + */ + apic_write(APIC_TMICT, tmict); + wait_until_tmcct_is_zero(tmict, true); + apic_change_mode(APIC_LVT_TIMER_PERIODIC); + report("TMCCT should have a non-zero value", apic_read(APIC_TMCCT)); + + /* + * After the change of mode, the counter should not be reset and continue + * counting down from where it was + */ + report("TMCCT should not be reset to TMICT value", apic_read(APIC_TMCCT) < (tmict / 2)); + wait_until_tmcct_is_zero(tmict, false); + report("TMCCT should be reset to the initial-count", apic_read(APIC_TMCCT) > (tmict / 2)); + + wait_until_tmcct_is_zero(tmict, true); + /* + * Keep the same TMICT and change timer mode to one-shot + * TMCCT should be > 0 and count-down to 0 + */ + apic_change_mode(APIC_LVT_TIMER_ONESHOT); + report("TMCCT should not be reset to init", apic_read(APIC_TMCCT) < (tmict / 2)); + wait_until_tmcct_is_zero(tmict, false); + report("TMCCT should have reach zero", !apic_read(APIC_TMCCT)); + + /* now tmcct == 0 and tmict != 0 */ + apic_change_mode(APIC_LVT_TIMER_PERIODIC); + report("TMCCT should stay at zero", !apic_read(APIC_TMCCT)); +} + int main() { setup_vm(); @@ -478,6 +555,7 @@ int main() test_multiple_nmi(); test_apic_timer_one_shot(); + test_apic_change_mode(); test_tsc_deadline_timer(); return report_summary();