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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Apr 19, 2017 at 02:48:57AM -0600, Jan Beulich wrote:
>>>> On 18.04.17 at 23:51, <chao.gao@xxxxxxxxx> wrote:
>> According to SDM "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
>> "EXTENDED XAPIC (X2APIC)" -> "x2APIC State Transitions", the APIC mode
>> and APIC ID are preserved when handling INIT signal, no matter the
>> current mode is x2APIC mode or xAPIC. All the other APIC registers are
>> initialized, exactly like the result of a reset.  This patch
>> introduces a new function lapic_INIT() and replaces the wrongly used
>> lapic_reset().
>
>Please refer to the correct function names. Also according to the
>patch there's nothing wrong with mode handling (you only correct
>the ID part), yet the text above reads as if both were wrong (but
>see below for a mode related aspect wrt RESET).

Agree. Regarding to the mode, I also think it should be resetting in 
vlapic_reset(). I will reset the mode and preserve the APIC base
address.

>
>> HVM guest can't enable x2apic as XENFEAT_hvm_pirqs is exposed to it.
>> Why we have this restriction? As a consequence, This patch isn't
>> tested in that case.
>
>HVM guests very well can use x2APIC, I'm seeing them use it all
>the time. Please be more specific with your question.

OK. I failed to enable guest's x2apic, with the output: IRQ Remapping doesn't
support X2APIC mode. Eventually, I found the xen_x2apic_para_available()
in linux kernel and it returns false when Xen exposes XENFEAT_hvm_pirqs to
guest.

>> @@ -1262,7 +1261,6 @@ void vlapic_reset(struct vlapic *vlapic)
>>      vlapic_set_reg(vlapic, APIC_TMICT,   0);
>>      vlapic_set_reg(vlapic, APIC_TMCCT,   0);
>
>Upwards from here is an LDR write. Judging from set_x2apic_id()
>this may need to remain untouched in the INIT case? The register
>is r/o in x2APIC mode after all. While the documentation includes
>LDR in the set of registers being set to zero, I'm wondering whether
>that's wrong.

agree.

>
>> +
>> +    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
>> +    vlapic_INIT(vlapic);
>
>With what is left here it now very much looks like mode isn't being
>(and hasn't been) reset to xAPIC - wouldn't you need to correct
>this too? This might be particularly relevant for the call from
>hvm_s3_suspend().

will do.

Thanks
Chao

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