Re: [Xen-devel] [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Apr 19, 2017 at 08:17:14AM -0600, Jan Beulich wrote:
>>>> On 19.04.17 at 08:40, <chao.gao@xxxxxxxxx> wrote:
>> @@ -1257,7 +1257,12 @@ void vlapic_reset(struct vlapic *vlapic)
>>      }
>>      vlapic_set_reg(vlapic, APIC_ICR,     0);
>>      vlapic_set_reg(vlapic, APIC_ICR2,    0);
>> -    vlapic_set_reg(vlapic, APIC_LDR,     0);
>> +    /*
>> +     * LDR is read-only in x2APIC mode. Preserve its value when handling
>> +     * INIT signal in x2APIC mode.
>> +     */
>> +    if ( !vlapic_x2apic_mode(vlapic) )
>
>In order for this to work you need to ...
>
>> +        vlapic_set_reg(vlapic, APIC_LDR, 0);
>>      vlapic_set_reg(vlapic, APIC_TASKPRI, 0);
>>      vlapic_set_reg(vlapic, APIC_TMICT,   0);
>>      vlapic_set_reg(vlapic, APIC_TMCCT,   0);
>> @@ -1275,6 +1280,22 @@ void vlapic_reset(struct vlapic *vlapic)
>>      destroy_periodic_time(&vlapic->pt);
>>  }
>>  
>> +/* Reset the VLAPIC back to its power-on/reset state. */
>> +void vlapic_reset(struct vlapic *vlapic)
>> +{
>> +    const struct vcpu *v = vlapic_vcpu(vlapic);
>> +
>> +    if ( !has_vlapic(v->domain) )
>> +        return;
>> +
>> +    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
>> +    vlapic_set_reg(vlapic, APIC_LDR, 0);
>> +    vlapic_do_init(vlapic);
>> +
>> +    vlapic->hw.apic_base_msr &= ~(uint64_t)MSR_IA32_APICBASE_EXTD;
>
>... do this before calling vlapic_do_init() afaict (and I see no strong
>reason why you would need a cast here)). Likely it is a good idea
>then to also ...

Got it and will fix. The APIC_LDR is also cleared in vlapic_reset.
Do you mean it will do a cast automatically? I don't know that before.
I was afraid that the higher qword of ~MSR_IA32_APICBASE_EXTD will is 
zero.

>
>> +    vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_ENABLE;
>
>... do this up front.
>
>> @@ -1489,8 +1510,7 @@ int vlapic_init(struct vcpu *v)
>>  
>>      vlapic_reset(vlapic);
>>  
>> -    vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
>> -                                APIC_DEFAULT_PHYS_BASE);
>> +    vlapic->hw.apic_base_msr |= APIC_DEFAULT_PHYS_BASE;
>
>Perhaps better move this ahead of the call to vlapic_reset() then
>too.
>
>Remains the question (not answered by the SDM afaics): What
>happens to the base address during reset?

Actually, I don't know and that's also why I don't touch apic_base_msr in the
first verson. Will try to get a confirmation from hardware guys.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel