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
