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.0On 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.
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
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
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.