On 27/11/2024 8:20 am, Jan Beulich wrote:
> On 26.11.2024 21:58, Andrew Cooper wrote:
>> @@ -1389,8 +1381,7 @@ static void cf_check error_interrupt(void)
>> unsigned int i;
>>
>> /* First tickle the hardware, only then report what went on. -- REW */
>> - apic_write(APIC_ESR, 0);
>> - v = apic_read(APIC_ESR);
>> + v = apic_read_esr();
>> ack_APIC_irq();
> As indicated in the earlier reply, I think this comment should be dropped,
This one probably can, but ...
> plus ...
>
>> --- 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.
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 ?
> With that and with Javi's change folded into here,
> Reviewed-by: Jan Beulich <[email protected]>
Thanks.
~Andrew
