[v2,1/4] KVM: LAPIC: Fix lapic timer mode transition

Commit Message

Wanpeng Li Sept. 29, 2017, 1:04 a.m. UTC

From: Wanpeng Li <[e-mail]>

SDM 10.5.4.1 TSC-Deadline Mode mentioned that "Transitioning between TSC-Deadline
mode and other timer modes also disarms the timer". So the APIC Timer Initial Count
Register for one-shot/periodic mode should be reset. This patch do it.

Cc: Paolo Bonzini <[e-mail]>
Cc: Radim Krčmář <[e-mail]>
Signed-off-by: Wanpeng Li <[e-mail]>
---
 arch/x86/include/asm/apicdef.h | 1 +
 arch/x86/kvm/lapic.c           | 3 +++
 2 files changed, 4 insertions(+)

Comments

Radim Krčmář Oct. 3, 2017, 5:05 p.m. UTC | #1

2017-09-28 18:04-0700, Wanpeng Li:
> From: Wanpeng Li <[e-mail]>
> 
> SDM 10.5.4.1 TSC-Deadline Mode mentioned that "Transitioning between TSC-Deadline
> mode and other timer modes also disarms the timer". So the APIC Timer Initial Count
> Register for one-shot/periodic mode should be reset. This patch do it.

At the beginning of the secion is also:

  A write to the LVT Timer Register that changes the timer mode disarms
  the local APIC timer. The supported timer modes are given in Table
  10-2. The three modes of the local APIC timer are mutually exclusive.

So we should also disarm when switching between one-shot and periodic.

apic_update_lvtt() already has logic to determine whether the timer mode
has changed and is the perfect place to clear APIC_TMICT.

Thanks.

> Cc: Paolo Bonzini <[e-mail]>
> Cc: Radim Krčmář <[e-mail]>
> Signed-off-by: Wanpeng Li <[e-mail]>
> ---
>  arch/x86/include/asm/apicdef.h | 1 +
>  arch/x86/kvm/lapic.c           | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
> index c46bb99..d8ef1b4 100644
> --- a/arch/x86/include/asm/apicdef.h
> +++ b/arch/x86/include/asm/apicdef.h
> @@ -100,6 +100,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/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 69c5612..a739cbb 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1722,6 +1722,9 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>  		break;
>  
>  	case APIC_LVTT:
> +		if (apic_lvtt_tscdeadline(apic) != ((val &
> +			APIC_LVT_TIMER_MASK) == APIC_LVT_TIMER_TSCDEADLINE))
> +			kvm_lapic_set_reg(apic, APIC_TMICT, 0);
>  		if (!kvm_apic_sw_enabled(apic))
>  			val |= APIC_LVT_MASKED;
>  		val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask);
> -- 
> 2.7.4
>

Wanpeng Li Oct. 4, 2017, 1:45 a.m. UTC | #2

2017-10-04 1:05 GMT+08:00 Radim Krčmář <[e-mail]>:
> 2017-09-28 18:04-0700, Wanpeng Li:
>> From: Wanpeng Li <[e-mail]>
>>
>> SDM 10.5.4.1 TSC-Deadline Mode mentioned that "Transitioning between TSC-Deadline
>> mode and other timer modes also disarms the timer". So the APIC Timer Initial Count
>> Register for one-shot/periodic mode should be reset. This patch do it.
>
> At the beginning of the secion is also:
>
>   A write to the LVT Timer Register that changes the timer mode disarms
>   the local APIC timer. The supported timer modes are given in Table
>   10-2. The three modes of the local APIC timer are mutually exclusive.

Yeah, I saw it before sending out the patches, but it is mentioned in
TSC-deadline section which looks strange, if the timer is still
disarmed when switching between one-shot and periodic mode before
TSC-deadline is introduced and w/o TSC-deadline section？

>
> So we should also disarm when switching between one-shot and periodic.
>
> apic_update_lvtt() already has logic to determine whether the timer mode
> has changed and is the perfect place to clear APIC_TMICT.

Agreed, thanks for your review, Radim. :)

Regards,
Wanpeng Li

>
> Thanks.
>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Radim Krčmář <[e-mail]>
>> Signed-off-by: Wanpeng Li <[e-mail]>
>> ---
>>  arch/x86/include/asm/apicdef.h | 1 +
>>  arch/x86/kvm/lapic.c           | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
>> index c46bb99..d8ef1b4 100644
>> --- a/arch/x86/include/asm/apicdef.h
>> +++ b/arch/x86/include/asm/apicdef.h
>> @@ -100,6 +100,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/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 69c5612..a739cbb 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1722,6 +1722,9 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>>               break;
>>
>>       case APIC_LVTT:
>> +             if (apic_lvtt_tscdeadline(apic) != ((val &
>> +                     APIC_LVT_TIMER_MASK) == APIC_LVT_TIMER_TSCDEADLINE))
>> +                     kvm_lapic_set_reg(apic, APIC_TMICT, 0);
>>               if (!kvm_apic_sw_enabled(apic))
>>                       val |= APIC_LVT_MASKED;
>>               val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask);
>> --
>> 2.7.4
>>

Radim Krčmář Oct. 4, 2017, 1:21 p.m. UTC | #3

2017-10-04 09:45+0800, Wanpeng Li:
> 2017-10-04 1:05 GMT+08:00 Radim Krčmář <[e-mail]>:
> > 2017-09-28 18:04-0700, Wanpeng Li:
> >> From: Wanpeng Li <[e-mail]>
> >>
> >> SDM 10.5.4.1 TSC-Deadline Mode mentioned that "Transitioning between TSC-Deadline
> >> mode and other timer modes also disarms the timer". So the APIC Timer Initial Count
> >> Register for one-shot/periodic mode should be reset. This patch do it.
> >
> > At the beginning of the secion is also:
> >
> >   A write to the LVT Timer Register that changes the timer mode disarms
> >   the local APIC timer. The supported timer modes are given in Table
> >   10-2. The three modes of the local APIC timer are mutually exclusive.
> 
> Yeah, I saw it before sending out the patches, but it is mentioned in
> TSC-deadline section which looks strange, if the timer is still
> disarmed when switching between one-shot and periodic mode before
> TSC-deadline is introduced and w/o TSC-deadline section？

Yeah, maybe it is only true if the machine has TSC.  APM doesn't mention
disarming at all.  Bochs only disables the timer it on switch from/to
TSC-deadline.

> > So we should also disarm when switching between one-shot and periodic.
> >
> > apic_update_lvtt() already has logic to determine whether the timer mode
> > has changed and is the perfect place to clear APIC_TMICT.
> 
> Agreed, thanks for your review, Radim. :)

Bochs doesn't write 0 to APIC_TMICT, but it seems that Xen guys verified
that on bare-metal, so the behavior is fine.
Please just move it to apic_update_lvtt(),

thanks.

Wanpeng Li Oct. 4, 2017, 1:50 p.m. UTC | #4

2017-10-04 21:21 GMT+08:00 Radim Krčmář <[e-mail]>:
> 2017-10-04 09:45+0800, Wanpeng Li:
>> 2017-10-04 1:05 GMT+08:00 Radim Krčmář <[e-mail]>:
>> > 2017-09-28 18:04-0700, Wanpeng Li:
>> >> From: Wanpeng Li <[e-mail]>
>> >>
>> >> SDM 10.5.4.1 TSC-Deadline Mode mentioned that "Transitioning between TSC-Deadline
>> >> mode and other timer modes also disarms the timer". So the APIC Timer Initial Count
>> >> Register for one-shot/periodic mode should be reset. This patch do it.
>> >
>> > At the beginning of the secion is also:
>> >
>> >   A write to the LVT Timer Register that changes the timer mode disarms
>> >   the local APIC timer. The supported timer modes are given in Table
>> >   10-2. The three modes of the local APIC timer are mutually exclusive.
>>
>> Yeah, I saw it before sending out the patches, but it is mentioned in
>> TSC-deadline section which looks strange, if the timer is still
>> disarmed when switching between one-shot and periodic mode before
>> TSC-deadline is introduced and w/o TSC-deadline section？
>
> Yeah, maybe it is only true if the machine has TSC.  APM doesn't mention

If APM is another emulator?

> disarming at all.  Bochs only disables the timer it on switch from/to
> TSC-deadline.
>
>> > So we should also disarm when switching between one-shot and periodic.
>> >
>> > apic_update_lvtt() already has logic to determine whether the timer mode
>> > has changed and is the perfect place to clear APIC_TMICT.
>>
>> Agreed, thanks for your review, Radim. :)
>
> Bochs doesn't write 0 to APIC_TMICT, but it seems that Xen guys verified
> that on bare-metal, so the behavior is fine.
> Please just move it to apic_update_lvtt(),
>
> thanks.

Ok, thanks for the review. :)

Regards,
Wanpeng Li

Wanpeng Li Oct. 4, 2017, 2:02 p.m. UTC | #5

2017-10-04 21:50 GMT+08:00 Wanpeng Li <[e-mail]>:
> 2017-10-04 21:21 GMT+08:00 Radim Krčmář <[e-mail]>:
>> 2017-10-04 09:45+0800, Wanpeng Li:
>>> 2017-10-04 1:05 GMT+08:00 Radim Krčmář <[e-mail]>:
>>> > 2017-09-28 18:04-0700, Wanpeng Li:
>>> >> From: Wanpeng Li <[e-mail]>
>>> >>
>>> >> SDM 10.5.4.1 TSC-Deadline Mode mentioned that "Transitioning between TSC-Deadline
>>> >> mode and other timer modes also disarms the timer". So the APIC Timer Initial Count
>>> >> Register for one-shot/periodic mode should be reset. This patch do it.
>>> >
>>> > At the beginning of the secion is also:
>>> >
>>> >   A write to the LVT Timer Register that changes the timer mode disarms
>>> >   the local APIC timer. The supported timer modes are given in Table
>>> >   10-2. The three modes of the local APIC timer are mutually exclusive.
>>>
>>> Yeah, I saw it before sending out the patches, but it is mentioned in
>>> TSC-deadline section which looks strange, if the timer is still
>>> disarmed when switching between one-shot and periodic mode before
>>> TSC-deadline is introduced and w/o TSC-deadline section？
>>
>> Yeah, maybe it is only true if the machine has TSC.  APM doesn't mention
>
> If APM is another emulator?

The document from AMD.

>
>> disarming at all.  Bochs only disables the timer it on switch from/to
>> TSC-deadline.
>>
>>> > So we should also disarm when switching between one-shot and periodic.
>>> >
>>> > apic_update_lvtt() already has logic to determine whether the timer mode
>>> > has changed and is the perfect place to clear APIC_TMICT.
>>>
>>> Agreed, thanks for your review, Radim. :)
>>
>> Bochs doesn't write 0 to APIC_TMICT, but it seems that Xen guys verified
>> that on bare-metal, so the behavior is fine.
>> Please just move it to apic_update_lvtt(),
>>
>> thanks.
>
> Ok, thanks for the review. :)
>
> Regards,
> Wanpeng Li

Patch

diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index c46bb99..d8ef1b4 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -100,6 +100,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/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 69c5612..a739cbb 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1722,6 +1722,9 @@  int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 
 	case APIC_LVTT:
+		if (apic_lvtt_tscdeadline(apic) != ((val &
+			APIC_LVT_TIMER_MASK) == APIC_LVT_TIMER_TSCDEADLINE))
+			kvm_lapic_set_reg(apic, APIC_TMICT, 0);
 		if (!kvm_apic_sw_enabled(apic))
 			val |= APIC_LVT_MASKED;
 		val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask);