re: [PATCH] svm: Fix AVIC incomplete IPI emulation

[Oren Twaig <oren@scalemp.com>]

Hello Suravee,

According to AMD's SDM, the target-not-running incomplete
ipi exit is only received if any of the destination cpus had the

not-running bit set in the avic backing page. However, not

before the CPU _already_ set the relevant IRR bit in all these cpus.


In this change, the patch forces KVM to send another interrupt

to the vcpu whether SVM already did that or not. Which means

the vcpu/s, under some conditions, can get an EXTRA interrupt

it never intended to get.


Example:
   1. vcpu B: Is in "not-running" state.
   2. vcpu A: Writes to the ICR to send vector 80 to vcpu B
   3. vcpu A: SVM updates vcpu B IRR with bit 80
   4. vcpu A: SVM exits on incomplete IPI target-not-running exit.
   5. vcpu A: Now stops executing any code @ hypervisor level.
   6. vcpu B: Due to another interrupt (like lapic timer)
      resumes running the guest. While handling interrupts,
      it also handles interrupt vector 80 (as it's in his IRR)
   7. vcpu A: resumes executing the below code and sends
      an _additional_interrupt to vcpu B.

Overall, vcpu B got two interrupts. The second is unwanted and
not documented in the system architecture.

Can you please elaborate more to why the implementation

below conflict with the specifications (which was the code

before this commit) ?


Thanks,
Oren Twaig


 > From    "Suthikulpanit, Suravee" <>
 > Subject    [PATCH] svm: Fix AVIC incomplete IPI emulation
 > Date    Tue, 22 Jan 2019 10:25:13 +0000
 > share
 > From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
 >
 > In case of incomplete IPI with invalid interrupt type, the current
 > SVM driver does not properly emulate the IPI, and fails to boot
 > FreeBSD guests with multiple vcpus when enabling AVIC.
 >
 > Fix this by update APIC ICR high/low registers, which also
 > emulate sending the IPI.
 >
 > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
 > ---
 > arch/x86/kvm/svm.c | 19 ++++---------------
 > 1 file changed, 4 insertions(+), 15 deletions(-)
 >
 > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
 > index 2aff835a65ed..8a0c9a1f6ac8 100644
 > --- a/arch/x86/kvm/svm.c
 > +++ b/arch/x86/kvm/svm.c
 > @@ -4504,25 +4504,14 @@ static int 
avic_incomplete_ipi_interception(struct vcpu_svm *svm)
 >          kvm_lapic_reg_write(apic, APIC_ICR, icrl);
 >          break;
 >      case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING: {
 > -        int i;
 > -        struct kvm_vcpu *vcpu;
 > -        struct kvm *kvm = svm->vcpu.kvm;
 >          struct kvm_lapic *apic = svm->vcpu.arch.apic;
 >
          /*
 > -         * At this point, we expect that the AVIC HW has already
 > -         * set the appropriate IRR bits on the valid target
 > -         * vcpus. So, we just need to kick the appropriate vcpu.
 > +         * Update ICR high and low, then emulate sending IPI,
 > +         * which is handled when writing APIC_ICR.
 >           */
 > -        kvm_for_each_vcpu(i, vcpu, kvm) {
 > -            bool m = kvm_apic_match_dest(vcpu, apic,
 > -                             icrl & KVM_APIC_SHORT_MASK,
 > - GET_APIC_DEST_FIELD(icrh),
 > -      icrl & KVM_APIC_DEST_MASK);
 > -
 > -            if (m && !avic_vcpu_is_running(vcpu))
 > - kvm_vcpu_wake_up(vcpu);
 > -        }
 > +        kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
 > +        kvm_lapic_reg_write(apic, APIC_ICR, icrl);
 >          break;
 >      }
 >      case AVIC_IPI_FAILURE_INVALID_TARGET:
 > --
 > 2.17.1