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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>

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

Just a couple of nits.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Javi Merino <javi.merino@xxxxxxxxx>
> 
> 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.