[PATCH v2 1/3] KVM: LAPIC: Write 0 to TMICT should also cancel vmx-preemption timer

[PATCH v2 1/3] KVM: LAPIC: Write 0 to TMICT should also cancel vmx-preemption timer

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

According to the SDM 10.5.4.1:

  A write of 0 to the initial-count register effectively stops the local
  APIC timer, in both one-shot and periodic mode.

However, the lapic timer oneshot/periodic mode which is emulated by vmx-preemption
timer doesn't stop by writing 0 to TMICT since vmx->hv_deadline_tsc is still
programmed and the guest will receive the spurious timer interrupt later. This
patch fixes it by also cancelling the vmx-preemption timer when writing 0 to
the initial-count register.

Reviewed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
v1 -> v2:
 * rename to cancel_apic_timer 
 * update patch description

 arch/x86/kvm/lapic.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 8120e86..6d72d8f 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1494,6 +1494,15 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic)
 
 static void cancel_hv_timer(struct kvm_lapic *apic);
 
+static void cancel_apic_timer(struct kvm_lapic *apic)
+{
+	hrtimer_cancel(&apic->lapic_timer.timer);
+	preempt_disable();
+	if (apic->lapic_timer.hv_timer_in_use)
+		cancel_hv_timer(apic);
+	preempt_enable();
+}
+
 static void apic_update_lvtt(struct kvm_lapic *apic)
 {
 	u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) &
@@ -1502,11 +1511,7 @@ static void apic_update_lvtt(struct kvm_lapic *apic)
 	if (apic->lapic_timer.timer_mode != timer_mode) {
 		if (apic_lvtt_tscdeadline(apic) != (timer_mode ==
 				APIC_LVT_TIMER_TSCDEADLINE)) {
-			hrtimer_cancel(&apic->lapic_timer.timer);
-			preempt_disable();
-			if (apic->lapic_timer.hv_timer_in_use)
-				cancel_hv_timer(apic);
-			preempt_enable();
+			cancel_apic_timer(apic);
 			kvm_lapic_set_reg(apic, APIC_TMICT, 0);
 			apic->lapic_timer.period = 0;
 			apic->lapic_timer.tscdeadline = 0;
@@ -2092,7 +2097,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		if (apic_lvtt_tscdeadline(apic))
 			break;
 
-		hrtimer_cancel(&apic->lapic_timer.timer);
+		cancel_apic_timer(apic);
 		kvm_lapic_set_reg(apic, APIC_TMICT, val);
 		start_apic_timer(apic);
 		break;
-- 
2.7.4

[PATCH v2 2/3] KVM: LAPIC: Reset TMCCT during vCPU reset

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

The value of the current counter register after reset is 0 for both 
Intel and AMD, let's do it in kvm, though, the TMCCT is always computed
on-demand and never directly readable.

Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
v1 -> v2:
 * update patch description

 arch/x86/kvm/lapic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6d72d8f..cbfdecd 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);
-- 
2.7.4

Re: [PATCH v2 2/3] KVM: LAPIC: Reset TMCCT during vCPU reset

[Paolo Bonzini]

On 07/06/21 09:19, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> The value of the current counter register after reset is 0 for both
> Intel and AMD, let's do it in kvm, though, the TMCCT is always computed
> on-demand and never directly readable.

It's useless though since it's never read except by KVM_SET_LAPIC. 
Perhaps instead set TMCCT to 0 in kvm_apic_set_state, instead of keeping 
the value that was filled in by KVM_GET_LAPIC?

Paolo

> Reviewed-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> v1 -> v2:
>   * update patch description
> 
>   arch/x86/kvm/lapic.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6d72d8f..cbfdecd 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);
> 

Re: [PATCH v2 2/3] KVM: LAPIC: Reset TMCCT during vCPU reset

[Wanpeng Li]

On Wed, 9 Jun 2021 at 00:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
[...]
> Perhaps instead set TMCCT to 0 in kvm_apic_set_state, instead of keeping
> the value that was filled in by KVM_GET_LAPIC?

Keeping the value that was filled in by KVM_GET_LAPIC is introduced by
commit 24647e0a39b6 (KVM: x86: Return updated timer current count
register from KVM_GET_LAPIC), could you elaborate more? :)

    Wanpeng

Re: [PATCH v2 2/3] KVM: LAPIC: Reset TMCCT during vCPU reset

[Paolo Bonzini]

On 09/06/21 04:15, Wanpeng Li wrote:
> On Wed, 9 Jun 2021 at 00:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
> [...]
>> Perhaps instead set TMCCT to 0 in kvm_apic_set_state, instead of keeping
>> the value that was filled in by KVM_GET_LAPIC?
> 
> Keeping the value that was filled in by KVM_GET_LAPIC is introduced by
> commit 24647e0a39b6 (KVM: x86: Return updated timer current count
> register from KVM_GET_LAPIC), could you elaborate more? :)

KVM_GET_LAPIC stores the current value of TMCCT and KVM_SET_LAPIC's 
memcpy stores it in vcpu->arch.apic->regs.  KVM_SET_LAPIC perhaps could 
store zero in vcpu->arch.apic->regs after it uses it, and then the 
stored value would always be zero.

Paolo

Re: [PATCH v2 2/3] KVM: LAPIC: Reset TMCCT during vCPU reset

[Wanpeng Li]

On Wed, 9 Jun 2021 at 13:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 09/06/21 04:15, Wanpeng Li wrote:
> > On Wed, 9 Jun 2021 at 00:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > [...]
> >> Perhaps instead set TMCCT to 0 in kvm_apic_set_state, instead of keeping
> >> the value that was filled in by KVM_GET_LAPIC?
> >
> > Keeping the value that was filled in by KVM_GET_LAPIC is introduced by
> > commit 24647e0a39b6 (KVM: x86: Return updated timer current count
> > register from KVM_GET_LAPIC), could you elaborate more? :)
>
> KVM_GET_LAPIC stores the current value of TMCCT and KVM_SET_LAPIC's
> memcpy stores it in vcpu->arch.apic->regs.  KVM_SET_LAPIC perhaps could
> store zero in vcpu->arch.apic->regs after it uses it, and then the
> stored value would always be zero.

Just do it in a new version, thanks. :)

   Wanpeng

[PATCH v2 3/3] KVM: X86: Let's harden the ipi fastpath condition edge-trigger mode

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

Let's harden the ipi fastpath condition edge-trigger mode.

Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b594275..dbd3e9d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1922,6 +1922,7 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
 		return 1;
 
 	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
+		((data & APIC_INT_LEVELTRIG) == 0) &&
 		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
 		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
 		((u32)(data >> 32) != X2APIC_BROADCAST)) {
-- 
2.7.4

Re: [PATCH v2 3/3] KVM: X86: Let's harden the ipi fastpath condition edge-trigger mode

[Paolo Bonzini]

On 07/06/21 09:19, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Let's harden the ipi fastpath condition edge-trigger mode.

This is not a good commit message...  And if it's a bug, it needs a 
kvm-unit-tests testcase.

Paolo

> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>   arch/x86/kvm/x86.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275..dbd3e9d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1922,6 +1922,7 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
>   		return 1;
>   
>   	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
> +		((data & APIC_INT_LEVELTRIG) == 0) &&
>   		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
>   		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
>   		((u32)(data >> 32) != X2APIC_BROADCAST)) {
> 

Re: [PATCH v2 3/3] KVM: X86: Let's harden the ipi fastpath condition edge-trigger mode

[Sean Christopherson]

On Mon, Jun 07, 2021, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Let's harden the ipi fastpath condition edge-trigger mode.

Can you elaborate on the motivation for this patch?

Intel's SDM states that the trigger mode is ignored for all IPIs except INIT,
and even clarifies that the local xAPIC will override the bit and send the IPI
as edge-triggered.

AMD's APM on the other hand explicitly lists level-triggered Fixed IPIs as a
valid ICR combination.

Regardless of which of the two conflicting specs we want KVM to emulate (which
is currently AMD), I don't see why the fastpath code should care, as I can't
find anything in the kvm_apic_send_ipi() path that would go awry if it's called
from the fastpath for a level-triggered IPI.

Related side topic, anyone happen to know if KVM (and Qemu's) emulation of IPIs
intentionally follows AMD instead of Intel?  I suspect it's unintentional,
especially since KVM's initial xAPIC emulation came from Intel.  Not that it's
likely to matter, but allowing level-triggered IPIs is bizarre, e.g. getting an
EOI sent to the right I/O APIC at the right time via a level-triggered IPI seems
extremely convoluted.

> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/x86.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275..dbd3e9d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1922,6 +1922,7 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
>  		return 1;
>  
>  	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
> +		((data & APIC_INT_LEVELTRIG) == 0) &&
>  		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
>  		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
>  		((u32)(data >> 32) != X2APIC_BROADCAST)) {
> -- 
> 2.7.4
> 

Re: [PATCH v2 3/3] KVM: X86: Let's harden the ipi fastpath condition edge-trigger mode

[Paolo Bonzini]

On 08/06/21 18:35, Sean Christopherson wrote:
> Related side topic, anyone happen to know if KVM (and Qemu's) emulation of IPIs
> intentionally follows AMD instead of Intel?  I suspect it's unintentional,
> especially since KVM's initial xAPIC emulation came from Intel.  Not that it's
> likely to matter, but allowing level-triggered IPIs is bizarre, e.g. getting an
> EOI sent to the right I/O APIC at the right time via a level-triggered IPI seems
> extremely convoluted.

QEMU traditionally followed AMD a bit more than Intel for historical 
reasons.  Probably the code went QEMU->Xen->KVM even though it was 
contributed by Intel.

Paolo

Re: [PATCH v2 3/3] KVM: X86: Let's harden the ipi fastpath condition edge-trigger mode

[Wanpeng Li]

On Wed, 9 Jun 2021 at 00:35, Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Jun 07, 2021, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Let's harden the ipi fastpath condition edge-trigger mode.
>
> Can you elaborate on the motivation for this patch?
>
> Intel's SDM states that the trigger mode is ignored for all IPIs except INIT,
> and even clarifies that the local xAPIC will override the bit and send the IPI
> as edge-triggered.
>
> AMD's APM on the other hand explicitly lists level-triggered Fixed IPIs as a
> valid ICR combination.
>
> Regardless of which of the two conflicting specs we want KVM to emulate (which
> is currently AMD), I don't see why the fastpath code should care, as I can't
> find anything in the kvm_apic_send_ipi() path that would go awry if it's called
> from the fastpath for a level-triggered IPI.

Fair enough.

    Wanpeng

Re: [PATCH v2 1/3] KVM: LAPIC: Write 0 to TMICT should also cancel vmx-preemption timer

[Paolo Bonzini]

On 07/06/21 09:19, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> According to the SDM 10.5.4.1:
> 
>    A write of 0 to the initial-count register effectively stops the local
>    APIC timer, in both one-shot and periodic mode.
> 
> However, the lapic timer oneshot/periodic mode which is emulated by vmx-preemption
> timer doesn't stop by writing 0 to TMICT since vmx->hv_deadline_tsc is still
> programmed and the guest will receive the spurious timer interrupt later. This
> patch fixes it by also cancelling the vmx-preemption timer when writing 0 to
> the initial-count register.
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> v1 -> v2:
>   * rename to cancel_apic_timer
>   * update patch description
> 
>   arch/x86/kvm/lapic.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 8120e86..6d72d8f 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1494,6 +1494,15 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic)
>   
>   static void cancel_hv_timer(struct kvm_lapic *apic);
>   
> +static void cancel_apic_timer(struct kvm_lapic *apic)
> +{
> +	hrtimer_cancel(&apic->lapic_timer.timer);
> +	preempt_disable();
> +	if (apic->lapic_timer.hv_timer_in_use)
> +		cancel_hv_timer(apic);
> +	preempt_enable();
> +}
> +
>   static void apic_update_lvtt(struct kvm_lapic *apic)
>   {
>   	u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) &
> @@ -1502,11 +1511,7 @@ static void apic_update_lvtt(struct kvm_lapic *apic)
>   	if (apic->lapic_timer.timer_mode != timer_mode) {
>   		if (apic_lvtt_tscdeadline(apic) != (timer_mode ==
>   				APIC_LVT_TIMER_TSCDEADLINE)) {
> -			hrtimer_cancel(&apic->lapic_timer.timer);
> -			preempt_disable();
> -			if (apic->lapic_timer.hv_timer_in_use)
> -				cancel_hv_timer(apic);
> -			preempt_enable();
> +			cancel_apic_timer(apic);
>   			kvm_lapic_set_reg(apic, APIC_TMICT, 0);
>   			apic->lapic_timer.period = 0;
>   			apic->lapic_timer.tscdeadline = 0;
> @@ -2092,7 +2097,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>   		if (apic_lvtt_tscdeadline(apic))
>   			break;
>   
> -		hrtimer_cancel(&apic->lapic_timer.timer);
> +		cancel_apic_timer(apic);
>   		kvm_lapic_set_reg(apic, APIC_TMICT, val);
>   		start_apic_timer(apic);
>   		break;
> 

Queued this one, thanks.

Paolo