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

[Oren Twaig]

Hi,


Any help appreciated..


Thanks,

Oren Twaig

On 3/4/2019 8:15 PM, Oren Twaig wrote:
> 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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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