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

[Liran Alon <LIRAN.ALON@ORACLE.COM>]

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);
         }

         return 0;