* Liran Alon (LIRAN.ALON@xxxxxxxxxx) wrote: > > > 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); Yes thank you; you're right, and this fixes the problem I'd spotted after I'd posted it with a Linux guest. Windows: [ 1539.833110] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=1 ldr=1 [ 1539.833204] kvm_apic_state_fixup: x2apic set id=1 x2apic_format=1 ldr=2 Linux: [ 1926.724608] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=0 ldr=1 [ 1926.724734] kvm_apic_state_fixup: x2apic set id=1000000 x2apic_format=0 ldr=1 [ 1926.724854] kvm_apic_state_fixup: x2apic set id=2000000 x2apic_format=0 ldr=1 [ 1926.724988] kvm_apic_state_fixup: x2apic set id=3000000 x2apic_format=0 ldr=1 and you see my original patch messed up the LDR in that case. Moving the *ldr to the bottom we get: Linux: [ 2296.390962] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=0 ldr=1 [ 2296.391134] kvm_apic_state_fixup: x2apic set id=1 x2apic_format=0 ldr=2 [ 2296.391254] kvm_apic_state_fixup: x2apic set id=2 x2apic_format=0 ldr=4 [ 2296.391374] kvm_apic_state_fixup: x2apic set id=3 x2apic_format=0 ldr=8 Windows: [ 2431.062751] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=1 ldr=1 [ 2431.062903] kvm_apic_state_fixup: x2apic set id=1 x2apic_format=1 ldr=2 Dave > } > > return 0; -- Dr. David Alan Gilbert / dgilbert@xxxxxxxxxx / Manchester, UK
