Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Liran Alon (LIRAN.ALON@ORACLE.COM) 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@redhat.com>
> > > 
> > > 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@redhat.com>
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >   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@redhat.com / Manchester, UK