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

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

On 20/11/17 02:53, Wanpeng Li wrote:
> 2017-11-18 5:03 GMT+08:00 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);
>
> 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;