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: Mon, 20 Nov 2017 03:04:00 +0200 Message-ID: <5A122A00.3020105@ORACLE.COM> References: <20171117115250.7564-1-dgilbert@redhat.com> <20171117115250.7564-3-dgilbert@redhat.com> <5A0F4E1F.70303@ORACLE.COM> <5A0F4EA0.6080906@ORACLE.COM> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "Dr. David Alan Gilbert (git)" , kvm , Paolo Bonzini To: Wanpeng Li Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:34558 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750952AbdKTBEI (ORCPT ); Sun, 19 Nov 2017 20:04:08 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 20/11/17 02:53, Wanpeng Li wrote: > 2017-11-18 5:03 GMT+08:00 Liran Alon : >> >> >> 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" >>>> >>>> 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 >> >> >> 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? First of all, I understand we agree that what I mentioned is indeed a bug in original commit? Second, if I understood original commit purpose correctly, you want to make sure s->regs APIC_LDR value is correct and matches the s->regs APIC_ID value. Instead of trusting userspace providing these values synced correctly. Therefore, you should re-calc LDR regardless of if x2apic_format was used by userspace or not. BTW, I would expect that if this is the intent, there should also be a commit which adds the "if (*id != vcpu->vcpu_id)" check to the case of the non-x2apic_format after value was shifted-right by 24. Regards, -Liran > > Regards, > Wanpeng Li > >> } >> >> return 0;