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

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

[Javi Merino]

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@cloud.com>
---
 xen/arch/x86/apic.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index bb86a1c161b3..b4f542d25918 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1385,20 +1385,19 @@ static void cf_check error_interrupt(void)
         ", Illegal register address",
     };
     const char *entries[ARRAY_SIZE(esr_fields)];
-    unsigned int v, v1;
+    unsigned int v;
     unsigned int i;
 
     /* First tickle the hardware, only then report what went on. -- REW */
-    v = apic_read(APIC_ESR);
     apic_write(APIC_ESR, 0);
-    v1 = apic_read(APIC_ESR);
+    v = apic_read(APIC_ESR);
     ack_APIC_irq();
 
     for ( i = 0; i < ARRAY_SIZE(entries); ++i )
-        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
+        entries[i] = v & (1 << i) ? esr_fields[i] : "";
     printk(XENLOG_DEBUG
-           "APIC error on CPU%u: %02x(%02x)%s%s%s%s%s%s%s%s\n",
-           smp_processor_id(), v, v1,
+           "APIC error on CPU%u: %02x%s%s%s%s%s%s%s%s\n",
+           smp_processor_id(), v,
            entries[7], entries[6], entries[5], entries[4],
            entries[3], entries[2], entries[1], entries[0]);
 }
-- 
2.46.0

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

[Andrew Cooper]

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@cloud.com>

In Linux, this bugfix was further corrected with
https://lore.kernel.org/lkml/alpine.LFD.2.11.1404011300010.27402@eddie.linux-mips.org/

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.

Thoughts?

~Andrew

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

[Jan Beulich]

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@cloud.com>
> 
> In Linux, this bugfix was further corrected with
> https://lore.kernel.org/lkml/alpine.LFD.2.11.1404011300010.27402@eddie.linux-mips.org/
> 
> 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

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

[Andrew Cooper]

The SDM instructs software to write 0 to ESR prior to reading it.  However,
due to an original Pentium erratum, most logic skips the write based on there
being more than 3 LVTs; a stand-in to identify the Pentium.

Xen, being 64bit, doesn't need compatibility for i586 processors.

Introduce a new apic_read_esr() helper, quoting the SDM to explain why a
function named apic_read_esr() has a write in it too.

Use the new helper throughout apic.c and smpboot.c, which allows us to remove
some useless reads of APIC_LVR.  This in turn removes the external callers of
get_maxlvt(), so make it local to apic.c

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Javi Merino <javi.merino@cloud.com>

Based on Javi's patch correcting error_interrupt()

Bloat-o-meter reports:

  add/remove: 0/1 grow/shrink: 0/3 up/down: 0/-269 (-269)
  Function                                     old     new   delta
  get_maxlvt                                    48       -     -48
  __cpu_up                                    1465    1417     -48
  clear_local_APIC                            1109    1050     -59
  setup_local_APIC                             942     828    -114
---
 xen/arch/x86/apic.c             | 29 ++++++++++-------------------
 xen/arch/x86/include/asm/apic.h | 24 +++++++++++++++++++++++-
 xen/arch/x86/smpboot.c          | 17 ++++-------------
 3 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index b4f542d25918..017d97054b06 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -142,7 +142,7 @@ int get_physical_broadcast(void)
         return 0xf;
 }
 
-int get_maxlvt(void)
+static int get_maxlvt(void)
 {
     unsigned int v = apic_read(APIC_LVR);
 
@@ -209,9 +209,7 @@ void clear_local_APIC(void)
         apic_write(APIC_LDR, v);
     }
 
-    if (maxlvt > 3)        /* Due to Pentium errata 3AP and 11AP. */
-        apic_write(APIC_ESR, 0);
-    apic_read(APIC_ESR);
+    apic_read_esr();
 }
 
 void __init connect_bsp_APIC(void)
@@ -488,7 +486,7 @@ static void resume_x2apic(void)
 
 void setup_local_APIC(bool bsp)
 {
-    unsigned long oldvalue, value, maxlvt;
+    unsigned long oldvalue, value;
     int i, j;
 
     BUILD_BUG_ON((SPURIOUS_APIC_VECTOR & 0x0f) != 0x0f);
@@ -614,17 +612,13 @@ void setup_local_APIC(bool bsp)
         value = APIC_DM_NMI | APIC_LVT_MASKED;
     apic_write(APIC_LVT1, value);
 
-    maxlvt = get_maxlvt();
-    if (maxlvt > 3)     /* Due to the Pentium erratum 3AP. */
-        apic_write(APIC_ESR, 0);
-    oldvalue = apic_read(APIC_ESR);
+    oldvalue = apic_read_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);
+
+    value = apic_read_esr();
+
     if (value != oldvalue)
         apic_printk(APIC_VERBOSE,
                     "ESR value before enabling vector: %#lx  after: %#lx\n",
@@ -719,11 +713,9 @@ int lapic_resume(void)
     apic_write(APIC_LVTT, apic_pm_state.apic_lvtt);
     apic_write(APIC_TDCR, apic_pm_state.apic_tdcr);
     apic_write(APIC_TMICT, apic_pm_state.apic_tmict);
-    apic_write(APIC_ESR, 0);
-    apic_read(APIC_ESR);
+    apic_read_esr();
     apic_write(APIC_LVTERR, apic_pm_state.apic_lvterr);
-    apic_write(APIC_ESR, 0);
-    apic_read(APIC_ESR);
+    apic_read_esr();
     local_irq_restore(flags);
     return 0;
 }
@@ -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();
 
     for ( i = 0; i < ARRAY_SIZE(entries); ++i )
diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index d8eda6df6d86..337eb5cf6642 100644
--- a/xen/arch/x86/include/asm/apic.h
+++ b/xen/arch/x86/include/asm/apic.h
@@ -151,6 +151,29 @@ static inline u32 get_apic_id(void)
     return x2apic_enabled ? id : GET_xAPIC_ID(id);
 }
 
+static inline uint32_t apic_read_esr(void)
+{
+    /*
+     * The SDM states:
+     *   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.
+     */
+    if ( x2apic_enabled )
+    {
+        apic_wrmsr(APIC_ESR, 0);
+        return apic_rdmsr(APIC_ESR);
+    }
+    else
+    {
+        apic_mem_write(APIC_ESR, 0);
+        return apic_mem_read(APIC_ESR);
+    }
+}
+
 void apic_wait_icr_idle(void);
 
 int get_physical_broadcast(void);
@@ -161,7 +184,6 @@ static inline void ack_APIC_irq(void)
 	apic_write(APIC_EOI, 0);
 }
 
-extern int get_maxlvt(void);
 extern void clear_local_APIC(void);
 extern void connect_bsp_APIC (void);
 extern void disconnect_bsp_APIC (int virt_wire_setup);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 79a79c54c304..7c77125fe715 100644
--- 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();
 
     if ( send_INIT )
     {
@@ -495,13 +494,10 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
         }
     }
 
-    maxlvt = get_maxlvt();
-
     for ( i = 0; i < 2; i++ )
     {
         Dprintk("Sending STARTUP #%d.\n", i+1);
-        apic_write(APIC_ESR, 0);
-        apic_read(APIC_ESR);
+        apic_read_esr();
         Dprintk("After apic_write.\n");
 
         /*
@@ -529,12 +525,7 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
             udelay(200);
         }
 
-        /* Due to the Pentium erratum 3AP. */
-        if ( maxlvt > 3 )
-        {
-            apic_write(APIC_ESR, 0);
-        }
-        accept_status = (apic_read(APIC_ESR) & 0xEF);
+        accept_status = apic_read_esr() & 0xEF;
         if ( send_status || accept_status )
             break;
     }

base-commit: c8e3e39085bf97d1afb775d54884d239387e32cd
prerequisite-patch-id: 1f86bfc85bb08e12c21535d5c527f555f192d4e7
-- 
2.39.5

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

[Jan Beulich]

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,
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. With that and with Javi's change folded into here,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Andrew Cooper]

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 <jbeulich@suse.com>

Thanks.

~Andrew

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

[Jan Beulich]

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.

Jan

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

[Andrew Cooper]

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

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

[Roger Pau Monné]

On Tue, Nov 26, 2024 at 08:58:59PM +0000, Andrew Cooper wrote:
> The SDM instructs software to write 0 to ESR prior to reading it.  However,
> due to an original Pentium erratum, most logic skips the write based on there
> being more than 3 LVTs; a stand-in to identify the Pentium.
> 
> Xen, being 64bit, doesn't need compatibility for i586 processors.
> 
> Introduce a new apic_read_esr() helper, quoting the SDM to explain why a
> function named apic_read_esr() has a write in it too.
> 
> Use the new helper throughout apic.c and smpboot.c, which allows us to remove
> some useless reads of APIC_LVR.  This in turn removes the external callers of
> get_maxlvt(), so make it local to apic.c
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Just a couple of nits.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Javi Merino <javi.merino@cloud.com>
> 
> Based on Javi's patch correcting error_interrupt()
> 
> Bloat-o-meter reports:
> 
>   add/remove: 0/1 grow/shrink: 0/3 up/down: 0/-269 (-269)
>   Function                                     old     new   delta
>   get_maxlvt                                    48       -     -48
>   __cpu_up                                    1465    1417     -48
>   clear_local_APIC                            1109    1050     -59
>   setup_local_APIC                             942     828    -114
> ---
>  xen/arch/x86/apic.c             | 29 ++++++++++-------------------
>  xen/arch/x86/include/asm/apic.h | 24 +++++++++++++++++++++++-
>  xen/arch/x86/smpboot.c          | 17 ++++-------------
>  3 files changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index b4f542d25918..017d97054b06 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -142,7 +142,7 @@ int get_physical_broadcast(void)
>          return 0xf;
>  }
>  
> -int get_maxlvt(void)
> +static int get_maxlvt(void)

I think returning unsigned would be more natural here, IMO uint8_t
would also be fine since it's the size of the field.

>  {
>      unsigned int v = apic_read(APIC_LVR);
>  
> @@ -209,9 +209,7 @@ void clear_local_APIC(void)
>          apic_write(APIC_LDR, v);
>      }
>  
> -    if (maxlvt > 3)        /* Due to Pentium errata 3AP and 11AP. */
> -        apic_write(APIC_ESR, 0);
> -    apic_read(APIC_ESR);
> +    apic_read_esr();
>  }
>  
>  void __init connect_bsp_APIC(void)
> @@ -488,7 +486,7 @@ static void resume_x2apic(void)
>  
>  void setup_local_APIC(bool bsp)
>  {
> -    unsigned long oldvalue, value, maxlvt;
> +    unsigned long oldvalue, value;

uint32_t would possibly be better here, since those are used to store
the contents of a register, but would need changes in the print
formatters.

>      int i, j;
>  
>      BUILD_BUG_ON((SPURIOUS_APIC_VECTOR & 0x0f) != 0x0f);
> @@ -614,17 +612,13 @@ void setup_local_APIC(bool bsp)
>          value = APIC_DM_NMI | APIC_LVT_MASKED;
>      apic_write(APIC_LVT1, value);
>  
> -    maxlvt = get_maxlvt();
> -    if (maxlvt > 3)     /* Due to the Pentium erratum 3AP. */
> -        apic_write(APIC_ESR, 0);
> -    oldvalue = apic_read(APIC_ESR);
> +    oldvalue = apic_read_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);
> +
> +    value = apic_read_esr();
> +
>      if (value != oldvalue)
>          apic_printk(APIC_VERBOSE,
>                      "ESR value before enabling vector: %#lx  after: %#lx\n",
> @@ -719,11 +713,9 @@ int lapic_resume(void)
>      apic_write(APIC_LVTT, apic_pm_state.apic_lvtt);
>      apic_write(APIC_TDCR, apic_pm_state.apic_tdcr);
>      apic_write(APIC_TMICT, apic_pm_state.apic_tmict);
> -    apic_write(APIC_ESR, 0);
> -    apic_read(APIC_ESR);
> +    apic_read_esr();
>      apic_write(APIC_LVTERR, apic_pm_state.apic_lvterr);
> -    apic_write(APIC_ESR, 0);
> -    apic_read(APIC_ESR);
> +    apic_read_esr();
>      local_irq_restore(flags);
>      return 0;
>  }
> @@ -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 */

I think the comment can be removed now, as there's no "tickle" in this
context anymore?

> -    apic_write(APIC_ESR, 0);
> -    v = apic_read(APIC_ESR);
> +    v = apic_read_esr();
>      ack_APIC_irq();
>  
>      for ( i = 0; i < ARRAY_SIZE(entries); ++i )
> diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
> index d8eda6df6d86..337eb5cf6642 100644
> --- a/xen/arch/x86/include/asm/apic.h
> +++ b/xen/arch/x86/include/asm/apic.h
> @@ -151,6 +151,29 @@ static inline u32 get_apic_id(void)
>      return x2apic_enabled ? id : GET_xAPIC_ID(id);
>  }
>  
> +static inline uint32_t apic_read_esr(void)
> +{
> +    /*
> +     * The SDM states:
> +     *   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.
> +     */
> +    if ( x2apic_enabled )
> +    {
> +        apic_wrmsr(APIC_ESR, 0);
> +        return apic_rdmsr(APIC_ESR);
> +    }
> +    else

There's no need for the else case if the previous branch unconditionally
ends with a return.  Can reduce indentation.

> +    {
> +        apic_mem_write(APIC_ESR, 0);
> +        return apic_mem_read(APIC_ESR);
> +    }
> +}
> +
>  void apic_wait_icr_idle(void);
>  
>  int get_physical_broadcast(void);
> @@ -161,7 +184,6 @@ static inline void ack_APIC_irq(void)
>  	apic_write(APIC_EOI, 0);
>  }
>  
> -extern int get_maxlvt(void);
>  extern void clear_local_APIC(void);
>  extern void connect_bsp_APIC (void);
>  extern void disconnect_bsp_APIC (int virt_wire_setup);
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 79a79c54c304..7c77125fe715 100644
> --- 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;

You could make those unsigned I believe.

Thanks, Roger.

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

[Andrew Cooper]

On 27/11/2024 8:38 am, Roger Pau Monné wrote:
> On Tue, Nov 26, 2024 at 08:58:59PM +0000, Andrew Cooper wrote:
>> The SDM instructs software to write 0 to ESR prior to reading it.  However,
>> due to an original Pentium erratum, most logic skips the write based on there
>> being more than 3 LVTs; a stand-in to identify the Pentium.
>>
>> Xen, being 64bit, doesn't need compatibility for i586 processors.
>>
>> Introduce a new apic_read_esr() helper, quoting the SDM to explain why a
>> function named apic_read_esr() has a write in it too.
>>
>> Use the new helper throughout apic.c and smpboot.c, which allows us to remove
>> some useless reads of APIC_LVR.  This in turn removes the external callers of
>> get_maxlvt(), so make it local to apic.c
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Just a couple of nits.

Thanks, but I'm going to intentionally defer the ancillary cleanup for
later.

Yes it should be done, but; while I'm very confident about the fix, if
it does turn out to break something then I don't want to be "was it a
different errata, or was it an integer handling change" because both
will be nasty.

~Andrew

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

[Javi Merino]

On Tue, Nov 26, 2024 at 08:58:59PM +0000, Andrew Cooper wrote:
> The SDM instructs software to write 0 to ESR prior to reading it.  However,
> due to an original Pentium erratum, most logic skips the write based on there
> being more than 3 LVTs; a stand-in to identify the Pentium.
> 
> Xen, being 64bit, doesn't need compatibility for i586 processors.
> 
> Introduce a new apic_read_esr() helper, quoting the SDM to explain why a
> function named apic_read_esr() has a write in it too.
> 
> Use the new helper throughout apic.c and smpboot.c, which allows us to remove
> some useless reads of APIC_LVR.  This in turn removes the external callers of
> get_maxlvt(), so make it local to apic.c
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Javi Merino <javi.merino@cloud.com>
> 
> Based on Javi's patch correcting error_interrupt()

Fair enough.  I was only looking at error_interrupt() and missed the
bigger picture. This patch is more comprehensive and this is very nice:

> Bloat-o-meter reports:
> 
>   add/remove: 0/1 grow/shrink: 0/3 up/down: 0/-269 (-269)
>   Function                                     old     new   delta
>   get_maxlvt                                    48       -     -48
>   __cpu_up                                    1465    1417     -48
>   clear_local_APIC                            1109    1050     -59
>   setup_local_APIC                             942     828    -114
> ---
>  xen/arch/x86/apic.c             | 29 ++++++++++-------------------
>  xen/arch/x86/include/asm/apic.h | 24 +++++++++++++++++++++++-
>  xen/arch/x86/smpboot.c          | 17 ++++-------------
>  3 files changed, 37 insertions(+), 33 deletions(-)

Reviewed-by: Javi Merino <javi.merino@cloud.com>

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

[Andrew Cooper]

On 27/11/2024 10:03 am, Javi Merino wrote:
> On Tue, Nov 26, 2024 at 08:58:59PM +0000, Andrew Cooper wrote:
>> The SDM instructs software to write 0 to ESR prior to reading it.  However,
>> due to an original Pentium erratum, most logic skips the write based on there
>> being more than 3 LVTs; a stand-in to identify the Pentium.
>>
>> Xen, being 64bit, doesn't need compatibility for i586 processors.
>>
>> Introduce a new apic_read_esr() helper, quoting the SDM to explain why a
>> function named apic_read_esr() has a write in it too.
>>
>> Use the new helper throughout apic.c and smpboot.c, which allows us to remove
>> some useless reads of APIC_LVR.  This in turn removes the external callers of
>> get_maxlvt(), so make it local to apic.c
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Javi Merino <javi.merino@cloud.com>
>>
>> Based on Javi's patch correcting error_interrupt()
> Fair enough.  I was only looking at error_interrupt() and missed the
> bigger picture. This patch is more comprehensive and this is very nice:
>
>> Bloat-o-meter reports:
>>
>>   add/remove: 0/1 grow/shrink: 0/3 up/down: 0/-269 (-269)
>>   Function                                     old     new   delta
>>   get_maxlvt                                    48       -     -48
>>   __cpu_up                                    1465    1417     -48
>>   clear_local_APIC                            1109    1050     -59
>>   setup_local_APIC                             942     828    -114
>> ---
>>  xen/arch/x86/apic.c             | 29 ++++++++++-------------------
>>  xen/arch/x86/include/asm/apic.h | 24 +++++++++++++++++++++++-
>>  xen/arch/x86/smpboot.c          | 17 ++++-------------
>>  3 files changed, 37 insertions(+), 33 deletions(-)
> Reviewed-by: Javi Merino <javi.merino@cloud.com>

Thanks.  Are you happy for me to merge the two patches?

~Andrew

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

[Javi Merino]

On Wed, Nov 27, 2024 at 05:45:29PM +0000, Andrew Cooper wrote:
> On 27/11/2024 10:03 am, Javi Merino wrote:
> > On Tue, Nov 26, 2024 at 08:58:59PM +0000, Andrew Cooper wrote:
> >> The SDM instructs software to write 0 to ESR prior to reading it.  However,
> >> due to an original Pentium erratum, most logic skips the write based on there
> >> being more than 3 LVTs; a stand-in to identify the Pentium.
> >>
> >> Xen, being 64bit, doesn't need compatibility for i586 processors.
> >>
> >> Introduce a new apic_read_esr() helper, quoting the SDM to explain why a
> >> function named apic_read_esr() has a write in it too.
> >>
> >> Use the new helper throughout apic.c and smpboot.c, which allows us to remove
> >> some useless reads of APIC_LVR.  This in turn removes the external callers of
> >> get_maxlvt(), so make it local to apic.c
> >>
> >> No practical change.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Javi Merino <javi.merino@cloud.com>
> >>
> >> Based on Javi's patch correcting error_interrupt()
> > Fair enough.  I was only looking at error_interrupt() and missed the
> > bigger picture. This patch is more comprehensive and this is very nice:
> >
> >> Bloat-o-meter reports:
> >>
> >>   add/remove: 0/1 grow/shrink: 0/3 up/down: 0/-269 (-269)
> >>   Function                                     old     new   delta
> >>   get_maxlvt                                    48       -     -48
> >>   __cpu_up                                    1465    1417     -48
> >>   clear_local_APIC                            1109    1050     -59
> >>   setup_local_APIC                             942     828    -114
> >> ---
> >>  xen/arch/x86/apic.c             | 29 ++++++++++-------------------
> >>  xen/arch/x86/include/asm/apic.h | 24 +++++++++++++++++++++++-
> >>  xen/arch/x86/smpboot.c          | 17 ++++-------------
> >>  3 files changed, 37 insertions(+), 33 deletions(-)
> > Reviewed-by: Javi Merino <javi.merino@cloud.com>
> 
> Thanks.  Are you happy for me to merge the two patches?

These changes represent a single logical change, so they should be in
one commit. Please merge the two patches.

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

[Roger Pau Monné]

On Tue, Nov 26, 2024 at 05:06:15PM +0000, 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@cloud.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Not sure whether the plan is to squash your commit and Andrews.

Thanks, Roger.