2017-11-18 5:03 GMT+08:00 Liran Alon <LIRAN.ALON@xxxxxxxxxx>: > > > On 17/11/17 23:01, Liran Alon wrote: >> >> >> >> On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote: >>> >>> From: "Dr. David Alan Gilbert" <dgilbert@xxxxxxxxxx> >>> >>> In x2apic mode the LDR is fixed based on the ID rather >>> than separately loadable like it was before x2. >>> When kvm_apic_set_state is called, the base is set, and if >>> it has the X2APIC_ENABLE flag set then the LDR is calculated; >>> however that value gets overwritten by the memcpy a few lines >>> below overwriting it with the value that came from userland. >>> >>> The symptom is a lack of EOI after loading the state >>> (e.g. after a QEMU migration) and is due to the EOI bitmap >>> being wrong due to the incorrect LDR. This was seen with >>> a Win2016 guest under Qemu with irqchip=split whose USB mouse >>> didn't work after a VM migration. >>> >>> This corresponds to RH bug: >>> >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1502591&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=BDEsclkj9SNWbZXiuKgX07QVY0LqtwHA13yqtK4wreE&s=MJS_JxKV0dJS6T8qobO29j530xNJLFqgSuRMP8oEiwI&e= >>> >>> >>> Reported-by: Yiqian Wei <yiwei@xxxxxxxxxx> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@xxxxxxxxxx> >>> --- >>> arch/x86/kvm/lapic.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>> index 4991e9e51611..cff55beb0263 100644 >>> --- a/arch/x86/kvm/lapic.c >>> +++ b/arch/x86/kvm/lapic.c >>> @@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu >>> *vcpu, >>> { >>> if (apic_x2apic_mode(vcpu->arch.apic)) { >>> u32 *id = (u32 *)(s->regs + APIC_ID); >>> + u32 *ldr = (u32 *)(s->regs + APIC_LDR); >>> >>> + if (set) { >>> + /* In x2 the LDR is fixed based on the id */ >>> + *ldr = kvm_apic_calc_x2apic_ldr(*id); >>> + } >>> if (vcpu->kvm->arch.x2apic_format) { >>> if (*id != vcpu->vcpu_id) >>> return -EINVAL; >>> >> >> I think there is a bug here of not adding the new code in the right >> place. I think diff should be instead: >> >> @@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu >> *vcpu, >> { >> if (apic_x2apic_mode(vcpu->arch.apic)) { >> u32 *id = (u32 *)(s->regs + APIC_ID); >> + u32 *ldr = (u32 *)(s->regs + APIC_LDR); >> >> if (vcpu->kvm->arch.x2apic_format) { >> if (*id != vcpu->vcpu_id) >> @@ -2255,6 +2256,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu >> *vcpu, >> else >> *id <<= 24; >> } >> + >> + *ldr = kvm_apic_calc_x2apic_ldr(*id); >> } >> >> return 0; >> >> This is because of the x2apic_format hack. >> (Fore more info, see commit 3713131345fb ("KVM: x86: add >> KVM_CAP_X2APIC_API")). >> Otherwise, you will use a value which can be shifted-left by 24. >> >> -Liran > > > Sorry I meant diff should be: > @@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, > { > if (apic_x2apic_mode(vcpu->arch.apic)) { > u32 *id = (u32 *)(s->regs + APIC_ID); > + u32 *ldr = (u32 *)(s->regs + APIC_LDR); > > if (vcpu->kvm->arch.x2apic_format) { > if (*id != vcpu->vcpu_id) > @@ -2255,6 +2256,9 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, > else > *id <<= 24; > } > + > + if (set) > > + *ldr = kvm_apic_calc_x2apic_ldr(*id); Why not move this to the else branch of vcpu->kvm->arch.x2apic_format since LDR has already stored 32-bit logical x2APIC ID in x2apic mode? Regards, Wanpeng Li > } > > return 0;
