[PATCH] KVM: x86: Fix emulation in writing cr8

In emulation of writing to cr8, one of the lowest four bits in TPR[3:0]
is kept.

According to Intel SDM 10.8.6.1(baremetal scenario):
"APIC.TPR[bits 7:4] = CR8[bits 3:0], APIC.TPR[bits 3:0] = 0";

and SDM 28.3(use TPR shadow):
"MOV to CR8. The instruction stores bits 3:0 of its source operand into
bits 7:4 of VTPR; the remainder of VTPR (bits 3:0 and bits 31:8) are
cleared.";

so in KVM emulated scenario, clear TPR[3:0] to make a consistent behavior
as in other scenarios.

This doesn't impact evaluation and delivery of pending virtual interrupts
because processor does not use the processor-priority sub-class to
determine which interrupts to delivery and which to inhibit.

Signed-off-by: Zhenzhong Duan <redacted>
---
 arch/x86/kvm/lapic.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d7e6fde82d25..306025db9959 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c

@@ -2242,10 +2242,7 @@ void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data)
 
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
-
-	apic_set_tpr(apic, ((cr8 & 0x0f) << 4)
-		     | (kvm_lapic_get_reg(apic, APIC_TASKPRI) & 4));
+	apic_set_tpr(vcpu->arch.apic, (cr8 & 0x0f) << 4);
 }
 
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)

-- 
2.25.1

On Wed, Feb 09, 2022, Zhenzhong Duan wrote:

In emulation of writing to cr8, one of the lowest four bits in TPR[3:0]
is kept.

According to Intel SDM 10.8.6.1(baremetal scenario):
"APIC.TPR[bits 7:4] = CR8[bits 3:0], APIC.TPR[bits 3:0] = 0";

and SDM 28.3(use TPR shadow):
"MOV to CR8. The instruction stores bits 3:0 of its source operand into
bits 7:4 of VTPR; the remainder of VTPR (bits 3:0 and bits 31:8) are
cleared.";

so in KVM emulated scenario, clear TPR[3:0] to make a consistent behavior
as in other scenarios.

AMD's APM agrees:

  Task Priority Sub-class (TPS)—Bits 3 : 0. The TPS field indicates the current
  sub-priority to be used when arbitrating lowest-priority messages. This field
  is written with zero when TPR is written using the architectural CR8 register.

This doesn't impact evaluation and delivery of pending virtual interrupts
because processor does not use the processor-priority sub-class to
determine which interrupts to delivery and which to inhibit.

I believe hardware uses it to arbitrate lowest priority interrupts, but KVM just
does a round-robin style delivery.

Signed-off-by: Zhenzhong Duan <redacted>

Probably worth:

  Fixes: b93463aa59d6 ("KVM: Accelerated apic support")

Reviewed-by: Sean Christopherson <seanjc@google.com>

This appears to have been deliberate, but I've no idea what on earth it was
trying to do.  Preserving only bit 2 is super weird.

Author: Avi Kivity [off-list ref]
Date:   Thu Oct 25 16:52:32 2007 +0200

    KVM: Accelerated apic support

    This adds a mechanism for exposing the virtual apic tpr to the guest, and a
    protocol for letting the guest update the tpr without causing a vmexit if
    conditions allow (e.g. there is no interrupt pending with a higher priority
    than the new tpr).

    Signed-off-by: Avi Kivity [off-list ref]

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 50c3f3a8dd3d..e7513bb98af1 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c

@@ -815,7 +815,8 @@ void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)

        if (!apic)
                return;
-       apic_set_tpr(apic, ((cr8 & 0x0f) << 4));
+       apic_set_tpr(apic, ((cr8 & 0x0f) << 4)
+                    | (apic_get_reg(apic, APIC_TASKPRI) & 4));
 }