From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic Date: Fri, 17 Nov 2017 23:01:19 +0200 Message-ID: <5A0F4E1F.70303@ORACLE.COM> References: <20171117115250.7564-1-dgilbert@redhat.com> <20171117115250.7564-3-dgilbert@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com To: "Dr. David Alan Gilbert (git)" , kvm@vger.kernel.org Return-path: Received: from userp1040.oracle.com ([156.151.31.81]:46982 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965376AbdKQVBY (ORCPT ); Fri, 17 Nov 2017 16:01:24 -0500 In-Reply-To: <20171117115250.7564-3-dgilbert@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > 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 > Signed-off-by: Dr. David Alan Gilbert > --- > 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