Re: [PATCH] x86/APIC: Remove esr_disable

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/08/2023 5:00 pm, Roger Pau Monné wrote:
> On Tue, Aug 29, 2023 at 04:46:21PM +0100, Andrew Cooper wrote:
>> It is unconditionally 0 in Xen, and was deleted in Linux somewhere between 
>> 2.5
>> and 2.6.
>>
>> Remove it in Xen too.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>>
>> I got bored waiting for `git log` to tell where it was disabled in Linux...
>> ---
>>  xen/arch/x86/apic.c                           | 50 ++++++-------------
>>  .../x86/include/asm/mach-generic/mach_apic.h  |  3 --
>>  2 files changed, 16 insertions(+), 37 deletions(-)
>>
>> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
>> index 41879230ec90..5c6935ba42db 100644
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -496,14 +496,6 @@ void setup_local_APIC(bool bsp)
>>      unsigned long oldvalue, value, maxlvt;
>>      int i, j;
>>  
>> -    /* Pound the ESR really hard over the head with a big hammer - mbligh */
>> -    if (esr_disable) {
>> -        apic_write(APIC_ESR, 0);
>> -        apic_write(APIC_ESR, 0);
>> -        apic_write(APIC_ESR, 0);
>> -        apic_write(APIC_ESR, 0);
>> -    }
>> -
>>      BUILD_BUG_ON((SPURIOUS_APIC_VECTOR & 0x0f) != 0x0f);
>>  
>>      /*
>> @@ -628,33 +620,23 @@ void setup_local_APIC(bool bsp)
>>          value = APIC_DM_NMI | APIC_LVT_MASKED;
>>      apic_write(APIC_LVT1, value);
>>  
>> -    if (!esr_disable) {
>> -        maxlvt = get_maxlvt();
>> -        if (maxlvt > 3)     /* Due to the Pentium erratum 3AP. */
>> -            apic_write(APIC_ESR, 0);
>> -        oldvalue = apic_read(APIC_ESR);
>> +    maxlvt = get_maxlvt();
>> +    if (maxlvt > 3)     /* Due to the Pentium erratum 3AP. */
>> +        apic_write(APIC_ESR, 0);
>> +    oldvalue = apic_read(APIC_ESR);
>>  
>> -        value = ERROR_APIC_VECTOR;      // enables sending errors
>> -        apic_write(APIC_LVTERR, value);
>> -        /*
>> -         * spec says clear errors after enabling vector.
>> -         */
>> -        if (maxlvt > 3)
>> -            apic_write(APIC_ESR, 0);
>> -        value = apic_read(APIC_ESR);
>> -        if (value != oldvalue)
>> -            apic_printk(APIC_VERBOSE, "ESR value before enabling "
>> -                        "vector: %#lx  after: %#lx\n",
>> -                        oldvalue, value);
>> -    } else {
>> -        /*
>> -         * Something untraceble is creating bad interrupts on
>> -         * secondary quads ... for the moment, just leave the
>> -         * ESR disabled - we can't do anything useful with the
>> -         * errors anyway - mbligh
>> -         */
>> -        printk("Leaving ESR disabled.\n");
>> -    }
>> +    value = ERROR_APIC_VECTOR;      // enables sending errors
>> +    apic_write(APIC_LVTERR, value);
>> +    /*
>> +     * spec says clear errors after enabling vector.
> Is it worth making this a one-line comment while adjusting padding?

Yeah ok.

>
> Regardless:
>
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

~Andrew