> On Aug 20, 2019, at 5:19 PM, Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
>
> On Wed, 21 Aug 2019 at 00:33, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
>>> On Aug 19, 2019, at 10:08 PM, Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
>>>
>>> On Tue, 20 Aug 2019 at 12:10, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
>>>>> On Aug 19, 2019, at 6:56 PM, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
>>>>>
>>>>> +Cc Nadav
>>>>>
>>>>> On Mon, Aug 19, 2019 at 06:07:01PM -0700, Matt Delco wrote:
>>>>>> On Mon, Aug 19, 2019 at 5:37 PM Sean Christopherson <
>>>>>> sean.j.christopherson@xxxxxxxxx> wrote:
>>>>>>
>>>>>>> On Tue, Aug 20, 2019 at 01:42:37AM +0200, Paolo Bonzini wrote:
>>>>>>>> On 20/08/19 01:04, Matt delco wrote:
>>>>>>>>> From: Matt Delco <delco@xxxxxxxxxx>
>>>>>>>>>
>>>>>>>>> Time seems to eventually stop in a Windows VM when using Skype.
>>>>>>>>> Instrumentation shows that the OS is frequently switching the APIC
>>>>>>>>> timer between one-shot and periodic mode. The OS is typically writing
>>>>>>>>> to both LVTT and TMICT. When time stops the sequence observed is that
>>>>>>>>> the APIC was in one-shot mode, the timer expired, and the OS writes to
>>>>>>>>> LVTT (but not TMICT) to change to periodic mode. No future timer
>>>>>>> events
>>>>>>>>> are received by the OS since the timer is only re-armed on TMICT
>>>>>>> writes.
>>>>>>>>> With this change time continues to advance in the VM. TBD if physical
>>>>>>>>> hardware will reset the current count if/when the mode is changed to
>>>>>>>>> period and the current count is zero.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Matt Delco <delco@xxxxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> arch/x86/kvm/lapic.c | 9 +++++++--
>>>>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>>>>> index 685d17c11461..fddd810eeca5 100644
>>>>>>>>> --- a/arch/x86/kvm/lapic.c
>>>>>>>>> +++ b/arch/x86/kvm/lapic.c
>>>>>>>>> @@ -1935,14 +1935,19 @@ int kvm_lapic_reg_write(struct kvm_lapic
>>>>>>> *apic, u32 reg, u32 val)
>>>>>>>>> break;
>>>>>>>>>
>>>>>>>>> - case APIC_LVTT:
>>>>>>>>> + case APIC_LVTT: {
>>>>>>>>> + u32 timer_mode = apic->lapic_timer.timer_mode;
>>>>>>>>> if (!kvm_apic_sw_enabled(apic))
>>>>>>>>> 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 (timer_mode == APIC_LVT_TIMER_ONESHOT &&
>>>>>>>>> + apic_lvtt_period(apic) &&
>>>>>>>>> + !hrtimer_active(&apic->lapic_timer.timer))
>>>>>>>>> + start_apic_timer(apic);
>>>>>>>>
>>>>>>>> Still, this needs some more explanation. Can you cover this, as well as
>>>>>>>> the oneshot->periodic transition, in kvm-unit-tests' x86/apic.c
>>>>>>>> testcase? Then we could try running it on bare metal and see what
>>>>>>> happens.
>>>>>>
>>>>>> I looked at apic.c and test_apic_change_mode() might already be testing
>>>>>> this. It sets oneshot & TMICT, waits for the current value to get
>>>>>> half-way, changes the mode to periodic, and then tries to test that the
>>>>>> value wraps back to the upper half. It then waits again for the half-way
>>>>>> point, changes the mode back to oneshot, and waits for zero. After
>>>>>> reaching zero it does:
>>>>>>
>>>>>> /* now tmcct == 0 and tmict != 0 */
>>>>>> apic_change_mode(APIC_LVT_TIMER_PERIODIC);
>>>>>> report("TMCCT should stay at zero", !apic_read(APIC_TMCCT));
>>>>>>
>>>>>> which seems to be testing that oneshot->periodic won't reset the timer if
>>>>>> it's already zero. A possible caveat is there's hardly any delay between
>>>>>> the mode change and the timer read. Emulated hardware will react
>>>>>> instantaneously (at least as seen from within the VM), but hardware might
>>>>>> need more time to react (though offhand I'd expect HW to be fast enough for
>>>>>> this particular timer).
>>>>>>
>>>>>> So, it looks like the code might already be ready to run on physical
>>>>>> hardware, and if it has (or does already as part of a regular test), then
>>>>>> that does raise some doubt on what's the appropriate code change to make
>>>>>> this work.
>>>>>
>>>>> Nadav has been running tests on bare metal, maybe he can weigh in on
>>>>> whether or not test_apic_change_mode() passes on bare metal.
>>>>
>>>> These tests pass on bare-metal.
>>>
>>> Good to know this. In addition, in linux apic driver, during mode
>>> switch __setup_APIC_LVTT() always sets lapic_timer_period(number of
>>> clock cycles per jiffy)/APIC_DIVISOR to APIC_TMICT which can avoid the
>>> issue Matt report. So is it because there is no such stuff in windows
>>> or the windows version which Matt testing is too old?
>>
>> I find it kind of disappointing that you (and others) did not try the
>> kvm-unit-tests of bare-metal. :(
>
> Origianlly xen guys confirm the testcase on bare-metal, thanks for
> your double confirm.
No worries, I don’t look for a “thank you” note. ;-)
