Re: [Xen-devel] [PATCH] x86/apic: fix disabling LVT0 in disconnect_bsp_APIC

[Jan Beulich <jbeulich@suse.com>]

On 23.01.2020 16:43, Roger Pau Monné wrote:
> On Fri, Jan 17, 2020 at 05:25:12PM +0100, Jan Beulich wrote:
>> On 17.01.2020 17:08, Roger Pau Monné wrote:
>>> On Fri, Jan 17, 2020 at 04:56:00PM +0100, Jan Beulich wrote:
>>>> On 17.01.2020 16:09, Roger Pau Monne wrote:
>>>>> The Intel SDM states:
>>>>>
>>>>> "When an illegal vector value (0 to 15) is written to a LVT entry and
>>>>> the delivery mode is Fixed (bits 8-11 equal 0), the APIC may signal an
>>>>> illegal vector error, without regard to whether the mask bit is set or
>>>>> whether an interrupt is actually seen on the input."
>>>>>
>>>>> And that's exactly what's currently done in disconnect_bsp_APIC when
>>>>> virt_wire_setup is true and LVT LINT0 is being masked. By writing only
>>>>> APIC_LVT_MASKED Xen is actually setting the vector to 0 and the
>>>>> delivery mode to Fixed (0), and hence it triggers an APIC error even
>>>>> when the LVT entry is masked.
>>>>
>>>> But there are many more instances where we (have a risk to) do so,
>>>> most notably in clear_local_APIC(). The two step logic there is
>>>> anyway somewhat in conflict with the citation above.
>>>
>>> clear_local_APIC masks the error vector before doing any write, and
>>> clears ESR afterwards, there's a comment at the top:
>>>
>>> "Masking an LVT entry on a P6 can trigger a local APIC error
>>> if the vector is zero. Mask LVTERR first to prevent this."
>>>
>>> We could do the same (ie: mask LVTERR first and clear ESR afterwards)
>>> if that seems preferable. There's a maxlvt check in clear_local_APIC,
>>> but the sdm doesn't specify anyway to check if the lapic will accept a
>>> masked vector 0 write or not, so not sure whether we should replicate
>>> that check or just do it unconditionally on both disconnect_bsp_APIC
>>> and clear_local_APIC.
>>
>> I think doing it the most careful way is going to be best. I find it
>> surprising anyway that disconnect_bsp_APIC() doesn't write LVTERR
>> (or other LVTs except for LVT1) at all. The function looks to have a
>> goal of putting the APIC back into the state that we found it when
>> booting.
> 
> Maybe it would be better to just call clear_local_APIC before trying
> to configure LVT{0/1}, which will leave LVT0 in a reset state and thus
> no write would be required in the !virt_wire_setup case?

Half of me was implying this as on option from the earlier reply.
The other half was thinking that this would be quite a lot of
behavioral change in one step. But since you think so too, why
don't we give this a try?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel