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
