Re: [PATCH] x86/APIC: Remove workaround Pentium 3AP APIC_ESR erratum

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/11/2024 9:41 am, Jan Beulich wrote:
> On 27.11.2024 19:01, Andrew Cooper wrote:
>> On 27/11/2024 8:20 am, Jan Beulich wrote:
>>> On 26.11.2024 21:58, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/smpboot.c
>>>> +++ b/xen/arch/x86/smpboot.c
>>>> @@ -422,7 +422,7 @@ void asmlinkage start_secondary(void *unused)
>>>>  static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
>>>>  {
>>>>      unsigned long send_status = 0, accept_status = 0;
>>>> -    int maxlvt, timeout, i;
>>>> +    int timeout, i;
>>>>  
>>>>      /*
>>>>       * Normal AP startup uses an INIT-SIPI-SIPI sequence.
>>>> @@ -444,8 +444,7 @@ static int wakeup_secondary_cpu(int phys_apicid, 
>>>> unsigned long start_eip)
>>>>      /*
>>>>       * Be paranoid about clearing APIC errors.
>>>>       */
>>>> -    apic_write(APIC_ESR, 0);
>>>> -    apic_read(APIC_ESR);
>>>> +    apic_read_esr();
>>> ... the one here.
>> ... this one is still correct in place.
> Oh, right.
>
>> I almost had a second function called apic_clear_esr() which was just
>> (void)apic_read_esr().
>>
>> I could put that back in if you think it would be clearer ?
> Nah, it's good as is.

Actually on discussing this with Christian, apic_clear_esr() needs the
write only, and not the read (given no 3AP workaround).

So it is genuinely beneficial to separate apic_{clear,read}_esr() in our
code.

I'll see what the result looks like.  I suspect it will be cleaner-still.

~Andrew