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(-)
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
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>
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
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.
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.
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.