Re: [XEN PATCH v1] x86/APIC: Read Error Status Register correctly

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2024 21:07, Andrew Cooper wrote:
> On 26/11/2024 5:06 pm, Javi Merino wrote:
>> The logic to read the APIC_ESR was copied from linux in a commit from
>> 2002: 4676bbf96dc8 (bitkeeper revision
>> 1.2 (3ddb79c9KusG02eh7i-uXkgY0IksKA), 2002-11-20).  In linux 3.14,
>> this logic was fixed to follow the Intel SDM (see commit
>> 60283df7ac26 (x86/apic: Read Error Status Register correctly,
>> 2014-01-14) in the linux kernel).  The Intel(r) 64 and IA-32
>> Architectures Software Develover's Manual currently says
>> in Volume 3, Section 12.5.3:
>>
>>   Before attempt to read from the ESR, software should first write to
>>   it. (The value written does not affect the values read subsequently;
>>   only zero may be written in x2APIC mode.) This write clears any
>>   previously logged errors and updates the ESR with any errors
>>   detected since the last write to the ESR. This write also rearms the
>>   APIC error interrupt triggering mechanism.
>>
>> Update error_interrupt() to remove the first read and follow the Intel
>> manual.
>>
>> Signed-off-by: Javi Merino <javi.merino@xxxxxxxxx>
> 
> In Linux, this bugfix was further corrected with
> https://lore.kernel.org/lkml/alpine.LFD.2.11.1404011300010.27402@xxxxxxxxxxxxxxxxxxxx/
> 
> However, Xen being 64-bit only doesn't care about the Pentium 3AP errata
> with writing to ESR.
> 
> I'm tempted to take this patch as-is, then do a followup on top to
> remove the remnants of the Pentium errata from Xen.  I don't think it's
> interesting to take bugfixes to bugfixes simply to delete them right after.

Hmm. I think the adjustment done here is actually removing part of the erratum
workaround, and hence ought to be removed in one go by your patch. The double
read there is - aiui - to be on the safe side wrt that erratum, i.e. to cover
the (at that time) potential case that the erratum would also be present on
CPUs with more than 3 LVTs.

In any event the comment ahead of the code being touched ought to be removed as
well, imo.

Jan