arch/x86/include/asm/mce.h | 4 +-- arch/x86/kernel/cpu/mce/amd.c | 18 +++++------ arch/x86/kernel/cpu/mce/core.c | 47 ++++++++++++++-------------- arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++----- arch/x86/kernel/cpu/mce/genpool.c | 8 ++--- arch/x86/kernel/cpu/mce/intel.c | 11 ++++--- arch/x86/kernel/cpu/mce/threshold.c | 2 +- 7 files changed, 46 insertions(+), 55 deletions(-)
1. Clean up some x86/mce code as below. No functional changes intended.
- Simplify some code.
- Remove some unnecessary code.
- Improve readability for some code.
- Fix some typos.
[ Reduce the text segment size by ~116 bytes. ]
2. Pass the following basic tests:
- Compile test.
- Correctable/uncorrectable memory errors can be notified via CMCI/MCE interrupts.
- Correctable/uncorrectable memory errors can be dispatched to the mcelog daemon and the EDAC driver.
[ Tested on an Intel Sapphire Rapids server. ]
3. This patch series is based on v6.12-rc2.
Qiuxu Zhuo (10):
x86/mce/dev-mcelog: Use xchg() to get and clear the flags
x86/mce/intel: Use MCG_BANKCNT_MASK instead of 0xff
x86/mce: Make several functions return bool
x86/mce/threshold: Remove the redundant this_cpu_dec_return()
x86/mce/genpool: Make mce_gen_pool_create() return explicit error codes
x86/mce: Convert multiple if () statements into a switch() statement
x86/mce: Remove the unnecessary {}
x86/mce: Remove the redundant zeroing assignments
x86/mce/amd: Remove unnecessary NULL pointer initializations
x86/mce: Fix typos in comments
arch/x86/include/asm/mce.h | 4 +--
arch/x86/kernel/cpu/mce/amd.c | 18 +++++------
arch/x86/kernel/cpu/mce/core.c | 47 ++++++++++++++--------------
arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++-----
arch/x86/kernel/cpu/mce/genpool.c | 8 ++---
arch/x86/kernel/cpu/mce/intel.c | 11 ++++---
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
7 files changed, 46 insertions(+), 55 deletions(-)
base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
--
2.17.1
1. Clean up some x86/mce code as below. No functional changes intended.
- Simplify some code.
- Remove some unnecessary code.
- Improve readability for some code.
- Fix some typos.
[ Reduce the text segment size by ~116 bytes. ]
2. Pass the following basic tests:
- Compile test.
- Correctable/uncorrectable memory errors can be notified via CMCI/MCE interrupts.
- Correctable/uncorrectable memory errors can be dispatched to the mcelog daemon and the EDAC driver.
[ Tested on an Intel Sapphire Rapids server. ]
3. This patch series is based on v6.12-rc3.
4. Changes in v2:
- Collect "Reviewed-by:" tags for patch {1-8,10}.
- Update the commit message of patch 9 to include the names of all
variables that don't need NULL pointer initializations.
Thanks Tony for reviewing this patch series.
Qiuxu Zhuo (10):
x86/mce/dev-mcelog: Use xchg() to get and clear the flags
x86/mce/intel: Use MCG_BANKCNT_MASK instead of 0xff
x86/mce: Make several functions return bool
x86/mce/threshold: Remove the redundant this_cpu_dec_return()
x86/mce/genpool: Make mce_gen_pool_create() return explicit error codes
x86/mce: Convert multiple if () statements into a switch() statement
x86/mce: Remove the unnecessary {}
x86/mce: Remove the redundant zeroing assignments
x86/mce/amd: Remove unnecessary NULL pointer initializations
x86/mce: Fix typos in comments
arch/x86/include/asm/mce.h | 4 +--
arch/x86/kernel/cpu/mce/amd.c | 18 +++++------
arch/x86/kernel/cpu/mce/core.c | 47 ++++++++++++++--------------
arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++-----
arch/x86/kernel/cpu/mce/genpool.c | 8 ++---
arch/x86/kernel/cpu/mce/intel.c | 11 ++++---
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
7 files changed, 46 insertions(+), 55 deletions(-)
base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
--
2.17.1
On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> Qiuxu Zhuo (10):
> x86/mce/dev-mcelog: Use xchg() to get and clear the flags
> x86/mce/intel: Use MCG_BANKCNT_MASK instead of 0xff
> x86/mce: Make several functions return bool
> x86/mce/threshold: Remove the redundant this_cpu_dec_return()
> x86/mce/genpool: Make mce_gen_pool_create() return explicit error codes
> x86/mce: Convert multiple if () statements into a switch() statement
> x86/mce: Remove the unnecessary {}
> x86/mce: Remove the redundant zeroing assignments
> x86/mce/amd: Remove unnecessary NULL pointer initializations
> x86/mce: Fix typos in comments
>
Apart from the minor nits and commit message changes, the patches look
fine to me.
With those addressed,
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
On 16.10.24 г. 15:30 ч., Qiuxu Zhuo wrote:
> 1. Clean up some x86/mce code as below. No functional changes intended.
>
> - Simplify some code.
>
> - Remove some unnecessary code.
>
> - Improve readability for some code.
>
> - Fix some typos.
>
> [ Reduce the text segment size by ~116 bytes. ]
>
> 2. Pass the following basic tests:
>
> - Compile test.
>
> - Correctable/uncorrectable memory errors can be notified via CMCI/MCE interrupts.
>
> - Correctable/uncorrectable memory errors can be dispatched to the mcelog daemon and the EDAC driver.
>
> [ Tested on an Intel Sapphire Rapids server. ]
>
> 3. This patch series is based on v6.12-rc3.
>
> 4. Changes in v2:
>
> - Collect "Reviewed-by:" tags for patch {1-8,10}.
>
> - Update the commit message of patch 9 to include the names of all
> variables that don't need NULL pointer initializations.
>
> Thanks Tony for reviewing this patch series.
>
> Qiuxu Zhuo (10):
> x86/mce/dev-mcelog: Use xchg() to get and clear the flags
> x86/mce/intel: Use MCG_BANKCNT_MASK instead of 0xff
> x86/mce: Make several functions return bool
> x86/mce/threshold: Remove the redundant this_cpu_dec_return()
> x86/mce/genpool: Make mce_gen_pool_create() return explicit error codes
> x86/mce: Convert multiple if () statements into a switch() statement
> x86/mce: Remove the unnecessary {}
> x86/mce: Remove the redundant zeroing assignments
> x86/mce/amd: Remove unnecessary NULL pointer initializations
> x86/mce: Fix typos in comments
>
> arch/x86/include/asm/mce.h | 4 +--
> arch/x86/kernel/cpu/mce/amd.c | 18 +++++------
> arch/x86/kernel/cpu/mce/core.c | 47 ++++++++++++++--------------
> arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++-----
> arch/x86/kernel/cpu/mce/genpool.c | 8 ++---
> arch/x86/kernel/cpu/mce/intel.c | 11 ++++---
> arch/x86/kernel/cpu/mce/threshold.c | 2 +-
> 7 files changed, 46 insertions(+), 55 deletions(-)
>
>
> base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Clean up some x86/mce code as below. No functional changes intended.
- Simplify some code.
- Remove some unnecessary code.
- Improve readability for some code.
- Convert some family/model mixed checks to VFM-based checks.
- Fix some typos.
Pass the following basic tests:
- Tested on an Intel Sapphire Rapids server.
- Compile test.
- System boot test.
- Correctable/uncorrectable memory errors can be notified via CMCI/MCE interrupts.
- Correctable/uncorrectable memory errors can be dispatched to the mcelog daemon and the EDAC driver.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Drop the "x86/mce: Remove the redundant zeroing assignments" patch.
- 0003: Rename mce_notify_irq() to mce_notify_user().
- 0005: Move the 'int ret' variable along with the other int variables.
- 0006: New patch. Break up __mcheck_cpu_apply_quirks().
- 0007: New patch. Convert family/model mixed checks to VFM-based checks.
- 0009: Remove the variable names from the commit message.
- 0010: Remove the detail typos from the commit message.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
- 0009: Add the missing variable names to the commit message.
This series is based on v6.12-rc4.
Thanks Tony, Dave, Sohil and Nikolay for your review and discussion on this series.
Qiuxu Zhuo (9):
x86/mce/dev-mcelog: Use xchg() to get and clear the flags
x86/mce/intel: Use MCG_BANKCNT_MASK instead of 0xff
x86/mce: Make several functions return bool and rename a function
x86/mce/threshold: Remove the redundant this_cpu_dec_return()
x86/mce/genpool: Make mce_gen_pool_create() return explicit error codes
x86/mce: Convert family/model mixed checks to VFM-based checks
x86/mce: Remove the unnecessary {}
x86/mce/amd: Remove unnecessary NULL pointer initializations
x86/mce: Fix typos
Tony Luck (1):
x86/mce: Break up __mcheck_cpu_apply_quirks()
arch/x86/include/asm/mce.h | 4 +-
arch/x86/kernel/cpu/mce/amd.c | 18 +--
arch/x86/kernel/cpu/mce/core.c | 230 +++++++++++++++------------
arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 +-
arch/x86/kernel/cpu/mce/genpool.c | 9 +-
arch/x86/kernel/cpu/mce/inject.c | 2 +-
arch/x86/kernel/cpu/mce/intel.c | 11 +-
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
8 files changed, 149 insertions(+), 138 deletions(-)
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
--
2.17.1
Clean up some x86/mce code as below. No functional changes intended.
- Simplify some code.
- Remove some unnecessary code.
- Improve readability for some code.
- Convert some family/model mixed checks to VFM-based checks.
- Fix some typos.
Pass the following basic tests:
- Tested on an Intel Sapphire Rapids server.
- Compile test.
- System boot test.
- Correctable/uncorrectable memory errors can be notified via CMCI/MCE interrupts.
- Correctable/uncorrectable memory errors can be dispatched to the mcelog daemon and the EDAC driver.
Changes in v4:
- Drop the first two patches as they've landed in the TIP ras/core branch.
- Drop "Make mce_gen_pool_create() return explicit error codes" patch.
- 0001: Don't rename mce_notify_irq().
- 0003: New patch. Make four functions return bool.
- 0004: Add necessary blank lines and directly use 'mca_cfg'.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Drop the "x86/mce: Remove the redundant zeroing assignments" patch.
- 0003: Rename mce_notify_irq() to mce_notify_user().
- 0005: Move the 'int ret' variable along with the other int variables.
- 0006: New patch. Break up __mcheck_cpu_apply_quirks().
- 0007: New patch. Convert family/model mixed checks to VFM-based checks.
- 0009: Remove the variables' names from the commit message.
- 0010: Remove the detail typos from the commit message.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
- Update the commit message of patch 9 to include the names of all
variables that don't need NULL pointer initializations.
This series is based on the TIP branch ras/core with top commit 612c2addff36.
Thanks Thomas, Boris, Tony, Dave, Sohil and Nikolay for your review and discussion on this series.
Qiuxu Zhuo (7):
x86/mce: Make several functions return bool
x86/mce/threshold: Remove the redundant this_cpu_dec_return()
x86/mce: Make four functions return bool
x86/mce: Convert family/model mixed checks to VFM-based checks
x86/mce: Remove the unnecessary {}
x86/mce/amd: Remove unnecessary NULL pointer initializations
x86/mce: Fix typos
Tony Luck (1):
x86/mce: Break up __mcheck_cpu_apply_quirks()
arch/x86/include/asm/mce.h | 4 +-
arch/x86/kernel/cpu/mce/amd.c | 18 +--
arch/x86/kernel/cpu/mce/core.c | 234 +++++++++++++++-------------
arch/x86/kernel/cpu/mce/genpool.c | 29 ++--
arch/x86/kernel/cpu/mce/intel.c | 9 +-
arch/x86/kernel/cpu/mce/internal.h | 4 +-
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
7 files changed, 158 insertions(+), 142 deletions(-)
base-commit: 612c2addff367ee461dc99ffca2bc786f105d2ec
--
2.17.1
Clean up some x86/mce code as below. No functional changes intended.
- Simplify some code.
- Remove some unnecessary code.
- Improve readability for some code.
- Convert some family/model mixed checks to VFM-based checks.
Pass the following basic tests:
- Tested on an Intel Sapphire Rapids server.
- Compile test.
- System boot test.
- Correctable/uncorrectable memory errors can be notified via CMCI/MCE interrupts.
- Correctable/uncorrectable memory errors can be dispatched to the mcelog daemon and the EDAC driver.
Changes in v5:
- Collect "Reviewed-by:" from Sohil & Yazen.
- Drop "Fix typos" patch.
- 0003: Update the commit message with mention the polarities of the return values are flipped.
- 0005: Reduce 'if (mce_num_banks > 0)' to 'if (mce_num_banks)'.
- 0006: Combine AMD and HYGON feature initialization and remove mce_hygon_feature_init().
Changes in v4:
- Drop the first two patches as they've landed in the TIP ras/core branch.
- Drop "Make mce_gen_pool_create() return explicit error codes" patch.
- 0001: Don't rename mce_notify_irq().
- 0003: New patch. Make four functions return bool.
- 0004: Add necessary blank lines and directly use 'mca_cfg'.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Drop the "x86/mce: Remove the redundant zeroing assignments" patch.
- 0003: Rename mce_notify_irq() to mce_notify_user().
- 0005: Move the 'int ret' variable along with the other int variables.
- 0006: New patch. Break up __mcheck_cpu_apply_quirks().
- 0007: New patch. Convert family/model mixed checks to VFM-based checks.
- 0009: Remove the variables' names from the commit message.
- 0010: Remove the detail typos from the commit message.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
- Update the commit message of patch 9 to include the names of all
variables that don't need NULL pointer initializations.
This series is based on v6.13-rc2.
Thanks Thomas, Boris, Tony, Dave, Sohil, Yazen, and Nikolay for your review and discussion on this series.
Qiuxu Zhuo (6):
x86/mce: Make several functions return bool
x86/mce/threshold: Remove the redundant this_cpu_dec_return()
x86/mce: Make four functions return bool
x86/mce: Convert family/model mixed checks to VFM-based checks
x86/mce: Remove the redundant mce_hygon_feature_init()
x86/mce/amd: Remove unnecessary NULL pointer initializations
Tony Luck (1):
x86/mce: Break up __mcheck_cpu_apply_quirks()
arch/x86/include/asm/mce.h | 6 +-
arch/x86/kernel/cpu/mce/amd.c | 18 +--
arch/x86/kernel/cpu/mce/core.c | 237 +++++++++++++++-------------
arch/x86/kernel/cpu/mce/genpool.c | 29 ++--
arch/x86/kernel/cpu/mce/intel.c | 9 +-
arch/x86/kernel/cpu/mce/internal.h | 4 +-
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
7 files changed, 158 insertions(+), 147 deletions(-)
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
--
2.17.1
Make several functions that return 0 or 1 return a boolean value for
better readability.
No functional changes are intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- No changes.
Changes in v4:
- Don't rename mce_notify_irq() - Boris.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Rename mce_notify_irq() to mce_notify_user() (Sohil).
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/include/asm/mce.h | 4 ++--
arch/x86/kernel/cpu/mce/amd.c | 10 +++++-----
arch/x86/kernel/cpu/mce/core.c | 22 +++++++++++-----------
arch/x86/kernel/cpu/mce/intel.c | 9 +++++----
4 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4543cf2eb5e8..ea9ca7689f6b 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -276,7 +276,7 @@ static inline void cmci_rediscover(void) {}
static inline void cmci_recheck(void) {}
#endif
-int mce_available(struct cpuinfo_x86 *c);
+bool mce_available(struct cpuinfo_x86 *c);
bool mce_is_memory_error(struct mce *m);
bool mce_is_correctable(struct mce *m);
bool mce_usable_address(struct mce *m);
@@ -296,7 +296,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-int mce_notify_irq(void);
+bool mce_notify_irq(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 6ca80fff1fea..018874b554cb 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -381,7 +381,7 @@ static bool lvt_interrupt_supported(unsigned int bank, u32 msr_high_bits)
return msr_high_bits & BIT(28);
}
-static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
+static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
{
int msr = (hi & MASK_LVTOFF_HI) >> 20;
@@ -389,7 +389,7 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
if (apic != msr) {
@@ -399,15 +399,15 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
* was set is reserved. Return early here:
*/
if (mce_flags.smca)
- return 0;
+ return false;
pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n",
b->cpu, apic, b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
- return 1;
+ return true;
};
/* Reprogram MCx_MISC MSR behind this threshold bank. */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 7fb5556a0b53..167965bd2ac0 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -492,10 +492,10 @@ static noinstr void mce_gather_info(struct mce_hw_err *err, struct pt_regs *regs
}
}
-int mce_available(struct cpuinfo_x86 *c)
+bool mce_available(struct cpuinfo_x86 *c)
{
if (mca_cfg.disabled)
- return 0;
+ return false;
return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
}
@@ -1778,7 +1778,7 @@ static void mce_timer_delete_all(void)
* Can be called from interrupt context, but not from machine check/NMI
* context.
*/
-int mce_notify_irq(void)
+bool mce_notify_irq(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1789,9 +1789,9 @@ int mce_notify_irq(void)
if (__ratelimit(&ratelimit))
pr_info(HW_ERR "Machine check events logged\n");
- return 1;
+ return true;
}
- return 0;
+ return false;
}
EXPORT_SYMBOL_GPL(mce_notify_irq);
@@ -2015,25 +2015,25 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
return 0;
}
-static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
{
if (c->x86 != 5)
- return 0;
+ return false;
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
mce_flags.p5 = 1;
- return 1;
+ return true;
case X86_VENDOR_CENTAUR:
winchip_mcheck_init(c);
mce_flags.winchip = 1;
- return 1;
+ return true;
default:
- return 0;
+ return false;
}
- return 0;
+ return false;
}
/*
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index b3cd2c61b11d..f863df0ff42c 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -75,12 +75,12 @@ static u16 cmci_threshold[MAX_NR_BANKS];
*/
#define CMCI_STORM_THRESHOLD 32749
-static int cmci_supported(int *banks)
+static bool cmci_supported(int *banks)
{
u64 cap;
if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
- return 0;
+ return false;
/*
* Vendor check is not strictly needed, but the initial
@@ -89,10 +89,11 @@ static int cmci_supported(int *banks)
*/
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
- return 0;
+ return false;
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
- return 0;
+ return false;
+
rdmsrl(MSR_IA32_MCG_CAP, cap);
*banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
--
2.17.1On Thu, Dec 12, 2024 at 10:00:57PM +0800, Qiuxu Zhuo wrote: > Make several functions that return 0 or 1 return a boolean value for > better readability. > > No functional changes are intended. > > Reviewed-by: Tony Luck <tony.luck@intel.com> > Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> > Reviewed-by: Sohil Mehta <sohil.mehta@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks, Yazen
> From: Yazen Ghannam <yazen.ghannam@amd.com> > [...] > Subject: Re: [PATCH v5 1/7] x86/mce: Make several functions return bool > [...] > > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Hi Yazen, Thanks for reviewing this patch series. - Qiuxu
The following commit has been merged into the ras/core branch of tip:
Commit-ID: c845cb8dbd2e1a804babfd13648026c3a7cfbc0b
Gitweb: https://git.kernel.org/tip/c845cb8dbd2e1a804babfd13648026c3a7cfbc0b
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:00:57 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 30 Dec 2024 19:05:50 +01:00
x86/mce: Make several functions return bool
Make several functions that return 0 or 1 return a boolean value for
better readability.
No functional changes are intended.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20241212140103.66964-2-qiuxu.zhuo@intel.com
---
arch/x86/include/asm/mce.h | 4 ++--
arch/x86/kernel/cpu/mce/amd.c | 10 +++++-----
arch/x86/kernel/cpu/mce/core.c | 22 +++++++++++-----------
arch/x86/kernel/cpu/mce/intel.c | 9 +++++----
4 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4543cf2..ea9ca76 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -276,7 +276,7 @@ static inline void cmci_rediscover(void) {}
static inline void cmci_recheck(void) {}
#endif
-int mce_available(struct cpuinfo_x86 *c);
+bool mce_available(struct cpuinfo_x86 *c);
bool mce_is_memory_error(struct mce *m);
bool mce_is_correctable(struct mce *m);
bool mce_usable_address(struct mce *m);
@@ -296,7 +296,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-int mce_notify_irq(void);
+bool mce_notify_irq(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 6ca80ff..018874b 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -381,7 +381,7 @@ static bool lvt_interrupt_supported(unsigned int bank, u32 msr_high_bits)
return msr_high_bits & BIT(28);
}
-static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
+static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
{
int msr = (hi & MASK_LVTOFF_HI) >> 20;
@@ -389,7 +389,7 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
if (apic != msr) {
@@ -399,15 +399,15 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
* was set is reserved. Return early here:
*/
if (mce_flags.smca)
- return 0;
+ return false;
pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n",
b->cpu, apic, b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
- return 1;
+ return true;
};
/* Reprogram MCx_MISC MSR behind this threshold bank. */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 7fb5556..167965b 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -492,10 +492,10 @@ static noinstr void mce_gather_info(struct mce_hw_err *err, struct pt_regs *regs
}
}
-int mce_available(struct cpuinfo_x86 *c)
+bool mce_available(struct cpuinfo_x86 *c)
{
if (mca_cfg.disabled)
- return 0;
+ return false;
return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
}
@@ -1778,7 +1778,7 @@ static void mce_timer_delete_all(void)
* Can be called from interrupt context, but not from machine check/NMI
* context.
*/
-int mce_notify_irq(void)
+bool mce_notify_irq(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1789,9 +1789,9 @@ int mce_notify_irq(void)
if (__ratelimit(&ratelimit))
pr_info(HW_ERR "Machine check events logged\n");
- return 1;
+ return true;
}
- return 0;
+ return false;
}
EXPORT_SYMBOL_GPL(mce_notify_irq);
@@ -2015,25 +2015,25 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
return 0;
}
-static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
{
if (c->x86 != 5)
- return 0;
+ return false;
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
mce_flags.p5 = 1;
- return 1;
+ return true;
case X86_VENDOR_CENTAUR:
winchip_mcheck_init(c);
mce_flags.winchip = 1;
- return 1;
+ return true;
default:
- return 0;
+ return false;
}
- return 0;
+ return false;
}
/*
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index b3cd2c6..f863df0 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -75,12 +75,12 @@ static u16 cmci_threshold[MAX_NR_BANKS];
*/
#define CMCI_STORM_THRESHOLD 32749
-static int cmci_supported(int *banks)
+static bool cmci_supported(int *banks)
{
u64 cap;
if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
- return 0;
+ return false;
/*
* Vendor check is not strictly needed, but the initial
@@ -89,10 +89,11 @@ static int cmci_supported(int *banks)
*/
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
- return 0;
+ return false;
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
- return 0;
+ return false;
+
rdmsrl(MSR_IA32_MCG_CAP, cap);
*banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);The 'storm' variable points to this_cpu_ptr(&storm_desc). Access the
'stormy_bank_count' field through the 'storm' to avoid calling
this_cpu_*() on the same per-CPU variable twice.
This minor optimization reduces the text size by 16 bytes.
$ size threshold.o.*
text data bss dec hex filename
1395 1664 0 3059 bf3 threshold.o.old
1379 1664 0 3043 be3 threshold.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- No changes.
Changes in v4:
- No changes.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index 89e31e1e5c9c..f4a007616468 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -90,7 +90,7 @@ void cmci_storm_end(unsigned int bank)
storm->banks[bank].in_storm_mode = false;
/* If no banks left in storm mode, stop polling. */
- if (!this_cpu_dec_return(storm_desc.stormy_bank_count))
+ if (!--storm->stormy_bank_count)
mce_timer_kick(false);
}
--
2.17.1On Thu, Dec 12, 2024 at 10:00:58PM +0800, Qiuxu Zhuo wrote: > The 'storm' variable points to this_cpu_ptr(&storm_desc). Access the > 'stormy_bank_count' field through the 'storm' to avoid calling > this_cpu_*() on the same per-CPU variable twice. > > This minor optimization reduces the text size by 16 bytes. > > $ size threshold.o.* > text data bss dec hex filename > 1395 1664 0 3059 bf3 threshold.o.old > 1379 1664 0 3043 be3 threshold.o.new > > No functional changes intended. > > Reviewed-by: Tony Luck <tony.luck@intel.com> > Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> > Reviewed-by: Sohil Mehta <sohil.mehta@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks, Yazen
The following commit has been merged into the ras/core branch of tip:
Commit-ID: 64a668fbea1b6ec06ddca66d09cc49352f063342
Gitweb: https://git.kernel.org/tip/64a668fbea1b6ec06ddca66d09cc49352f063342
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:00:58 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 30 Dec 2024 19:45:03 +01:00
x86/mce/threshold: Remove the redundant this_cpu_dec_return()
The 'storm' variable points to this_cpu_ptr(&storm_desc). Access the
'stormy_bank_count' field through the 'storm' to avoid calling
this_cpu_*() on the same per-CPU variable twice.
This minor optimization reduces the text size by 16 bytes.
$ size threshold.o.*
text data bss dec hex filename
1395 1664 0 3059 bf3 threshold.o.old
1379 1664 0 3043 be3 threshold.o.new
No functional changes intended.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20241212140103.66964-3-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index 89e31e1..f4a0076 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -90,7 +90,7 @@ void cmci_storm_end(unsigned int bank)
storm->banks[bank].in_storm_mode = false;
/* If no banks left in storm mode, stop polling. */
- if (!this_cpu_dec_return(storm_desc.stormy_bank_count))
+ if (!--storm->stormy_bank_count)
mce_timer_kick(false);
}Make those functions whose callers only care about success or failure
return a boolean value for better readability. Also, update the call
sites accordingly as the polarities of all the return values have been
flipped.
No functional changes.
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- Collect "Reviewed-by:" from Sohil.
- Mention the polarities of return values are flipped in the commit message.
Changes in v4:
- New patch.
arch/x86/kernel/cpu/mce/core.c | 12 ++++++------
arch/x86/kernel/cpu/mce/genpool.c | 29 ++++++++++++++---------------
arch/x86/kernel/cpu/mce/internal.h | 4 ++--
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 167965bd2ac0..ce6fe5e20805 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -151,7 +151,7 @@ EXPORT_PER_CPU_SYMBOL_GPL(injectm);
void mce_log(struct mce_hw_err *err)
{
- if (!mce_gen_pool_add(err))
+ if (mce_gen_pool_add(err))
irq_work_queue(&mce_irq_work);
}
EXPORT_SYMBOL_GPL(mce_log);
@@ -1911,14 +1911,14 @@ static void __mcheck_cpu_check_banks(void)
}
/* Add per CPU specific workarounds here */
-static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
pr_info("unknown CPU type - not enabling MCE support\n");
- return -EOPNOTSUPP;
+ return false;
}
/* This should be disabled by the BIOS, but isn't always */
@@ -2012,7 +2012,7 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
if (cfg->bootlog != 0)
cfg->panic_timeout = 30;
- return 0;
+ return true;
}
static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
@@ -2279,12 +2279,12 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
__mcheck_cpu_cap_init();
- if (__mcheck_cpu_apply_quirks(c) < 0) {
+ if (!__mcheck_cpu_apply_quirks(c)) {
mca_cfg.disabled = 1;
return;
}
- if (mce_gen_pool_init()) {
+ if (!mce_gen_pool_init()) {
mca_cfg.disabled = 1;
pr_emerg("Couldn't allocate MCE records pool!\n");
return;
diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
index d0be6dda0c14..3ca9c007a666 100644
--- a/arch/x86/kernel/cpu/mce/genpool.c
+++ b/arch/x86/kernel/cpu/mce/genpool.c
@@ -94,64 +94,63 @@ bool mce_gen_pool_empty(void)
return llist_empty(&mce_event_llist);
}
-int mce_gen_pool_add(struct mce_hw_err *err)
+bool mce_gen_pool_add(struct mce_hw_err *err)
{
struct mce_evt_llist *node;
if (filter_mce(&err->m))
- return -EINVAL;
+ return false;
if (!mce_evt_pool)
- return -EINVAL;
+ return false;
node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
if (!node) {
pr_warn_ratelimited("MCE records pool full!\n");
- return -ENOMEM;
+ return false;
}
memcpy(&node->err, err, sizeof(*err));
llist_add(&node->llnode, &mce_event_llist);
- return 0;
+ return true;
}
-static int mce_gen_pool_create(void)
+static bool mce_gen_pool_create(void)
{
int mce_numrecords, mce_poolsz, order;
struct gen_pool *gpool;
- int ret = -ENOMEM;
void *mce_pool;
order = order_base_2(sizeof(struct mce_evt_llist));
gpool = gen_pool_create(order, -1);
if (!gpool)
- return ret;
+ return false;
mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
mce_poolsz = mce_numrecords * (1 << order);
mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
if (!mce_pool) {
gen_pool_destroy(gpool);
- return ret;
+ return false;
}
- ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
- if (ret) {
+
+ if (gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1)) {
gen_pool_destroy(gpool);
kfree(mce_pool);
- return ret;
+ return false;
}
mce_evt_pool = gpool;
- return ret;
+ return true;
}
-int mce_gen_pool_init(void)
+bool mce_gen_pool_init(void)
{
/* Just init mce_gen_pool once. */
if (mce_evt_pool)
- return 0;
+ return true;
return mce_gen_pool_create();
}
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 84f810598231..95a504ece43e 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -31,8 +31,8 @@ struct mce_evt_llist {
void mce_gen_pool_process(struct work_struct *__unused);
bool mce_gen_pool_empty(void);
-int mce_gen_pool_add(struct mce_hw_err *err);
-int mce_gen_pool_init(void);
+bool mce_gen_pool_add(struct mce_hw_err *err);
+bool mce_gen_pool_init(void);
struct llist_node *mce_gen_pool_prepare_records(void);
int mce_severity(struct mce *a, struct pt_regs *regs, char **msg, bool is_excp);
--
2.17.1On Thu, Dec 12, 2024 at 10:00:59PM +0800, Qiuxu Zhuo wrote:
> Make those functions whose callers only care about success or failure
> return a boolean value for better readability. Also, update the call
> sites accordingly as the polarities of all the return values have been
> flipped.
>
> No functional changes.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
But one thought below...
[...]
> -static int mce_gen_pool_create(void)
> +static bool mce_gen_pool_create(void)
> {
> int mce_numrecords, mce_poolsz, order;
> struct gen_pool *gpool;
> - int ret = -ENOMEM;
> void *mce_pool;
>
> order = order_base_2(sizeof(struct mce_evt_llist));
> gpool = gen_pool_create(order, -1);
> if (!gpool)
> - return ret;
> + return false;
>
> mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
> mce_poolsz = mce_numrecords * (1 << order);
> mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
> if (!mce_pool) {
> gen_pool_destroy(gpool);
> - return ret;
> + return false;
> }
> - ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
> - if (ret) {
> +
> + if (gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1)) {
> gen_pool_destroy(gpool);
> kfree(mce_pool);
> - return ret;
> + return false;
> }
>
> mce_evt_pool = gpool;
>
> - return ret;
> + return true;
> }
>
It seems like this segment could make use of the helpers in
<linux/cleanup.h> (maybe as future work).
Thanks,
Yazen
The following commit has been merged into the ras/core branch of tip:
Commit-ID: c46945c9cac8437a674edb9d8fbe71511fb4acee
Gitweb: https://git.kernel.org/tip/c46945c9cac8437a674edb9d8fbe71511fb4acee
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:00:59 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 30 Dec 2024 22:06:36 +01:00
x86/mce: Make four functions return bool
Make those functions whose callers only care about success or failure return
a boolean value for better readability. Also, update the call sites
accordingly as the polarities of all the return values have been flipped.
No functional changes.
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20241212140103.66964-4-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/core.c | 12 ++++++------
arch/x86/kernel/cpu/mce/genpool.c | 29 ++++++++++++++---------------
arch/x86/kernel/cpu/mce/internal.h | 4 ++--
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 167965b..ce6fe5e 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -151,7 +151,7 @@ EXPORT_PER_CPU_SYMBOL_GPL(injectm);
void mce_log(struct mce_hw_err *err)
{
- if (!mce_gen_pool_add(err))
+ if (mce_gen_pool_add(err))
irq_work_queue(&mce_irq_work);
}
EXPORT_SYMBOL_GPL(mce_log);
@@ -1911,14 +1911,14 @@ static void __mcheck_cpu_check_banks(void)
}
/* Add per CPU specific workarounds here */
-static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
pr_info("unknown CPU type - not enabling MCE support\n");
- return -EOPNOTSUPP;
+ return false;
}
/* This should be disabled by the BIOS, but isn't always */
@@ -2012,7 +2012,7 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
if (cfg->bootlog != 0)
cfg->panic_timeout = 30;
- return 0;
+ return true;
}
static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
@@ -2279,12 +2279,12 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
__mcheck_cpu_cap_init();
- if (__mcheck_cpu_apply_quirks(c) < 0) {
+ if (!__mcheck_cpu_apply_quirks(c)) {
mca_cfg.disabled = 1;
return;
}
- if (mce_gen_pool_init()) {
+ if (!mce_gen_pool_init()) {
mca_cfg.disabled = 1;
pr_emerg("Couldn't allocate MCE records pool!\n");
return;
diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
index d0be6dd..3ca9c00 100644
--- a/arch/x86/kernel/cpu/mce/genpool.c
+++ b/arch/x86/kernel/cpu/mce/genpool.c
@@ -94,64 +94,63 @@ bool mce_gen_pool_empty(void)
return llist_empty(&mce_event_llist);
}
-int mce_gen_pool_add(struct mce_hw_err *err)
+bool mce_gen_pool_add(struct mce_hw_err *err)
{
struct mce_evt_llist *node;
if (filter_mce(&err->m))
- return -EINVAL;
+ return false;
if (!mce_evt_pool)
- return -EINVAL;
+ return false;
node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
if (!node) {
pr_warn_ratelimited("MCE records pool full!\n");
- return -ENOMEM;
+ return false;
}
memcpy(&node->err, err, sizeof(*err));
llist_add(&node->llnode, &mce_event_llist);
- return 0;
+ return true;
}
-static int mce_gen_pool_create(void)
+static bool mce_gen_pool_create(void)
{
int mce_numrecords, mce_poolsz, order;
struct gen_pool *gpool;
- int ret = -ENOMEM;
void *mce_pool;
order = order_base_2(sizeof(struct mce_evt_llist));
gpool = gen_pool_create(order, -1);
if (!gpool)
- return ret;
+ return false;
mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
mce_poolsz = mce_numrecords * (1 << order);
mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
if (!mce_pool) {
gen_pool_destroy(gpool);
- return ret;
+ return false;
}
- ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
- if (ret) {
+
+ if (gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1)) {
gen_pool_destroy(gpool);
kfree(mce_pool);
- return ret;
+ return false;
}
mce_evt_pool = gpool;
- return ret;
+ return true;
}
-int mce_gen_pool_init(void)
+bool mce_gen_pool_init(void)
{
/* Just init mce_gen_pool once. */
if (mce_evt_pool)
- return 0;
+ return true;
return mce_gen_pool_create();
}
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 84f8105..95a504e 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -31,8 +31,8 @@ struct mce_evt_llist {
void mce_gen_pool_process(struct work_struct *__unused);
bool mce_gen_pool_empty(void);
-int mce_gen_pool_add(struct mce_hw_err *err);
-int mce_gen_pool_init(void);
+bool mce_gen_pool_add(struct mce_hw_err *err);
+bool mce_gen_pool_init(void);
struct llist_node *mce_gen_pool_prepare_records(void);
int mce_severity(struct mce *a, struct pt_regs *regs, char **msg, bool is_excp);From: Tony Luck <tony.luck@intel.com>
Split each vendor specific part into its own helper function.
Signed-off-by: Tony Luck <tony.luck@intel.com>
Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- Collect "Reviewed-by:" from Sohil.
Changes in v4:
- Add necessary blank lines in apply_quirks_amd() (Yazen).
- Use 'mca_cfg' instead of 'cfg' in apply_quirks_*(). (Yazen).
Changes in v3:
- New patch.
arch/x86/kernel/cpu/mce/core.c | 192 ++++++++++++++++++---------------
1 file changed, 104 insertions(+), 88 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index ce6fe5e20805..3855ec2ed0e0 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1910,101 +1910,117 @@ static void __mcheck_cpu_check_banks(void)
}
}
+static void apply_quirks_amd(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+
+ /* This should be disabled by the BIOS, but isn't always */
+ if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
+ /*
+ * disable GART TBL walk error reporting, which
+ * trips off incorrectly with the IOMMU & 3ware
+ * & Cerberus:
+ */
+ clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
+ }
+
+ if (c->x86 < 0x11 && mca_cfg.bootlog < 0) {
+ /*
+ * Lots of broken BIOS around that don't clear them
+ * by default and leave crap in there. Don't log:
+ */
+ mca_cfg.bootlog = 0;
+ }
+
+ /*
+ * Various K7s with broken bank 0 around. Always disable
+ * by default.
+ */
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].ctl = 0;
+
+ /*
+ * overflow_recov is supported for F15h Models 00h-0fh
+ * even though we don't have a CPUID bit for it.
+ */
+ if (c->x86 == 0x15 && c->x86_model <= 0xf)
+ mce_flags.overflow_recov = 1;
+
+ if (c->x86 >= 0x17 && c->x86 <= 0x1A)
+ mce_flags.zen_ifu_quirk = 1;
+}
+
+static void apply_quirks_intel(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+
+ /*
+ * SDM documents that on family 6 bank 0 should not be written
+ * because it aliases to another special BIOS controlled
+ * register.
+ * But it's not aliased anymore on model 0x1a+
+ * Don't ignore bank 0 completely because there could be a
+ * valid event later, merely don't write CTL0.
+ */
+ if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].init = false;
+
+ /*
+ * All newer Intel systems support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
+ mca_cfg.monarch_timeout < 0)
+ mca_cfg.monarch_timeout = USEC_PER_SEC;
+
+ /*
+ * There are also broken BIOSes on some Pentium M and
+ * earlier systems:
+ */
+ if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0)
+ mca_cfg.bootlog = 0;
+
+ if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
+ mce_flags.snb_ifu_quirk = 1;
+
+ /*
+ * Skylake, Cascacde Lake and Cooper Lake require a quirk on
+ * rep movs.
+ */
+ if (c->x86_vfm == INTEL_SKYLAKE_X)
+ mce_flags.skx_repmov_quirk = 1;
+}
+
+static void apply_quirks_zhaoxin(struct cpuinfo_x86 *c)
+{
+ /*
+ * All newer Zhaoxin CPUs support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
+ if (mca_cfg.monarch_timeout < 0)
+ mca_cfg.monarch_timeout = USEC_PER_SEC;
+ }
+}
+
/* Add per CPU specific workarounds here */
static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
{
- struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
- if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
+ switch (c->x86_vendor) {
+ case X86_VENDOR_UNKNOWN:
pr_info("unknown CPU type - not enabling MCE support\n");
return false;
- }
-
- /* This should be disabled by the BIOS, but isn't always */
- if (c->x86_vendor == X86_VENDOR_AMD) {
- if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
- /*
- * disable GART TBL walk error reporting, which
- * trips off incorrectly with the IOMMU & 3ware
- * & Cerberus:
- */
- clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
- }
- if (c->x86 < 0x11 && cfg->bootlog < 0) {
- /*
- * Lots of broken BIOS around that don't clear them
- * by default and leave crap in there. Don't log:
- */
- cfg->bootlog = 0;
- }
- /*
- * Various K7s with broken bank 0 around. Always disable
- * by default.
- */
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].ctl = 0;
-
- /*
- * overflow_recov is supported for F15h Models 00h-0fh
- * even though we don't have a CPUID bit for it.
- */
- if (c->x86 == 0x15 && c->x86_model <= 0xf)
- mce_flags.overflow_recov = 1;
-
- if (c->x86 >= 0x17 && c->x86 <= 0x1A)
- mce_flags.zen_ifu_quirk = 1;
-
- }
-
- if (c->x86_vendor == X86_VENDOR_INTEL) {
- /*
- * SDM documents that on family 6 bank 0 should not be written
- * because it aliases to another special BIOS controlled
- * register.
- * But it's not aliased anymore on model 0x1a+
- * Don't ignore bank 0 completely because there could be a
- * valid event later, merely don't write CTL0.
- */
-
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].init = false;
-
- /*
- * All newer Intel systems support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
-
- /*
- * There are also broken BIOSes on some Pentium M and
- * earlier systems:
- */
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
- cfg->bootlog = 0;
-
- if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
- mce_flags.snb_ifu_quirk = 1;
-
- /*
- * Skylake, Cascacde Lake and Cooper Lake require a quirk on
- * rep movs.
- */
- if (c->x86_vfm == INTEL_SKYLAKE_X)
- mce_flags.skx_repmov_quirk = 1;
- }
-
- if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
- /*
- * All newer Zhaoxin CPUs support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
- if (cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
- }
+ case X86_VENDOR_AMD:
+ apply_quirks_amd(c);
+ break;
+ case X86_VENDOR_INTEL:
+ apply_quirks_intel(c);
+ break;
+ case X86_VENDOR_ZHAOXIN:
+ apply_quirks_zhaoxin(c);
+ break;
}
if (cfg->monarch_timeout < 0)
--
2.17.1On Thu, Dec 12, 2024 at 10:01:00PM +0800, Qiuxu Zhuo wrote: > From: Tony Luck <tony.luck@intel.com> > > Split each vendor specific part into its own helper function. > > Signed-off-by: Tony Luck <tony.luck@intel.com> > Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > Reviewed-by: Sohil Mehta <sohil.mehta@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks, Yazen
The following commit has been merged into the ras/core branch of tip:
Commit-ID: 51a12c28bb9a043e9444db5bd214b00ec161a639
Gitweb: https://git.kernel.org/tip/51a12c28bb9a043e9444db5bd214b00ec161a639
Author: Tony Luck <tony.luck@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:01:00 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 11:07:05 +01:00
x86/mce: Break up __mcheck_cpu_apply_quirks()
Split each vendor specific part into its own helper function.
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Link: https://lore.kernel.org/r/20241212140103.66964-5-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/core.c | 168 +++++++++++++++++---------------
1 file changed, 92 insertions(+), 76 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index ce6fe5e..3855ec2 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1910,101 +1910,117 @@ static void __mcheck_cpu_check_banks(void)
}
}
-/* Add per CPU specific workarounds here */
-static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+static void apply_quirks_amd(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
- struct mca_config *cfg = &mca_cfg;
-
- if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
- pr_info("unknown CPU type - not enabling MCE support\n");
- return false;
- }
/* This should be disabled by the BIOS, but isn't always */
- if (c->x86_vendor == X86_VENDOR_AMD) {
- if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
- /*
- * disable GART TBL walk error reporting, which
- * trips off incorrectly with the IOMMU & 3ware
- * & Cerberus:
- */
- clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
- }
- if (c->x86 < 0x11 && cfg->bootlog < 0) {
- /*
- * Lots of broken BIOS around that don't clear them
- * by default and leave crap in there. Don't log:
- */
- cfg->bootlog = 0;
- }
+ if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
/*
- * Various K7s with broken bank 0 around. Always disable
- * by default.
+ * disable GART TBL walk error reporting, which
+ * trips off incorrectly with the IOMMU & 3ware
+ * & Cerberus:
*/
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].ctl = 0;
+ clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
+ }
+ if (c->x86 < 0x11 && mca_cfg.bootlog < 0) {
/*
- * overflow_recov is supported for F15h Models 00h-0fh
- * even though we don't have a CPUID bit for it.
+ * Lots of broken BIOS around that don't clear them
+ * by default and leave crap in there. Don't log:
*/
- if (c->x86 == 0x15 && c->x86_model <= 0xf)
- mce_flags.overflow_recov = 1;
+ mca_cfg.bootlog = 0;
+ }
- if (c->x86 >= 0x17 && c->x86 <= 0x1A)
- mce_flags.zen_ifu_quirk = 1;
+ /*
+ * Various K7s with broken bank 0 around. Always disable
+ * by default.
+ */
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].ctl = 0;
- }
+ /*
+ * overflow_recov is supported for F15h Models 00h-0fh
+ * even though we don't have a CPUID bit for it.
+ */
+ if (c->x86 == 0x15 && c->x86_model <= 0xf)
+ mce_flags.overflow_recov = 1;
- if (c->x86_vendor == X86_VENDOR_INTEL) {
- /*
- * SDM documents that on family 6 bank 0 should not be written
- * because it aliases to another special BIOS controlled
- * register.
- * But it's not aliased anymore on model 0x1a+
- * Don't ignore bank 0 completely because there could be a
- * valid event later, merely don't write CTL0.
- */
+ if (c->x86 >= 0x17 && c->x86 <= 0x1A)
+ mce_flags.zen_ifu_quirk = 1;
+}
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].init = false;
+static void apply_quirks_intel(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
- /*
- * All newer Intel systems support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
+ /*
+ * SDM documents that on family 6 bank 0 should not be written
+ * because it aliases to another special BIOS controlled
+ * register.
+ * But it's not aliased anymore on model 0x1a+
+ * Don't ignore bank 0 completely because there could be a
+ * valid event later, merely don't write CTL0.
+ */
+ if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].init = false;
- /*
- * There are also broken BIOSes on some Pentium M and
- * earlier systems:
- */
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
- cfg->bootlog = 0;
+ /*
+ * All newer Intel systems support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
+ mca_cfg.monarch_timeout < 0)
+ mca_cfg.monarch_timeout = USEC_PER_SEC;
+
+ /*
+ * There are also broken BIOSes on some Pentium M and
+ * earlier systems:
+ */
+ if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0)
+ mca_cfg.bootlog = 0;
- if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
- mce_flags.snb_ifu_quirk = 1;
+ if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
+ mce_flags.snb_ifu_quirk = 1;
- /*
- * Skylake, Cascacde Lake and Cooper Lake require a quirk on
- * rep movs.
- */
- if (c->x86_vfm == INTEL_SKYLAKE_X)
- mce_flags.skx_repmov_quirk = 1;
+ /*
+ * Skylake, Cascacde Lake and Cooper Lake require a quirk on
+ * rep movs.
+ */
+ if (c->x86_vfm == INTEL_SKYLAKE_X)
+ mce_flags.skx_repmov_quirk = 1;
+}
+
+static void apply_quirks_zhaoxin(struct cpuinfo_x86 *c)
+{
+ /*
+ * All newer Zhaoxin CPUs support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
+ if (mca_cfg.monarch_timeout < 0)
+ mca_cfg.monarch_timeout = USEC_PER_SEC;
}
+}
- if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
- /*
- * All newer Zhaoxin CPUs support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
- if (cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
- }
+/* Add per CPU specific workarounds here */
+static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+{
+ struct mca_config *cfg = &mca_cfg;
+
+ switch (c->x86_vendor) {
+ case X86_VENDOR_UNKNOWN:
+ pr_info("unknown CPU type - not enabling MCE support\n");
+ return false;
+ case X86_VENDOR_AMD:
+ apply_quirks_amd(c);
+ break;
+ case X86_VENDOR_INTEL:
+ apply_quirks_intel(c);
+ break;
+ case X86_VENDOR_ZHAOXIN:
+ apply_quirks_zhaoxin(c);
+ break;
}
if (cfg->monarch_timeout < 0)Convert family/model mixed checks to VFM-based checks to make the code
more compact. Also, as 'mce_num_banks' is an unsigned int, simplify the
check from 'if (this_cpu_read(mce_num_banks) > 0)' to
'if (this_cpu_read(mce_num_banks))'.
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- Collect "Reviewed-by:" from Sohil & Yazen.
- Reduce 'if (mce_num_banks > 0)' to 'if (mce_num_banks)' - Yazen.
Changes in v4:
- No changes but rebased.
Changes in v3:
- Newly added.
arch/x86/kernel/cpu/mce/core.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 3855ec2ed0e0..f90cbcb31a62 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1936,7 +1936,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
* Various K7s with broken bank 0 around. Always disable
* by default.
*/
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks))
mce_banks[0].ctl = 0;
/*
@@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+ /* Older CPUs (prior to family 6) don't need quirks. */
+ if (c->x86_vfm < INTEL_PENTIUM_PRO)
+ return;
+
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1962,22 +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
* Don't ignore bank 0 completely because there could be a
* valid event later, merely don't write CTL0.
*/
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks))
mce_banks[0].init = false;
/*
* All newer Intel systems support MCE broadcasting. Enable
* synchronization with a one second timeout.
*/
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- mca_cfg.monarch_timeout < 0)
+ if (c->x86_vfm >= INTEL_CORE_YONAH && mca_cfg.monarch_timeout < 0)
mca_cfg.monarch_timeout = USEC_PER_SEC;
/*
* There are also broken BIOSes on some Pentium M and
* earlier systems:
*/
- if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && mca_cfg.bootlog < 0)
mca_cfg.bootlog = 0;
if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
--
2.17.1The following commit has been merged into the ras/core branch of tip:
Commit-ID: 359d7a98e3e3f88dbf45411427b284bb3bbbaea5
Gitweb: https://git.kernel.org/tip/359d7a98e3e3f88dbf45411427b284bb3bbbaea5
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:01:01 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 11:11:08 +01:00
x86/mce: Convert family/model mixed checks to VFM-based checks
Convert family/model mixed checks to VFM-based checks to make the code
more compact. Simplify.
[ bp: Drop the "what" from the commit message - it should be visible from
the diff alone. ]
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20241212140103.66964-6-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/core.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 3855ec2..f90cbcb 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1936,7 +1936,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
* Various K7s with broken bank 0 around. Always disable
* by default.
*/
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks))
mce_banks[0].ctl = 0;
/*
@@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+ /* Older CPUs (prior to family 6) don't need quirks. */
+ if (c->x86_vfm < INTEL_PENTIUM_PRO)
+ return;
+
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1962,22 +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
* Don't ignore bank 0 completely because there could be a
* valid event later, merely don't write CTL0.
*/
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks))
mce_banks[0].init = false;
/*
* All newer Intel systems support MCE broadcasting. Enable
* synchronization with a one second timeout.
*/
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- mca_cfg.monarch_timeout < 0)
+ if (c->x86_vfm >= INTEL_CORE_YONAH && mca_cfg.monarch_timeout < 0)
mca_cfg.monarch_timeout = USEC_PER_SEC;
/*
* There are also broken BIOSes on some Pentium M and
* earlier systems:
*/
- if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && mca_cfg.bootlog < 0)
mca_cfg.bootlog = 0;
if (c->x86_vfm == INTEL_SANDYBRIDGE_X)Get HYGON to directly call mce_amd_feature_init() and remove the
redundant mce_hygon_feature_init().
Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- New patch.
arch/x86/include/asm/mce.h | 2 --
arch/x86/kernel/cpu/mce/core.c | 8 ++------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index ea9ca7689f6b..eb2db07ef39c 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -386,8 +386,6 @@ static inline bool amd_mce_is_memory_error(struct mce *m) { return false; };
static inline void mce_amd_feature_init(struct cpuinfo_x86 *c) { }
#endif
-static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_amd_feature_init(c); }
-
unsigned long copy_mc_fragile_handle_tail(char *to, char *from, unsigned len);
#endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index f90cbcb31a62..0dc00c9894c7 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2118,14 +2118,10 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
mce_intel_feature_init(c);
break;
- case X86_VENDOR_AMD: {
+ case X86_VENDOR_AMD:
+ case X86_VENDOR_HYGON:
mce_amd_feature_init(c);
break;
- }
-
- case X86_VENDOR_HYGON:
- mce_hygon_feature_init(c);
- break;
case X86_VENDOR_CENTAUR:
mce_centaur_feature_init(c);
--
2.17.1On Thu, Dec 12, 2024 at 10:01:02PM +0800, Qiuxu Zhuo wrote:
> Get HYGON to directly call mce_amd_feature_init() and remove the
> redundant mce_hygon_feature_init().
>
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Changes in v5:
> - New patch.
>
> arch/x86/include/asm/mce.h | 2 --
> arch/x86/kernel/cpu/mce/core.c | 8 ++------
> 2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index ea9ca7689f6b..eb2db07ef39c 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -386,8 +386,6 @@ static inline bool amd_mce_is_memory_error(struct mce *m) { return false; };
> static inline void mce_amd_feature_init(struct cpuinfo_x86 *c) { }
> #endif
>
> -static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_amd_feature_init(c); }
> -
Another cleanup idea: move functions from <asm/mce.h> to "internal.h".
Thanks,
Yazen
On 12/12/2024 6:01 AM, Qiuxu Zhuo wrote: > Get HYGON to directly call mce_amd_feature_init() and remove the > redundant mce_hygon_feature_init(). > > Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > --- Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
The following commit has been merged into the ras/core branch of tip:
Commit-ID: 053d18057e6292462f1b3f9460dd0c1e34609f67
Gitweb: https://git.kernel.org/tip/053d18057e6292462f1b3f9460dd0c1e34609f67
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:01:02 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 11:12:45 +01:00
x86/mce: Remove the redundant mce_hygon_feature_init()
Get HYGON to directly call mce_amd_feature_init() and remove the redundant
mce_hygon_feature_init().
Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20241212140103.66964-7-qiuxu.zhuo@intel.com
---
arch/x86/include/asm/mce.h | 2 --
arch/x86/kernel/cpu/mce/core.c | 8 ++------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index ea9ca76..eb2db07 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -386,8 +386,6 @@ static inline bool amd_mce_is_memory_error(struct mce *m) { return false; };
static inline void mce_amd_feature_init(struct cpuinfo_x86 *c) { }
#endif
-static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_amd_feature_init(c); }
-
unsigned long copy_mc_fragile_handle_tail(char *to, char *from, unsigned len);
#endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index f90cbcb..0dc00c9 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2118,13 +2118,9 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
mce_intel_feature_init(c);
break;
- case X86_VENDOR_AMD: {
- mce_amd_feature_init(c);
- break;
- }
-
+ case X86_VENDOR_AMD:
case X86_VENDOR_HYGON:
- mce_hygon_feature_init(c);
+ mce_amd_feature_init(c);
break;
case X86_VENDOR_CENTAUR:Remove unnecessary NULL pointer initializations from variables that
are already initialized before use.
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v5:
- Collect "Reviewed-by:" Yazen.
Changes in v4:
- No changes.
Changes in v3:
- Collect "Reviewed-by:" Nikolay & Sohil.
- Remove the variables' names from the commit message (Sohil).
arch/x86/kernel/cpu/mce/amd.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 018874b554cb..c79a82912d38 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -921,8 +921,8 @@ static void log_and_reset_block(struct threshold_block *block)
*/
static void amd_threshold_interrupt(void)
{
- struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
struct threshold_bank **bp = this_cpu_read(threshold_banks);
+ struct threshold_block *first_block, *block, *tmp;
unsigned int bank, cpu = smp_processor_id();
/*
@@ -1201,8 +1201,7 @@ static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb
static int __threshold_add_blocks(struct threshold_bank *b)
{
struct list_head *head = &b->blocks->miscj;
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
int err = 0;
err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name);
@@ -1312,8 +1311,7 @@ static void deallocate_threshold_blocks(struct threshold_bank *bank)
static void __threshold_remove_blocks(struct threshold_bank *b)
{
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
kobject_put(b->kobj);
--
2.17.1The following commit has been merged into the ras/core branch of tip:
Commit-ID: 0a6c5391f8812323300057f4dbb89b8bf886ee8e
Gitweb: https://git.kernel.org/tip/0a6c5391f8812323300057f4dbb89b8bf886ee8e
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Thu, 12 Dec 2024 22:01:03 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 11:14:36 +01:00
x86/mce/amd: Remove unnecessary NULL pointer initializations
Remove unnecessary NULL pointer initializations from variables that
are already initialized before use.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20241212140103.66964-8-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/amd.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 018874b..c79a829 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -921,8 +921,8 @@ static void log_and_reset_block(struct threshold_block *block)
*/
static void amd_threshold_interrupt(void)
{
- struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
struct threshold_bank **bp = this_cpu_read(threshold_banks);
+ struct threshold_block *first_block, *block, *tmp;
unsigned int bank, cpu = smp_processor_id();
/*
@@ -1201,8 +1201,7 @@ out_free:
static int __threshold_add_blocks(struct threshold_bank *b)
{
struct list_head *head = &b->blocks->miscj;
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
int err = 0;
err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name);
@@ -1312,8 +1311,7 @@ static void deallocate_threshold_blocks(struct threshold_bank *bank)
static void __threshold_remove_blocks(struct threshold_bank *b)
{
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
kobject_put(b->kobj);On Wed, Jan 01, 2025 at 11:44:03AM -0000, tip-bot2 for Qiuxu Zhuo wrote:
> The following commit has been merged into the ras/core branch of tip:
>
> Commit-ID: 0a6c5391f8812323300057f4dbb89b8bf886ee8e
> Gitweb: https://git.kernel.org/tip/0a6c5391f8812323300057f4dbb89b8bf886ee8e
> Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> AuthorDate: Thu, 12 Dec 2024 22:01:03 +08:00
> Committer: Borislav Petkov (AMD) <bp@alien8.de>
> CommitterDate: Tue, 31 Dec 2024 11:14:36 +01:00
>
> x86/mce/amd: Remove unnecessary NULL pointer initializations
>
> Remove unnecessary NULL pointer initializations from variables that
> are already initialized before use.
>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Link: https://lore.kernel.org/r/20241212140103.66964-8-qiuxu.zhuo@intel.com
Zapping this one in favor of:
https://lore.kernel.org/r/20241206161210.163701-2-yazen.ghannam@amd.com
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Make several functions that return 0 or 1 return a boolean value for
better readability.
No functional changes are intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- Don't rename mce_notify_irq() (Boris).
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Rename mce_notify_irq() to mce_notify_user() (Sohil).
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/include/asm/mce.h | 4 ++--
arch/x86/kernel/cpu/mce/amd.c | 10 +++++-----
arch/x86/kernel/cpu/mce/core.c | 22 +++++++++++-----------
arch/x86/kernel/cpu/mce/intel.c | 9 +++++----
4 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4543cf2eb5e8..ea9ca7689f6b 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -276,7 +276,7 @@ static inline void cmci_rediscover(void) {}
static inline void cmci_recheck(void) {}
#endif
-int mce_available(struct cpuinfo_x86 *c);
+bool mce_available(struct cpuinfo_x86 *c);
bool mce_is_memory_error(struct mce *m);
bool mce_is_correctable(struct mce *m);
bool mce_usable_address(struct mce *m);
@@ -296,7 +296,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-int mce_notify_irq(void);
+bool mce_notify_irq(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 6ca80fff1fea..018874b554cb 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -381,7 +381,7 @@ static bool lvt_interrupt_supported(unsigned int bank, u32 msr_high_bits)
return msr_high_bits & BIT(28);
}
-static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
+static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
{
int msr = (hi & MASK_LVTOFF_HI) >> 20;
@@ -389,7 +389,7 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
if (apic != msr) {
@@ -399,15 +399,15 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
* was set is reserved. Return early here:
*/
if (mce_flags.smca)
- return 0;
+ return false;
pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n",
b->cpu, apic, b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
- return 1;
+ return true;
};
/* Reprogram MCx_MISC MSR behind this threshold bank. */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 7fb5556a0b53..167965bd2ac0 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -492,10 +492,10 @@ static noinstr void mce_gather_info(struct mce_hw_err *err, struct pt_regs *regs
}
}
-int mce_available(struct cpuinfo_x86 *c)
+bool mce_available(struct cpuinfo_x86 *c)
{
if (mca_cfg.disabled)
- return 0;
+ return false;
return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
}
@@ -1778,7 +1778,7 @@ static void mce_timer_delete_all(void)
* Can be called from interrupt context, but not from machine check/NMI
* context.
*/
-int mce_notify_irq(void)
+bool mce_notify_irq(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1789,9 +1789,9 @@ int mce_notify_irq(void)
if (__ratelimit(&ratelimit))
pr_info(HW_ERR "Machine check events logged\n");
- return 1;
+ return true;
}
- return 0;
+ return false;
}
EXPORT_SYMBOL_GPL(mce_notify_irq);
@@ -2015,25 +2015,25 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
return 0;
}
-static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
{
if (c->x86 != 5)
- return 0;
+ return false;
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
mce_flags.p5 = 1;
- return 1;
+ return true;
case X86_VENDOR_CENTAUR:
winchip_mcheck_init(c);
mce_flags.winchip = 1;
- return 1;
+ return true;
default:
- return 0;
+ return false;
}
- return 0;
+ return false;
}
/*
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index b3cd2c61b11d..f863df0ff42c 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -75,12 +75,12 @@ static u16 cmci_threshold[MAX_NR_BANKS];
*/
#define CMCI_STORM_THRESHOLD 32749
-static int cmci_supported(int *banks)
+static bool cmci_supported(int *banks)
{
u64 cap;
if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
- return 0;
+ return false;
/*
* Vendor check is not strictly needed, but the initial
@@ -89,10 +89,11 @@ static int cmci_supported(int *banks)
*/
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
- return 0;
+ return false;
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
- return 0;
+ return false;
+
rdmsrl(MSR_IA32_MCG_CAP, cap);
*banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
--
2.17.1The 'storm' variable points to this_cpu_ptr(&storm_desc). Access the
'stormy_bank_count' field through the 'storm' to avoid calling
this_cpu_*() on the same per-CPU variable twice.
This minor optimization reduces the text size by 16 bytes.
$ size threshold.o.*
text data bss dec hex filename
1395 1664 0 3059 bf3 threshold.o.old
1379 1664 0 3043 be3 threshold.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- No changes.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index 89e31e1e5c9c..f4a007616468 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -90,7 +90,7 @@ void cmci_storm_end(unsigned int bank)
storm->banks[bank].in_storm_mode = false;
/* If no banks left in storm mode, stop polling. */
- if (!this_cpu_dec_return(storm_desc.stormy_bank_count))
+ if (!--storm->stormy_bank_count)
mce_timer_kick(false);
}
--
2.17.1Make those functions whose callers only care about success or failure
return a boolean value for better readability. Also, update the call
sites accordingly.
No functional changes.
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- New patch.
arch/x86/kernel/cpu/mce/core.c | 12 ++++++------
arch/x86/kernel/cpu/mce/genpool.c | 29 ++++++++++++++---------------
arch/x86/kernel/cpu/mce/internal.h | 4 ++--
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 167965bd2ac0..ce6fe5e20805 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -151,7 +151,7 @@ EXPORT_PER_CPU_SYMBOL_GPL(injectm);
void mce_log(struct mce_hw_err *err)
{
- if (!mce_gen_pool_add(err))
+ if (mce_gen_pool_add(err))
irq_work_queue(&mce_irq_work);
}
EXPORT_SYMBOL_GPL(mce_log);
@@ -1911,14 +1911,14 @@ static void __mcheck_cpu_check_banks(void)
}
/* Add per CPU specific workarounds here */
-static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
pr_info("unknown CPU type - not enabling MCE support\n");
- return -EOPNOTSUPP;
+ return false;
}
/* This should be disabled by the BIOS, but isn't always */
@@ -2012,7 +2012,7 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
if (cfg->bootlog != 0)
cfg->panic_timeout = 30;
- return 0;
+ return true;
}
static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
@@ -2279,12 +2279,12 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
__mcheck_cpu_cap_init();
- if (__mcheck_cpu_apply_quirks(c) < 0) {
+ if (!__mcheck_cpu_apply_quirks(c)) {
mca_cfg.disabled = 1;
return;
}
- if (mce_gen_pool_init()) {
+ if (!mce_gen_pool_init()) {
mca_cfg.disabled = 1;
pr_emerg("Couldn't allocate MCE records pool!\n");
return;
diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
index d0be6dda0c14..3ca9c007a666 100644
--- a/arch/x86/kernel/cpu/mce/genpool.c
+++ b/arch/x86/kernel/cpu/mce/genpool.c
@@ -94,64 +94,63 @@ bool mce_gen_pool_empty(void)
return llist_empty(&mce_event_llist);
}
-int mce_gen_pool_add(struct mce_hw_err *err)
+bool mce_gen_pool_add(struct mce_hw_err *err)
{
struct mce_evt_llist *node;
if (filter_mce(&err->m))
- return -EINVAL;
+ return false;
if (!mce_evt_pool)
- return -EINVAL;
+ return false;
node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
if (!node) {
pr_warn_ratelimited("MCE records pool full!\n");
- return -ENOMEM;
+ return false;
}
memcpy(&node->err, err, sizeof(*err));
llist_add(&node->llnode, &mce_event_llist);
- return 0;
+ return true;
}
-static int mce_gen_pool_create(void)
+static bool mce_gen_pool_create(void)
{
int mce_numrecords, mce_poolsz, order;
struct gen_pool *gpool;
- int ret = -ENOMEM;
void *mce_pool;
order = order_base_2(sizeof(struct mce_evt_llist));
gpool = gen_pool_create(order, -1);
if (!gpool)
- return ret;
+ return false;
mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
mce_poolsz = mce_numrecords * (1 << order);
mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
if (!mce_pool) {
gen_pool_destroy(gpool);
- return ret;
+ return false;
}
- ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
- if (ret) {
+
+ if (gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1)) {
gen_pool_destroy(gpool);
kfree(mce_pool);
- return ret;
+ return false;
}
mce_evt_pool = gpool;
- return ret;
+ return true;
}
-int mce_gen_pool_init(void)
+bool mce_gen_pool_init(void)
{
/* Just init mce_gen_pool once. */
if (mce_evt_pool)
- return 0;
+ return true;
return mce_gen_pool_create();
}
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 84f810598231..95a504ece43e 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -31,8 +31,8 @@ struct mce_evt_llist {
void mce_gen_pool_process(struct work_struct *__unused);
bool mce_gen_pool_empty(void);
-int mce_gen_pool_add(struct mce_hw_err *err);
-int mce_gen_pool_init(void);
+bool mce_gen_pool_add(struct mce_hw_err *err);
+bool mce_gen_pool_init(void);
struct llist_node *mce_gen_pool_prepare_records(void);
int mce_severity(struct mce *a, struct pt_regs *regs, char **msg, bool is_excp);
--
2.17.1On 11/10/2024 10:04 PM, Qiuxu Zhuo wrote:
> Make those functions whose callers only care about success or failure
> return a boolean value for better readability. Also, update the call
> sites accordingly.
>
> No functional changes.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Changes in v4:
> - New patch.
>
> arch/x86/kernel/cpu/mce/core.c | 12 ++++++------
> arch/x86/kernel/cpu/mce/genpool.c | 29 ++++++++++++++---------------
> arch/x86/kernel/cpu/mce/internal.h | 4 ++--
> 3 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 167965bd2ac0..ce6fe5e20805 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -151,7 +151,7 @@ EXPORT_PER_CPU_SYMBOL_GPL(injectm);
>
> void mce_log(struct mce_hw_err *err)
> {
> - if (!mce_gen_pool_add(err))
> + if (mce_gen_pool_add(err))
> irq_work_queue(&mce_irq_work);
> }
The polarities of all the return values have been flipped. They all look
correct but took me a few minutes to figure out. Might be useful to make
a mention in the commit message if you end up doing another version (Not
needed otherwise).
> EXPORT_SYMBOL_GPL(mce_log);
From: Tony Luck <tony.luck@intel.com>
Split each vendor specific part into its own helper function.
Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- Add necessary blank lines in apply_quirks_amd() (Yazen).
- Use 'mca_cfg' instead of 'cfg' in apply_quirks_*(). (Yazen).
Changes in v3:
- New patch.
arch/x86/kernel/cpu/mce/core.c | 192 ++++++++++++++++++---------------
1 file changed, 104 insertions(+), 88 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index ce6fe5e20805..3855ec2ed0e0 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1910,101 +1910,117 @@ static void __mcheck_cpu_check_banks(void)
}
}
+static void apply_quirks_amd(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+
+ /* This should be disabled by the BIOS, but isn't always */
+ if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
+ /*
+ * disable GART TBL walk error reporting, which
+ * trips off incorrectly with the IOMMU & 3ware
+ * & Cerberus:
+ */
+ clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
+ }
+
+ if (c->x86 < 0x11 && mca_cfg.bootlog < 0) {
+ /*
+ * Lots of broken BIOS around that don't clear them
+ * by default and leave crap in there. Don't log:
+ */
+ mca_cfg.bootlog = 0;
+ }
+
+ /*
+ * Various K7s with broken bank 0 around. Always disable
+ * by default.
+ */
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].ctl = 0;
+
+ /*
+ * overflow_recov is supported for F15h Models 00h-0fh
+ * even though we don't have a CPUID bit for it.
+ */
+ if (c->x86 == 0x15 && c->x86_model <= 0xf)
+ mce_flags.overflow_recov = 1;
+
+ if (c->x86 >= 0x17 && c->x86 <= 0x1A)
+ mce_flags.zen_ifu_quirk = 1;
+}
+
+static void apply_quirks_intel(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+
+ /*
+ * SDM documents that on family 6 bank 0 should not be written
+ * because it aliases to another special BIOS controlled
+ * register.
+ * But it's not aliased anymore on model 0x1a+
+ * Don't ignore bank 0 completely because there could be a
+ * valid event later, merely don't write CTL0.
+ */
+ if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].init = false;
+
+ /*
+ * All newer Intel systems support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
+ mca_cfg.monarch_timeout < 0)
+ mca_cfg.monarch_timeout = USEC_PER_SEC;
+
+ /*
+ * There are also broken BIOSes on some Pentium M and
+ * earlier systems:
+ */
+ if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0)
+ mca_cfg.bootlog = 0;
+
+ if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
+ mce_flags.snb_ifu_quirk = 1;
+
+ /*
+ * Skylake, Cascacde Lake and Cooper Lake require a quirk on
+ * rep movs.
+ */
+ if (c->x86_vfm == INTEL_SKYLAKE_X)
+ mce_flags.skx_repmov_quirk = 1;
+}
+
+static void apply_quirks_zhaoxin(struct cpuinfo_x86 *c)
+{
+ /*
+ * All newer Zhaoxin CPUs support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
+ if (mca_cfg.monarch_timeout < 0)
+ mca_cfg.monarch_timeout = USEC_PER_SEC;
+ }
+}
+
/* Add per CPU specific workarounds here */
static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
{
- struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
- if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
+ switch (c->x86_vendor) {
+ case X86_VENDOR_UNKNOWN:
pr_info("unknown CPU type - not enabling MCE support\n");
return false;
- }
-
- /* This should be disabled by the BIOS, but isn't always */
- if (c->x86_vendor == X86_VENDOR_AMD) {
- if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
- /*
- * disable GART TBL walk error reporting, which
- * trips off incorrectly with the IOMMU & 3ware
- * & Cerberus:
- */
- clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
- }
- if (c->x86 < 0x11 && cfg->bootlog < 0) {
- /*
- * Lots of broken BIOS around that don't clear them
- * by default and leave crap in there. Don't log:
- */
- cfg->bootlog = 0;
- }
- /*
- * Various K7s with broken bank 0 around. Always disable
- * by default.
- */
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].ctl = 0;
-
- /*
- * overflow_recov is supported for F15h Models 00h-0fh
- * even though we don't have a CPUID bit for it.
- */
- if (c->x86 == 0x15 && c->x86_model <= 0xf)
- mce_flags.overflow_recov = 1;
-
- if (c->x86 >= 0x17 && c->x86 <= 0x1A)
- mce_flags.zen_ifu_quirk = 1;
-
- }
-
- if (c->x86_vendor == X86_VENDOR_INTEL) {
- /*
- * SDM documents that on family 6 bank 0 should not be written
- * because it aliases to another special BIOS controlled
- * register.
- * But it's not aliased anymore on model 0x1a+
- * Don't ignore bank 0 completely because there could be a
- * valid event later, merely don't write CTL0.
- */
-
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].init = false;
-
- /*
- * All newer Intel systems support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
-
- /*
- * There are also broken BIOSes on some Pentium M and
- * earlier systems:
- */
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
- cfg->bootlog = 0;
-
- if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
- mce_flags.snb_ifu_quirk = 1;
-
- /*
- * Skylake, Cascacde Lake and Cooper Lake require a quirk on
- * rep movs.
- */
- if (c->x86_vfm == INTEL_SKYLAKE_X)
- mce_flags.skx_repmov_quirk = 1;
- }
-
- if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
- /*
- * All newer Zhaoxin CPUs support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
- if (cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
- }
+ case X86_VENDOR_AMD:
+ apply_quirks_amd(c);
+ break;
+ case X86_VENDOR_INTEL:
+ apply_quirks_intel(c);
+ break;
+ case X86_VENDOR_ZHAOXIN:
+ apply_quirks_zhaoxin(c);
+ break;
}
if (cfg->monarch_timeout < 0)
--
2.17.1On 11/10/2024 10:04 PM, Qiuxu Zhuo wrote: > From: Tony Luck <tony.luck@intel.com> > > Split each vendor specific part into its own helper function. > > Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > Signed-off-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > --- Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Convert family/model mixed checks to VFM-based checks to make
the code more compact.
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- No changes but rebased.
Changes in v3:
- Newly added.
arch/x86/kernel/cpu/mce/core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 3855ec2ed0e0..d288cc7390f6 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+ /* Older CPUs (prior to family 6) don't need quirks. */
+ if (c->x86_vfm < INTEL_PENTIUM_PRO)
+ return;
+
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1962,22 +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
* Don't ignore bank 0 completely because there could be a
* valid event later, merely don't write CTL0.
*/
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
mce_banks[0].init = false;
/*
* All newer Intel systems support MCE broadcasting. Enable
* synchronization with a one second timeout.
*/
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- mca_cfg.monarch_timeout < 0)
+ if (c->x86_vfm >= INTEL_CORE_YONAH && mca_cfg.monarch_timeout < 0)
mca_cfg.monarch_timeout = USEC_PER_SEC;
/*
* There are also broken BIOSes on some Pentium M and
* earlier systems:
*/
- if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && mca_cfg.bootlog < 0)
mca_cfg.bootlog = 0;
if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
--
2.17.1On Mon, Nov 11, 2024 at 02:04:25PM +0800, Qiuxu Zhuo wrote:
> Convert family/model mixed checks to VFM-based checks to make
> the code more compact.
>
> Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> Changes in v4:
> - No changes but rebased.
>
> Changes in v3:
> - Newly added.
>
> arch/x86/kernel/cpu/mce/core.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 3855ec2ed0e0..d288cc7390f6 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
> {
> struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
>
> + /* Older CPUs (prior to family 6) don't need quirks. */
> + if (c->x86_vfm < INTEL_PENTIUM_PRO)
> + return;
> +
Is it possible for pre-"family 6" to get here?
Family 5 is "ancient" which has its own MCE init path. And I assume
anything older doesn't support MCE/MCA. Is this correct?
> /*
> * SDM documents that on family 6 bank 0 should not be written
> * because it aliases to another special BIOS controlled
> @@ -1962,22 +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
> * Don't ignore bank 0 completely because there could be a
> * valid event later, merely don't write CTL0.
> */
> - if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
> + if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
The "> 0" is not needed, since mce_num_banks is unsigned int.
Otherwise, looks good.
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Thanks,
Yazen
Hi Yazen,
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct
> > cpuinfo_x86 *c) {
> > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> >
> > + /* Older CPUs (prior to family 6) don't need quirks. */
> > + if (c->x86_vfm < INTEL_PENTIUM_PRO)
> > + return;
> > +
>
> Is it possible for pre-"family 6" to get here?
>
> Family 5 is "ancient" which has its own MCE init path. And I assume anything
> older doesn't support MCE/MCA. Is this correct?
Yes, there is an early return in __mcheck_cpu_ancient_init() for Family 5.
However, this code explicitly indicates that "prior to families 6 don't need quirks"
and addresses concerns like:
https://lore.kernel.org/all/dcfdba92-7004-413d-8011-12771636d11f@intel.com/
> > /*
> > * SDM documents that on family 6 bank 0 should not be written
> > * because it aliases to another special BIOS controlled @@ -1962,22
> > +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
> > * Don't ignore bank 0 completely because there could be a
> > * valid event later, merely don't write CTL0.
> > */
> > - if (c->x86 == 6 && c->x86_model < 0x1A &&
> this_cpu_read(mce_num_banks) > 0)
> > + if (c->x86_vfm < INTEL_NEHALEM_EP &&
> this_cpu_read(mce_num_banks) >
> > +0)
>
> The "> 0" is not needed, since mce_num_banks is unsigned int.
I don't get your point here.
But it needs to check for the case where mce_num_banks == 0.
> Otherwise, looks good.
>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Thanks!
-Qiuxu
On Wed, Nov 13, 2024 at 12:10:31PM +0000, Zhuo, Qiuxu wrote:
> Hi Yazen,
>
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > > @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct
> > > cpuinfo_x86 *c) {
> > > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> > >
> > > + /* Older CPUs (prior to family 6) don't need quirks. */
> > > + if (c->x86_vfm < INTEL_PENTIUM_PRO)
> > > + return;
> > > +
> >
> > Is it possible for pre-"family 6" to get here?
> >
> > Family 5 is "ancient" which has its own MCE init path. And I assume anything
> > older doesn't support MCE/MCA. Is this correct?
>
> Yes, there is an early return in __mcheck_cpu_ancient_init() for Family 5.
> However, this code explicitly indicates that "prior to families 6 don't need quirks"
> and addresses concerns like:
>
> https://lore.kernel.org/all/dcfdba92-7004-413d-8011-12771636d11f@intel.com/
>
Right, but my point is that this check would never be executed, since
the older systems would not get here during init. So this seems like
dead code.
> > > /*
> > > * SDM documents that on family 6 bank 0 should not be written
> > > * because it aliases to another special BIOS controlled @@ -1962,22
> > > +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
> > > * Don't ignore bank 0 completely because there could be a
> > > * valid event later, merely don't write CTL0.
> > > */
> > > - if (c->x86 == 6 && c->x86_model < 0x1A &&
> > this_cpu_read(mce_num_banks) > 0)
> > > + if (c->x86_vfm < INTEL_NEHALEM_EP &&
> > this_cpu_read(mce_num_banks) >
> > > +0)
> >
> > The "> 0" is not needed, since mce_num_banks is unsigned int.
>
> I don't get your point here.
> But it needs to check for the case where mce_num_banks == 0.
>
The check is "mce_num_banks > 0", and mce_num_banks is an unsigned int.
Therefore, the check is reduced to "mce_num_banks != 0". In this case,
you can just do "if (mce_num_banks)" to the same effect.
Thanks,
Yazen
Hi Yazen, > From: Yazen Ghannam <yazen.ghannam@amd.com> > [...] > > > > + if (c->x86_vfm < INTEL_NEHALEM_EP && > > > this_cpu_read(mce_num_banks) > > > > > +0) > > > > > > The "> 0" is not needed, since mce_num_banks is unsigned int. > > > > I don't get your point here. > > But it needs to check for the case where mce_num_banks == 0. > > > > The check is "mce_num_banks > 0", and mce_num_banks is an unsigned int. > Therefore, the check is reduced to "mce_num_banks != 0". In this case, you > can just do "if (mce_num_banks)" to the same effect. I got you. OK, if nobody else objects, I'll update it in the next version. [ Hope others won't blame this as over-optimization. ] -Qiuxu
On 11/10/2024 10:04 PM, Qiuxu Zhuo wrote: > Convert family/model mixed checks to VFM-based checks to make > the code more compact. > > Suggested-by: Sohil Mehta <sohil.mehta@intel.com> > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > --- Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Remove the unnecessary {} from the case statement.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- No changes.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index d288cc7390f6..0f0c6e9d9183 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2118,10 +2118,9 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
mce_intel_feature_init(c);
break;
- case X86_VENDOR_AMD: {
+ case X86_VENDOR_AMD:
mce_amd_feature_init(c);
break;
- }
case X86_VENDOR_HYGON:
mce_hygon_feature_init(c);
--
2.17.1On Mon, Nov 11, 2024 at 02:04:26PM +0800, Qiuxu Zhuo wrote:
> Remove the unnecessary {} from the case statement.
>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
But please see note below.
> ---
> Changes in v4:
> - No changes.
>
> Changes in v3:
> - Collect "Reviewed-by:" from Nikolay & Sohil.
>
> Changes in v2:
> - Collect "Reviewed-by:" from Tony.
>
> arch/x86/kernel/cpu/mce/core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index d288cc7390f6..0f0c6e9d9183 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -2118,10 +2118,9 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
> mce_intel_feature_init(c);
> break;
>
> - case X86_VENDOR_AMD: {
> + case X86_VENDOR_AMD:
> mce_amd_feature_init(c);
> break;
> - }
>
> case X86_VENDOR_HYGON:
> mce_hygon_feature_init(c);
> --
I think this could be a bit more substantive if you also combine the AMD
and HYGON cases. And remove mce_hygon_feature_init() which just calls
mce_amd_feature_init() anyway.
Thanks,
Yazen
Hi Yazen,
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Thanks!
> But please see note below.
> [...]
> > - case X86_VENDOR_AMD: {
> > + case X86_VENDOR_AMD:
> > mce_amd_feature_init(c);
> > break;
> > - }
> >
> > case X86_VENDOR_HYGON:
> > mce_hygon_feature_init(c);
> > --
>
> I think this could be a bit more substantive if you also combine the AMD and
> HYGON cases. And remove mce_hygon_feature_init() which just calls
> mce_amd_feature_init() anyway.
How about a separate patch for this?
If It's OK for you, I'll follow up on it after the current patch series has settled.
-Qiuxu
On Wed, Nov 13, 2024 at 01:32:08PM +0000, Zhuo, Qiuxu wrote:
> How about a separate patch for this?
Can you drop this micro-change per patch? Just do a single patch here which
fixes up everything mentioned during review in that area and be done with it.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris, > From: Borislav Petkov <bp@alien8.de> > [...] > > Can you drop this micro-change per patch? Just do a single patch here which > fixes up everything mentioned during review in that area and be done with it. OK. I'll drop this one and replace it with Yazen's suggestion in next version. -Qiuxu
On Wed, Nov 13, 2024 at 01:32:08PM +0000, Zhuo, Qiuxu wrote:
> Hi Yazen,
>
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> >
> > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
>
> Thanks!
>
> > But please see note below.
> > [...]
> > > - case X86_VENDOR_AMD: {
> > > + case X86_VENDOR_AMD:
> > > mce_amd_feature_init(c);
> > > break;
> > > - }
> > >
> > > case X86_VENDOR_HYGON:
> > > mce_hygon_feature_init(c);
> > > --
> >
> > I think this could be a bit more substantive if you also combine the AMD and
> > HYGON cases. And remove mce_hygon_feature_init() which just calls
> > mce_amd_feature_init() anyway.
>
> How about a separate patch for this?
> If It's OK for you, I'll follow up on it after the current patch series has settled.
>
Sure thing. No problem.
Thanks,
Yazen
Remove unnecessary NULL pointer initializations from variables that
are already initialized before use.
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- No changes.
Changes in v3:
- Collect "Reviewed-by:" Nikolay & Sohil.
- Remove the variables' names from the commit message (Sohil).
arch/x86/kernel/cpu/mce/amd.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 018874b554cb..c79a82912d38 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -921,8 +921,8 @@ static void log_and_reset_block(struct threshold_block *block)
*/
static void amd_threshold_interrupt(void)
{
- struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
struct threshold_bank **bp = this_cpu_read(threshold_banks);
+ struct threshold_block *first_block, *block, *tmp;
unsigned int bank, cpu = smp_processor_id();
/*
@@ -1201,8 +1201,7 @@ static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb
static int __threshold_add_blocks(struct threshold_bank *b)
{
struct list_head *head = &b->blocks->miscj;
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
int err = 0;
err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name);
@@ -1312,8 +1311,7 @@ static void deallocate_threshold_blocks(struct threshold_bank *bank)
static void __threshold_remove_blocks(struct threshold_bank *b)
{
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
kobject_put(b->kobj);
--
2.17.1On Mon, Nov 11, 2024 at 02:04:27PM +0800, Qiuxu Zhuo wrote: > Remove unnecessary NULL pointer initializations from variables that > are already initialized before use. > > Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> > Reviewed-by: Sohil Mehta <sohil.mehta@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > --- Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks, Yazen
Fix typos in comments.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v4:
- No changes.
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Remove the detail typos from the commit message (Sohil).
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 0f0c6e9d9183..6e194ccffc7c 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
} else {
/*
* Subject: Now start the scanning loop one by one in
- * the original callin order.
+ * the original calling order.
* This way when there are any shared banks it will be
* only seen by one CPU before cleared, avoiding duplicates.
*/
@@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
/* This should be disabled by the BIOS, but isn't always */
if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
/*
- * disable GART TBL walk error reporting, which
+ * disable GART TLB walk error reporting, which
* trips off incorrectly with the IOMMU & 3ware
* & Cerberus:
*/
--
2.17.1On Mon, Nov 11, 2024 at 02:04:28PM +0800, Qiuxu Zhuo wrote:
> Fix typos in comments.
>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> Changes in v4:
> - No changes.
>
> Changes in v3:
> - Collect "Reviewed-by:" from Nikolay & Sohil.
> - Remove the detail typos from the commit message (Sohil).
>
> Changes in v2:
> - Collect "Reviewed-by:" from Tony.
>
> arch/x86/kernel/cpu/mce/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 0f0c6e9d9183..6e194ccffc7c 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
> } else {
> /*
> * Subject: Now start the scanning loop one by one in
> - * the original callin order.
> + * the original calling order.
I don't think this is a typo. It seems to refer to the mce_callin
variable/idea.
For example, each CPU "calls in" when ready. This is independent of when
each CPU is "called" to do something.
CPUs are called in this order 0, 1, 2.
CPUs "call in" in this order 1, 0, 2.
When a CPU is called can be different from when it responds.
Maybe I'm reading too much into this. :/
> * This way when there are any shared banks it will be
> * only seen by one CPU before cleared, avoiding duplicates.
> */
> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
> /* This should be disabled by the BIOS, but isn't always */
> if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> /*
> - * disable GART TBL walk error reporting, which
> + * disable GART TLB walk error reporting, which
This also is not a typo. TBL -> table
From old AMD K8 BKDG document:
10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0
Thanks,
Yazen
Hi Yazen,
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > @@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
> > } else {
> > /*
> > * Subject: Now start the scanning loop one by one in
> > - * the original callin order.
> > + * the original calling order.
>
> I don't think this is a typo. It seems to refer to the mce_callin variable/idea.
>
> For example, each CPU "calls in" when ready. This is independent of when
> each CPU is "called" to do something.
>
> CPUs are called in this order 0, 1, 2.
> CPUs "call in" in this order 1, 0, 2.
>
> When a CPU is called can be different from when it responds.
>
> Maybe I'm reading too much into this. :/
Too finicky to me :(
But I appreciate you sharing your reading and thoughts. 😊
-Qiuxu
On 11/12/2024 7:38 AM, Yazen Ghannam wrote:
>
>> * This way when there are any shared banks it will be
>> * only seen by one CPU before cleared, avoiding duplicates.
>> */
>> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
>> /* This should be disabled by the BIOS, but isn't always */
>> if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
>> /*
>> - * disable GART TBL walk error reporting, which
>> + * disable GART TLB walk error reporting, which
>
> This also is not a typo. TBL -> table
>
> From old AMD K8 BKDG document:
> 10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0
>
There is another comment in init_amd_gh() that seems to be related to
the same thing and similarly worded. That seems to refer to TLB instead
of TBL(table).
/*
* Disable GART TLB Walk Errors on Fam10h. We do this here because this
* is always needed when GART is enabled, even in a kernel which has no
* MCE support built in. BIOS should disable GartTlbWlk Errors already.
* If it doesn't, we do it here as suggested by the BKDG.
*
* Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=33012
*/
msr_set_bit(MSR_AMD64_MCx_MASK(4), 10);
On Tue, Nov 12, 2024 at 02:35:32PM -0800, Sohil Mehta wrote:
> On 11/12/2024 7:38 AM, Yazen Ghannam wrote:
> >
> >> * This way when there are any shared banks it will be
> >> * only seen by one CPU before cleared, avoiding duplicates.
> >> */
> >> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
> >> /* This should be disabled by the BIOS, but isn't always */
> >> if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> >> /*
> >> - * disable GART TBL walk error reporting, which
> >> + * disable GART TLB walk error reporting, which
> >
> > This also is not a typo. TBL -> table
> >
> > From old AMD K8 BKDG document:
> > 10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0
> >
>
> There is another comment in init_amd_gh() that seems to be related to
> the same thing and similarly worded. That seems to refer to TLB instead
> of TBL(table).
>
> /*
> * Disable GART TLB Walk Errors on Fam10h. We do this here because this
> * is always needed when GART is enabled, even in a kernel which has no
> * MCE support built in. BIOS should disable GartTlbWlk Errors already.
> * If it doesn't, we do it here as suggested by the BKDG.
> *
> * Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=33012
> */
> msr_set_bit(MSR_AMD64_MCx_MASK(4), 10);
>
>
Now I think *that* is a typo, since it doesn't match the documentation.
:)
Thanks,
Yazen
> From: Yazen Ghannam <yazen.ghannam@amd.com> > [...] > > >> - * disable GART TBL walk error reporting, which > > >> + * disable GART TLB walk error reporting, which > > > > > > This also is not a typo. TBL -> table > > > > > > From old AMD K8 BKDG document: > > > 10 GartTblWkEn GART Table Walk Error Reporting Enable R/W 0 > > > > > > > There is another comment in init_amd_gh() that seems to be related to > > the same thing and similarly worded. That seems to refer to TLB > > instead of TBL(table). > > > > /* > > * Disable GART TLB Walk Errors on Fam10h. We do this here because > > this > > * is always needed when GART is enabled, even in a kernel which has > > no > > * MCE support built in. BIOS should disable GartTlbWlk Errors already. > > * If it doesn't, we do it here as suggested by the BKDG. > > * > > * Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=33012 > > */ Thanks Sohil for this reference. > > msr_set_bit(MSR_AMD64_MCx_MASK(4), 10); > > > > > > Now I think *that* is a typo, since it doesn't match the documentation. > :) As Yazen has the documentation that says TBL (table) is the intended meaning. I'll drop this patch in next version. -Qiuxu
On 11/10/2024 10:04 PM, Qiuxu Zhuo wrote:
> Fix typos in comments.
>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
It might have been fine to merge this with patch 6/8. They both touch
the same file and the changes are mostly superficial. Probably consider
it if you do another revision but not needed otherwise.
> arch/x86/kernel/cpu/mce/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 0f0c6e9d9183..6e194ccffc7c 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1144,7 +1144,7 @@ static noinstr int mce_start(int *no_way_out)
> } else {
> /*
> * Subject: Now start the scanning loop one by one in
> - * the original callin order.
> + * the original calling order.
> * This way when there are any shared banks it will be
> * only seen by one CPU before cleared, avoiding duplicates.
> */
> @@ -1917,7 +1917,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
> /* This should be disabled by the BIOS, but isn't always */
> if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> /*
> - * disable GART TBL walk error reporting, which
> + * disable GART TLB walk error reporting, which
> * trips off incorrectly with the IOMMU & 3ware
> * & Cerberus:
> */
Using xchg() to atomically get and clear the MCE log buffer flags,
streamlines the code and reduces the text size by 20 bytes.
$ size dev-mcelog.o.*
text data bss dec hex filename
3013 360 160 3533 dcd dev-mcelog.o.old
2993 360 160 3513 db9 dev-mcelog.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/dev-mcelog.c b/arch/x86/kernel/cpu/mce/dev-mcelog.c
index af44fd5dbd7c..8d023239ce18 100644
--- a/arch/x86/kernel/cpu/mce/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mce/dev-mcelog.c
@@ -264,15 +264,8 @@ static long mce_chrdev_ioctl(struct file *f, unsigned int cmd,
return put_user(sizeof(struct mce), p);
case MCE_GET_LOG_LEN:
return put_user(mcelog->len, p);
- case MCE_GETCLEAR_FLAGS: {
- unsigned flags;
-
- do {
- flags = mcelog->flags;
- } while (cmpxchg(&mcelog->flags, flags, 0) != flags);
-
- return put_user(flags, p);
- }
+ case MCE_GETCLEAR_FLAGS:
+ return put_user(xchg(&mcelog->flags, 0), p);
default:
return -ENOTTY;
}
--
2.17.1The following commit has been merged into the ras/core branch of tip:
Commit-ID: 325c3376afad838eec8b9342e9e5eef270c5b184
Gitweb: https://git.kernel.org/tip/325c3376afad838eec8b9342e9e5eef270c5b184
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Fri, 25 Oct 2024 10:45:53 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 28 Oct 2024 14:07:47 +01:00
x86/mce/mcelog: Use xchg() to get and clear the flags
Using xchg() to atomically get and clear the MCE log buffer flags,
streamlines the code and reduces the text size by 20 bytes.
$ size dev-mcelog.o.*
text data bss dec hex filename
3013 360 160 3533 dcd dev-mcelog.o.old
2993 360 160 3513 db9 dev-mcelog.o.new
No functional changes intended.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Link: https://lore.kernel.org/r/20241025024602.24318-2-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/dev-mcelog.c b/arch/x86/kernel/cpu/mce/dev-mcelog.c
index af44fd5..8d02323 100644
--- a/arch/x86/kernel/cpu/mce/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mce/dev-mcelog.c
@@ -264,15 +264,8 @@ static long mce_chrdev_ioctl(struct file *f, unsigned int cmd,
return put_user(sizeof(struct mce), p);
case MCE_GET_LOG_LEN:
return put_user(mcelog->len, p);
- case MCE_GETCLEAR_FLAGS: {
- unsigned flags;
-
- do {
- flags = mcelog->flags;
- } while (cmpxchg(&mcelog->flags, flags, 0) != flags);
-
- return put_user(flags, p);
- }
+ case MCE_GETCLEAR_FLAGS:
+ return put_user(xchg(&mcelog->flags, 0), p);
default:
return -ENOTTY;
}Use the predefined MCG_BANKCNT_MASK macro instead of the hardcoded
0xff to mask the bank number bits.
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index f6103e6bf69a..b3cd2c61b11d 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -94,7 +94,7 @@ static int cmci_supported(int *banks)
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
return 0;
rdmsrl(MSR_IA32_MCG_CAP, cap);
- *banks = min_t(unsigned, MAX_NR_BANKS, cap & 0xff);
+ *banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
}
--
2.17.1The following commit has been merged into the ras/core branch of tip:
Commit-ID: 754269ccf03d68da15b9e5cdd26a6464b81cec67
Gitweb: https://git.kernel.org/tip/754269ccf03d68da15b9e5cdd26a6464b81cec67
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
AuthorDate: Fri, 25 Oct 2024 10:45:54 +08:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 28 Oct 2024 14:27:34 +01:00
x86/mce/intel: Use MCG_BANKCNT_MASK instead of 0xff
Use the predefined MCG_BANKCNT_MASK macro instead of the hardcoded
0xff to mask the bank number bits.
No functional changes intended.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Link: https://lore.kernel.org/r/20241025024602.24318-3-qiuxu.zhuo@intel.com
---
arch/x86/kernel/cpu/mce/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index f6103e6..b3cd2c6 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -94,7 +94,7 @@ static int cmci_supported(int *banks)
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
return 0;
rdmsrl(MSR_IA32_MCG_CAP, cap);
- *banks = min_t(unsigned, MAX_NR_BANKS, cap & 0xff);
+ *banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
}Make several functions that return 0 or 1 return a boolean value for
better readability. Additionally, mce_notify_irq() is about whether
notifying the user and is not IRQ relevant. Rename it to mce_notify_user()
to better reflect its purpose.
No functional changes are intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Rename mce_notify_irq() to mce_notify_user() (Sohil).
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/include/asm/mce.h | 4 ++--
arch/x86/kernel/cpu/mce/amd.c | 10 +++++-----
arch/x86/kernel/cpu/mce/core.c | 28 ++++++++++++++--------------
arch/x86/kernel/cpu/mce/inject.c | 2 +-
arch/x86/kernel/cpu/mce/intel.c | 9 +++++----
5 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3b9970117a0f..dd9effd7c8b5 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -244,7 +244,7 @@ static inline void cmci_rediscover(void) {}
static inline void cmci_recheck(void) {}
#endif
-int mce_available(struct cpuinfo_x86 *c);
+bool mce_available(struct cpuinfo_x86 *c);
bool mce_is_memory_error(struct mce *m);
bool mce_is_correctable(struct mce *m);
bool mce_usable_address(struct mce *m);
@@ -264,7 +264,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-int mce_notify_irq(void);
+bool mce_notify_user(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 14bf8c232e45..4dae9841ee38 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -381,7 +381,7 @@ static bool lvt_interrupt_supported(unsigned int bank, u32 msr_high_bits)
return msr_high_bits & BIT(28);
}
-static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
+static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
{
int msr = (hi & MASK_LVTOFF_HI) >> 20;
@@ -389,7 +389,7 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
if (apic != msr) {
@@ -399,15 +399,15 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
* was set is reserved. Return early here:
*/
if (mce_flags.smca)
- return 0;
+ return false;
pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n",
b->cpu, apic, b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
- return 1;
+ return true;
};
/* Reprogram MCx_MISC MSR behind this threshold bank. */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2a938f429c4d..57c05015f984 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -479,10 +479,10 @@ static noinstr void mce_gather_info(struct mce *m, struct pt_regs *regs)
}
}
-int mce_available(struct cpuinfo_x86 *c)
+bool mce_available(struct cpuinfo_x86 *c)
{
if (mca_cfg.disabled)
- return 0;
+ return false;
return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
}
@@ -584,7 +584,7 @@ static int mce_early_notifier(struct notifier_block *nb, unsigned long val,
set_bit(0, &mce_need_notify);
- mce_notify_irq();
+ mce_notify_user();
return NOTIFY_DONE;
}
@@ -1704,7 +1704,7 @@ static void mce_timer_fn(struct timer_list *t)
* Alert userspace if needed. If we logged an MCE, reduce the polling
* interval, otherwise increase the polling interval.
*/
- if (mce_notify_irq())
+ if (mce_notify_user())
iv = max(iv / 2, (unsigned long) HZ/100);
else
iv = min(iv * 2, round_jiffies_relative(check_interval * HZ));
@@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
* Can be called from interrupt context, but not from machine check/NMI
* context.
*/
-int mce_notify_irq(void)
+bool mce_notify_user(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1759,11 +1759,11 @@ int mce_notify_irq(void)
if (__ratelimit(&ratelimit))
pr_info(HW_ERR "Machine check events logged\n");
- return 1;
+ return true;
}
- return 0;
+ return false;
}
-EXPORT_SYMBOL_GPL(mce_notify_irq);
+EXPORT_SYMBOL_GPL(mce_notify_user);
static void __mcheck_cpu_mce_banks_init(void)
{
@@ -1985,25 +1985,25 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
return 0;
}
-static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
{
if (c->x86 != 5)
- return 0;
+ return false;
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
mce_flags.p5 = 1;
- return 1;
+ return true;
case X86_VENDOR_CENTAUR:
winchip_mcheck_init(c);
mce_flags.winchip = 1;
- return 1;
+ return true;
default:
- return 0;
+ return false;
}
- return 0;
+ return false;
}
/*
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 49ed3428785d..f5a7dae385c6 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -229,7 +229,7 @@ static int raise_local(void)
} else if (m->status) {
pr_info("Starting machine check poll CPU %d\n", cpu);
raise_poll(m);
- mce_notify_irq();
+ mce_notify_user();
pr_info("Machine check poll done on CPU %d\n", cpu);
} else
m->finished = 0;
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index b3cd2c61b11d..f863df0ff42c 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -75,12 +75,12 @@ static u16 cmci_threshold[MAX_NR_BANKS];
*/
#define CMCI_STORM_THRESHOLD 32749
-static int cmci_supported(int *banks)
+static bool cmci_supported(int *banks)
{
u64 cap;
if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
- return 0;
+ return false;
/*
* Vendor check is not strictly needed, but the initial
@@ -89,10 +89,11 @@ static int cmci_supported(int *banks)
*/
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
- return 0;
+ return false;
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
- return 0;
+ return false;
+
rdmsrl(MSR_IA32_MCG_CAP, cap);
*banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
--
2.17.1On Fri, Oct 25, 2024 at 10:45:55AM +0800, Qiuxu Zhuo wrote:
> @@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
> * Can be called from interrupt context, but not from machine check/NMI
> * context.
> */
> -int mce_notify_irq(void)
> +bool mce_notify_user(void)
So why are you not fixing the comment above it too then?
Have you audited all the code to make sure this function is not called from
IRQ context anymore?
Did you do git archeology to find out why it was called "_irq" at all in the
first place and why is it ok to change the name and adjust the comment above
it now?
In any case, this change needs to be a separate patch along with all the
explanations why is it ok to rename it.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris,
Thanks for your time reviewing the series.
> From: Borislav Petkov <bp@alien8.de>
> [...]
> On Fri, Oct 25, 2024 at 10:45:55AM +0800, Qiuxu Zhuo wrote:
> > @@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
> > * Can be called from interrupt context, but not from machine check/NMI
> > * context.
> > */
> > -int mce_notify_irq(void)
> > +bool mce_notify_user(void)
>
> So why are you not fixing the comment above it too then?
>
> Have you audited all the code to make sure this function is not called from
> IRQ context anymore?
Currently this function is called from either interrupt context (timer) or
process context (notifier chain).
> Did you do git archeology to find out why it was called "_irq" at all in the first
Thanks for reminding me this.
Commit
9ff36ee9668f ("x86, mce: rename mce_notify_user to mce_notify_irq")
renamed mce_notify_user() to mce_notify_irq() to indicate that this function
should only be called from interrupt context and not used in machine check
or NMI context.
> place and why is it ok to change the name and adjust the comment above it
> now?
At first glance, the function name mce_notify_irq() it looks like "MCE notifies IRQ ...",
which is confusing and doesn't clearly reflect what it does. After the "git archeology"
above and offline communication from Andi (the commit author), it was clarified
that the suffix '_irq' was only used to indicate that the function can only be used in
interrupt context.
But I think the comments above the function clearly indicates which types of context
it can be used in, so it doesn't need the suffix '_irq' in the function name. Renaming it
back to mce_notify_user() can better reflect its function of notifying the user(s) about
the new machine check events.
> In any case, this change needs to be a separate patch along with all the
> explanations why is it ok to rename it.
OK.
I'll sperate this patch in the next version.
How about the following diff?
From f33f7340a40d3db1f4acf3c9ded574d1b9801fa7 Mon Sep 17 00:00:00 2001
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Date: Tue, 29 Oct 2024 09:07:47 +0800
Subject: [PATCH 1/1] x86/mce: Rename mce_notify_irq() back to
mce_notify_user()
Commit
9ff36ee9668f ("x86, mce: rename mce_notify_user to mce_notify_irq")
renamed mce_notify_user() to mce_notify_irq() to indicate that this
function should only be called from interrupt context and not used in
machine check or NMI context. However, the function name mce_notify_irq()
is confusing and doesn't clearly reflect what it does.
The comments above the function clearly indicates which types of context
it can be used in, so it doesn't need the suffix '_irq' in the function
name. Rename it back to mce_notify_user() to better reflect its function
of notifying the user(s) about the new machine check events and adjust the
comment to indicate that it can also be called from process context.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/include/asm/mce.h | 2 +-
arch/x86/kernel/cpu/mce/core.c | 10 +++++-----
arch/x86/kernel/cpu/mce/inject.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 7a01bb5b19d3..dd9effd7c8b5 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -264,7 +264,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-bool mce_notify_irq(void);
+bool mce_notify_user(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 725c1d6fb1e5..5f42ab2ea944 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -584,7 +584,7 @@ static int mce_early_notifier(struct notifier_block *nb, unsigned long val,
set_bit(0, &mce_need_notify);
- mce_notify_irq();
+ mce_notify_user();
return NOTIFY_DONE;
}
@@ -1704,7 +1704,7 @@ static void mce_timer_fn(struct timer_list *t)
* Alert userspace if needed. If we logged an MCE, reduce the polling
* interval, otherwise increase the polling interval.
*/
- if (mce_notify_irq())
+ if (mce_notify_user())
iv = max(iv / 2, (unsigned long) HZ/100);
else
iv = min(iv * 2, round_jiffies_relative(check_interval * HZ));
@@ -1745,10 +1745,10 @@ static void mce_timer_delete_all(void)
/*
* Notify the user(s) about new machine check events.
- * Can be called from interrupt context, but not from machine check/NMI
+ * Can be called from process/interrupt context, but not from machine check/NMI
* context.
*/
-bool mce_notify_irq(void)
+bool mce_notify_user(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1763,7 +1763,7 @@ bool mce_notify_irq(void)
}
return false;
}
-EXPORT_SYMBOL_GPL(mce_notify_irq);
+EXPORT_SYMBOL_GPL(mce_notify_user);
static void __mcheck_cpu_mce_banks_init(void)
{
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 49ed3428785d..f5a7dae385c6 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -229,7 +229,7 @@ static int raise_local(void)
} else if (m->status) {
pr_info("Starting machine check poll CPU %d\n", cpu);
raise_poll(m);
- mce_notify_irq();
+ mce_notify_user();
pr_info("Machine check poll done on CPU %d\n", cpu);
} else
m->finished = 0;
--
2.17.1
On Tue, Oct 29, 2024 at 03:32:00AM +0000, Zhuo, Qiuxu wrote:
> At first glance, the function name mce_notify_irq() it looks like "MCE
> notifies IRQ ...", which is confusing and doesn't clearly reflect what it
> does.
Maybe to you but the name means exactly that - it is run in irq context.
> But I think the comments above the function clearly indicates which types of
> context it can be used in, so it doesn't need the suffix '_irq' in the
> function name. Renaming it back to mce_notify_user() can better reflect its
> function of notifying the user(s) about the new machine check events.
Who else would you be notifying except the users?!
> renamed mce_notify_user() to mce_notify_irq() to indicate that this function
> should only be called from interrupt context and not used in machine check
> or NMI context. However, the function name mce_notify_irq() is confusing and
> doesn't clearly reflect what it does.
Maybe it confuses you only.
Considering how there's not a notify-in-NMI/MCE counterpart, I guess this
function could be renamed to "mce_notify" simply.
Or not do anything at all. It has been that way for over a decade and hasn't
bothered anyone. Let's not get overeager.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris, > From: Borislav Petkov <bp@alien8.de> > [...] > Or not do anything at all. It has been that way for over a decade and hasn't > bothered anyone. Let's not get overeager. Thanks for letting me know your thoughts. OK. I'll drop this part in the next version. -Qiuxu
The 'storm' variable points to this_cpu_ptr(&storm_desc). Access the
'stormy_bank_count' field through the 'storm' to avoid calling
this_cpu_*() on the same per-CPU variable twice.
This minor optimization reduces the text size by 16 bytes.
$ size threshold.o.*
text data bss dec hex filename
1395 1664 0 3059 bf3 threshold.o.old
1379 1664 0 3043 be3 threshold.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index 89e31e1e5c9c..f4a007616468 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -90,7 +90,7 @@ void cmci_storm_end(unsigned int bank)
storm->banks[bank].in_storm_mode = false;
/* If no banks left in storm mode, stop polling. */
- if (!this_cpu_dec_return(storm_desc.stormy_bank_count))
+ if (!--storm->stormy_bank_count)
mce_timer_kick(false);
}
--
2.17.1Make mce_gen_pool_create() return explicit error codes for better
readability.
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Move the 'int ret' along with the other int variables (Sohil).
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/genpool.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
index 4284749ec803..64dffb50335a 100644
--- a/arch/x86/kernel/cpu/mce/genpool.c
+++ b/arch/x86/kernel/cpu/mce/genpool.c
@@ -118,22 +118,21 @@ int mce_gen_pool_add(struct mce *mce)
static int mce_gen_pool_create(void)
{
- int mce_numrecords, mce_poolsz, order;
+ int mce_numrecords, mce_poolsz, order, ret;
struct gen_pool *gpool;
- int ret = -ENOMEM;
void *mce_pool;
order = order_base_2(sizeof(struct mce_evt_llist));
gpool = gen_pool_create(order, -1);
if (!gpool)
- return ret;
+ return -ENOMEM;
mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
mce_poolsz = mce_numrecords * (1 << order);
mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
if (!mce_pool) {
gen_pool_destroy(gpool);
- return ret;
+ return -ENOMEM;
}
ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
if (ret) {
@@ -144,7 +143,7 @@ static int mce_gen_pool_create(void)
mce_evt_pool = gpool;
- return ret;
+ return 0;
}
int mce_gen_pool_init(void)
--
2.17.1On Fri, Oct 25, 2024 at 10:45:57AM +0800, Qiuxu Zhuo wrote:
> mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
> mce_poolsz = mce_numrecords * (1 << order);
> mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
> if (!mce_pool) {
> gen_pool_destroy(gpool);
> - return ret;
> + return -ENOMEM;
This patch is just silly: the function is not that huge not to be able to see
at a quick glance that it is -ENOMEM that is being returned in all error cases
...
> }
> ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
> if (ret) {
... except in this case where, oh well, actually, it is -ENOMEM again but you
have to go down the bowells of genalloc to see it.
All in all, this is causing more churn than actually improving something...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: Tony Luck <tony.luck@intel.com>
Split each vendor specific part into its own helper function.
Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Newly added.
arch/x86/kernel/cpu/mce/core.c | 194 ++++++++++++++++++---------------
1 file changed, 106 insertions(+), 88 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 57c05015f984..bb8b1000fa0a 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1880,101 +1880,119 @@ static void __mcheck_cpu_check_banks(void)
}
}
+static void apply_quirks_amd(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+ struct mca_config *cfg = &mca_cfg;
+
+ /* This should be disabled by the BIOS, but isn't always */
+ if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
+ /*
+ * disable GART TBL walk error reporting, which
+ * trips off incorrectly with the IOMMU & 3ware
+ * & Cerberus:
+ */
+ clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
+ }
+ if (c->x86 < 0x11 && cfg->bootlog < 0) {
+ /*
+ * Lots of broken BIOS around that don't clear them
+ * by default and leave crap in there. Don't log:
+ */
+ cfg->bootlog = 0;
+ }
+ /*
+ * Various K7s with broken bank 0 around. Always disable
+ * by default.
+ */
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].ctl = 0;
+
+ /*
+ * overflow_recov is supported for F15h Models 00h-0fh
+ * even though we don't have a CPUID bit for it.
+ */
+ if (c->x86 == 0x15 && c->x86_model <= 0xf)
+ mce_flags.overflow_recov = 1;
+
+ if (c->x86 >= 0x17 && c->x86 <= 0x1A)
+ mce_flags.zen_ifu_quirk = 1;
+}
+
+static void apply_quirks_intel(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+ struct mca_config *cfg = &mca_cfg;
+
+ /*
+ * SDM documents that on family 6 bank 0 should not be written
+ * because it aliases to another special BIOS controlled
+ * register.
+ * But it's not aliased anymore on model 0x1a+
+ * Don't ignore bank 0 completely because there could be a
+ * valid event later, merely don't write CTL0.
+ */
+ if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].init = false;
+
+ /*
+ * All newer Intel systems support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
+ cfg->monarch_timeout < 0)
+ cfg->monarch_timeout = USEC_PER_SEC;
+
+ /*
+ * There are also broken BIOSes on some Pentium M and
+ * earlier systems:
+ */
+ if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
+ cfg->bootlog = 0;
+
+ if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
+ mce_flags.snb_ifu_quirk = 1;
+
+ /*
+ * Skylake, Cascacde Lake and Cooper Lake require a quirk on
+ * rep movs.
+ */
+ if (c->x86_vfm == INTEL_SKYLAKE_X)
+ mce_flags.skx_repmov_quirk = 1;
+}
+
+static void apply_quirks_zhaoxin(struct cpuinfo_x86 *c)
+{
+ struct mca_config *cfg = &mca_cfg;
+
+ /*
+ * All newer Zhaoxin CPUs support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
+ if (cfg->monarch_timeout < 0)
+ cfg->monarch_timeout = USEC_PER_SEC;
+ }
+}
+
/* Add per CPU specific workarounds here */
static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
{
- struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
- if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
+ switch (c->x86_vendor) {
+ case X86_VENDOR_UNKNOWN:
pr_info("unknown CPU type - not enabling MCE support\n");
return -EOPNOTSUPP;
- }
-
- /* This should be disabled by the BIOS, but isn't always */
- if (c->x86_vendor == X86_VENDOR_AMD) {
- if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
- /*
- * disable GART TBL walk error reporting, which
- * trips off incorrectly with the IOMMU & 3ware
- * & Cerberus:
- */
- clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
- }
- if (c->x86 < 0x11 && cfg->bootlog < 0) {
- /*
- * Lots of broken BIOS around that don't clear them
- * by default and leave crap in there. Don't log:
- */
- cfg->bootlog = 0;
- }
- /*
- * Various K7s with broken bank 0 around. Always disable
- * by default.
- */
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].ctl = 0;
-
- /*
- * overflow_recov is supported for F15h Models 00h-0fh
- * even though we don't have a CPUID bit for it.
- */
- if (c->x86 == 0x15 && c->x86_model <= 0xf)
- mce_flags.overflow_recov = 1;
-
- if (c->x86 >= 0x17 && c->x86 <= 0x1A)
- mce_flags.zen_ifu_quirk = 1;
-
- }
-
- if (c->x86_vendor == X86_VENDOR_INTEL) {
- /*
- * SDM documents that on family 6 bank 0 should not be written
- * because it aliases to another special BIOS controlled
- * register.
- * But it's not aliased anymore on model 0x1a+
- * Don't ignore bank 0 completely because there could be a
- * valid event later, merely don't write CTL0.
- */
-
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].init = false;
-
- /*
- * All newer Intel systems support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
-
- /*
- * There are also broken BIOSes on some Pentium M and
- * earlier systems:
- */
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
- cfg->bootlog = 0;
-
- if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
- mce_flags.snb_ifu_quirk = 1;
-
- /*
- * Skylake, Cascacde Lake and Cooper Lake require a quirk on
- * rep movs.
- */
- if (c->x86_vfm == INTEL_SKYLAKE_X)
- mce_flags.skx_repmov_quirk = 1;
- }
-
- if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
- /*
- * All newer Zhaoxin CPUs support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
- if (cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
- }
+ case X86_VENDOR_AMD:
+ apply_quirks_amd(c);
+ break;
+ case X86_VENDOR_INTEL:
+ apply_quirks_intel(c);
+ break;
+ case X86_VENDOR_ZHAOXIN:
+ apply_quirks_zhaoxin(c);
+ break;
}
if (cfg->monarch_timeout < 0)
--
2.17.1On Fri, Oct 25, 2024 at 10:45:58AM +0800, Qiuxu Zhuo wrote:
> From: Tony Luck <tony.luck@intel.com>
>
> Split each vendor specific part into its own helper function.
>
> Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> Changes in v3:
> - Newly added.
>
> arch/x86/kernel/cpu/mce/core.c | 194 ++++++++++++++++++---------------
> 1 file changed, 106 insertions(+), 88 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 57c05015f984..bb8b1000fa0a 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1880,101 +1880,119 @@ static void __mcheck_cpu_check_banks(void)
> }
> }
>
> +static void apply_quirks_amd(struct cpuinfo_x86 *c)
> +{
> + struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> + struct mca_config *cfg = &mca_cfg;
> +
> + /* This should be disabled by the BIOS, but isn't always */
> + if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> + /*
> + * disable GART TBL walk error reporting, which
> + * trips off incorrectly with the IOMMU & 3ware
> + * & Cerberus:
> + */
> + clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
> + }
Newline here please.
> + if (c->x86 < 0x11 && cfg->bootlog < 0) {
> + /*
> + * Lots of broken BIOS around that don't clear them
> + * by default and leave crap in there. Don't log:
> + */
> + cfg->bootlog = 0;
> + }
And here.
> + /*
> + * Various K7s with broken bank 0 around. Always disable
> + * by default.
> + */
> + if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
> + mce_banks[0].ctl = 0;
> +
> + /*
> + * overflow_recov is supported for F15h Models 00h-0fh
> + * even though we don't have a CPUID bit for it.
> + */
> + if (c->x86 == 0x15 && c->x86_model <= 0xf)
> + mce_flags.overflow_recov = 1;
> +
> + if (c->x86 >= 0x17 && c->x86 <= 0x1A)
> + mce_flags.zen_ifu_quirk = 1;
> +}
> +
> +static void apply_quirks_intel(struct cpuinfo_x86 *c)
> +{
> + struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> + struct mca_config *cfg = &mca_cfg;
Is there a benefit to this pointer? We use mca_cfg.FIELD in most other
places.
Thanks,
Yazen
Hi Yazen,
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > +static void apply_quirks_amd(struct cpuinfo_x86 *c) {
> > + struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> > + struct mca_config *cfg = &mca_cfg;
> > +
> > + /* This should be disabled by the BIOS, but isn't always */
> > + if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> > + /*
> > + * disable GART TBL walk error reporting, which
> > + * trips off incorrectly with the IOMMU & 3ware
> > + * & Cerberus:
> > + */
> > + clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
> > + }
>
> Newline here please.
OK.
Will update it in next version.
> > + if (c->x86 < 0x11 && cfg->bootlog < 0) {
> > + /*
> > + * Lots of broken BIOS around that don't clear them
> > + * by default and leave crap in there. Don't log:
> > + */
> > + cfg->bootlog = 0;
> > + }
>
> And here.
And will update it in next version.
> [...]
> > +static void apply_quirks_intel(struct cpuinfo_x86 *c) {
> > + struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> > + struct mca_config *cfg = &mca_cfg;
>
> Is there a benefit to this pointer? We use mca_cfg.FIELD in most other places.
This could make the diff smaller for easier review, and I also believe that fewer direct
uses of global variables in functions are better. Additionally, there are multiple uses of
'mca_cfg' in the function, the local variable 'cfg' is shorter and more convenient to use.
[ Certainly, if the global variable 'mca_cfg' is only used once in the function, directly
using it might be more convenient. ]
Just from my perspective, no strong preference. 😊
-Qiuxu
On Wed, Oct 30, 2024 at 01:39:43AM +0000, Zhuo, Qiuxu wrote:
[...]
Thanks Qiuxu.
>
> > > +static void apply_quirks_intel(struct cpuinfo_x86 *c) {
> > > + struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> > > + struct mca_config *cfg = &mca_cfg;
> >
> > Is there a benefit to this pointer? We use mca_cfg.FIELD in most other places.
>
> This could make the diff smaller for easier review, and I also believe that fewer direct
> uses of global variables in functions are better. Additionally, there are multiple uses of
> 'mca_cfg' in the function, the local variable 'cfg' is shorter and more convenient to use.
>
I don't think it would make the diff smaller here since the code is
already being moved.
Though you could say this is a separate logical change compared to just
moving the code as-is.
Also, I don't think the "shorter, more convenient" idea holds. It's not
that much shorter. And there are already cases of using the global
variables "mca_cfg" and "mce_flags".
Why is "...fewer direct uses of global variables in functions..." better?
> [ Certainly, if the global variable 'mca_cfg' is only used once in the function, directly
> using it might be more convenient. ]
>
There is one such case in your patch.
> Just from my perspective, no strong preference. 😊
>
Same here. I just figured this suggestion would be another possible
cleanup. :)
Thanks,
Yazen
Hi Yazen,
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > Just from my perspective, no strong preference. 😊
>
> Same here. I just figured this suggestion would be another possible
> cleanup. :)
Thanks for your suggestion. Yes, it does save 3 lines of code.
Either the current patch or your suggestion is OK with me.
Hi @Boris,
may I know which option is OK with you:
Option A (current patch):
struct mca_config *cfg = &mca_cfg;
and then use 'cfg' in apply_quirks_{amd, intel, zhaoxin}()
Option B (suggested by Yazen):
Directly use 'mca_cfg' in apply_quirks_{amd, intel, zhaoxin}()
Thanks!
-Qiuxu
Convert family/model mixed checks to VFM-based checks to make
the code more compact.
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Newly added.
arch/x86/kernel/cpu/mce/core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index bb8b1000fa0a..936804a5a0b9 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
+ /* Older CPUs (prior to family 6) don't need quirks. */
+ if (c->x86_vfm < INTEL_PENTIUM_PRO)
+ return;
+
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1932,22 +1936,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
* Don't ignore bank 0 completely because there could be a
* valid event later, merely don't write CTL0.
*/
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
mce_banks[0].init = false;
/*
* All newer Intel systems support MCE broadcasting. Enable
* synchronization with a one second timeout.
*/
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
+ if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0)
cfg->monarch_timeout = USEC_PER_SEC;
/*
* There are also broken BIOSes on some Pentium M and
* earlier systems:
*/
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0)
cfg->bootlog = 0;
if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
--
2.17.1Remove the unnecessary {} from the case statement.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 936804a5a0b9..f57a68b53f29 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2090,10 +2090,9 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
mce_intel_feature_init(c);
break;
- case X86_VENDOR_AMD: {
+ case X86_VENDOR_AMD:
mce_amd_feature_init(c);
break;
- }
case X86_VENDOR_HYGON:
mce_hygon_feature_init(c);
--
2.17.1Remove unnecessary NULL pointer initializations from variables that
are already initialized before use.
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" Nikolay & Sohil.
- Remove the variables' names from the commit message (Sohil).
arch/x86/kernel/cpu/mce/amd.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 4dae9841ee38..aecea842dac2 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -917,8 +917,8 @@ static void log_and_reset_block(struct threshold_block *block)
*/
static void amd_threshold_interrupt(void)
{
- struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
struct threshold_bank **bp = this_cpu_read(threshold_banks);
+ struct threshold_block *first_block, *block, *tmp;
unsigned int bank, cpu = smp_processor_id();
/*
@@ -1197,8 +1197,7 @@ static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb
static int __threshold_add_blocks(struct threshold_bank *b)
{
struct list_head *head = &b->blocks->miscj;
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
int err = 0;
err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name);
@@ -1308,8 +1307,7 @@ static void deallocate_threshold_blocks(struct threshold_bank *bank)
static void __threshold_remove_blocks(struct threshold_bank *b)
{
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
kobject_put(b->kobj);
--
2.17.1Fix typos in comments.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v3:
- Collect "Reviewed-by:" from Nikolay & Sohil.
- Remove the detail typos from the commit message (Sohil).
Changes in v2:
- Collect "Reviewed-by:" from Tony.
arch/x86/kernel/cpu/mce/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index f57a68b53f29..4c4558ed4736 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1120,7 +1120,7 @@ static noinstr int mce_start(int *no_way_out)
} else {
/*
* Subject: Now start the scanning loop one by one in
- * the original callin order.
+ * the original calling order.
* This way when there are any shared banks it will be
* only seen by one CPU before cleared, avoiding duplicates.
*/
@@ -1888,7 +1888,7 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
/* This should be disabled by the BIOS, but isn't always */
if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
/*
- * disable GART TBL walk error reporting, which
+ * disable GART TLB walk error reporting, which
* trips off incorrectly with the IOMMU & 3ware
* & Cerberus:
*/
--
2.17.1Using xchg() to atomically get and clear the MCE log buffer flags,
streamlines the code and reduces the text size by 20 bytes.
$ size dev-mcelog.o.*
text data bss dec hex filename
3013 360 160 3533 dcd dev-mcelog.o.old
2993 360 160 3513 db9 dev-mcelog.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/dev-mcelog.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/dev-mcelog.c b/arch/x86/kernel/cpu/mce/dev-mcelog.c
index af44fd5dbd7c..8d023239ce18 100644
--- a/arch/x86/kernel/cpu/mce/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mce/dev-mcelog.c
@@ -264,15 +264,8 @@ static long mce_chrdev_ioctl(struct file *f, unsigned int cmd,
return put_user(sizeof(struct mce), p);
case MCE_GET_LOG_LEN:
return put_user(mcelog->len, p);
- case MCE_GETCLEAR_FLAGS: {
- unsigned flags;
-
- do {
- flags = mcelog->flags;
- } while (cmpxchg(&mcelog->flags, flags, 0) != flags);
-
- return put_user(flags, p);
- }
+ case MCE_GETCLEAR_FLAGS:
+ return put_user(xchg(&mcelog->flags, 0), p);
default:
return -ENOTTY;
}
--
2.17.1Use the predefined MCG_BANKCNT_MASK macro instead of the hardcoded
0xff to mask the bank number bits.
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index f6103e6bf69a..b3cd2c61b11d 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -94,7 +94,7 @@ static int cmci_supported(int *banks)
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
return 0;
rdmsrl(MSR_IA32_MCG_CAP, cap);
- *banks = min_t(unsigned, MAX_NR_BANKS, cap & 0xff);
+ *banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
}
--
2.17.1Make several functions that return 0 or 1 return a boolean value for
better readability.
No functional changes are intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/include/asm/mce.h | 4 ++--
arch/x86/kernel/cpu/mce/amd.c | 10 +++++-----
arch/x86/kernel/cpu/mce/core.c | 22 +++++++++++-----------
arch/x86/kernel/cpu/mce/intel.c | 9 +++++----
4 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3b9970117a0f..7a01bb5b19d3 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -244,7 +244,7 @@ static inline void cmci_rediscover(void) {}
static inline void cmci_recheck(void) {}
#endif
-int mce_available(struct cpuinfo_x86 *c);
+bool mce_available(struct cpuinfo_x86 *c);
bool mce_is_memory_error(struct mce *m);
bool mce_is_correctable(struct mce *m);
bool mce_usable_address(struct mce *m);
@@ -264,7 +264,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-int mce_notify_irq(void);
+bool mce_notify_irq(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 14bf8c232e45..4dae9841ee38 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -381,7 +381,7 @@ static bool lvt_interrupt_supported(unsigned int bank, u32 msr_high_bits)
return msr_high_bits & BIT(28);
}
-static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
+static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
{
int msr = (hi & MASK_LVTOFF_HI) >> 20;
@@ -389,7 +389,7 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
if (apic != msr) {
@@ -399,15 +399,15 @@ static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
* was set is reserved. Return early here:
*/
if (mce_flags.smca)
- return 0;
+ return false;
pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
"for bank %d, block %d (MSR%08X=0x%x%08x)\n",
b->cpu, apic, b->bank, b->block, b->address, hi, lo);
- return 0;
+ return false;
}
- return 1;
+ return true;
};
/* Reprogram MCx_MISC MSR behind this threshold bank. */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2a938f429c4d..725c1d6fb1e5 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -479,10 +479,10 @@ static noinstr void mce_gather_info(struct mce *m, struct pt_regs *regs)
}
}
-int mce_available(struct cpuinfo_x86 *c)
+bool mce_available(struct cpuinfo_x86 *c)
{
if (mca_cfg.disabled)
- return 0;
+ return false;
return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
}
@@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
* Can be called from interrupt context, but not from machine check/NMI
* context.
*/
-int mce_notify_irq(void)
+bool mce_notify_irq(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1759,9 +1759,9 @@ int mce_notify_irq(void)
if (__ratelimit(&ratelimit))
pr_info(HW_ERR "Machine check events logged\n");
- return 1;
+ return true;
}
- return 0;
+ return false;
}
EXPORT_SYMBOL_GPL(mce_notify_irq);
@@ -1985,25 +1985,25 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
return 0;
}
-static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
+static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
{
if (c->x86 != 5)
- return 0;
+ return false;
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
mce_flags.p5 = 1;
- return 1;
+ return true;
case X86_VENDOR_CENTAUR:
winchip_mcheck_init(c);
mce_flags.winchip = 1;
- return 1;
+ return true;
default:
- return 0;
+ return false;
}
- return 0;
+ return false;
}
/*
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index b3cd2c61b11d..f863df0ff42c 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -75,12 +75,12 @@ static u16 cmci_threshold[MAX_NR_BANKS];
*/
#define CMCI_STORM_THRESHOLD 32749
-static int cmci_supported(int *banks)
+static bool cmci_supported(int *banks)
{
u64 cap;
if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
- return 0;
+ return false;
/*
* Vendor check is not strictly needed, but the initial
@@ -89,10 +89,11 @@ static int cmci_supported(int *banks)
*/
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
- return 0;
+ return false;
if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
- return 0;
+ return false;
+
rdmsrl(MSR_IA32_MCG_CAP, cap);
*banks = min_t(unsigned, MAX_NR_BANKS, cap & MCG_BANKCNT_MASK);
return !!(cap & MCG_CMCI_P);
--
2.17.1On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> @@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
> * Can be called from interrupt context, but not from machine check/NMI
> * context.
> */
> -int mce_notify_irq(void)
> +bool mce_notify_irq(void)
> {
> /* Not more than two messages every minute */
> static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
> @@ -1759,9 +1759,9 @@ int mce_notify_irq(void)
> if (__ratelimit(&ratelimit))
> pr_info(HW_ERR "Machine check events logged\n");
>
> - return 1;
> + return true;
> }
> - return 0;
> + return false;
> }
> EXPORT_SYMBOL_GPL(mce_notify_irq);
>
I am slightly confused by the function name mce_notify_irq() and the
boolean meaning. Would it better to rename this function to
mce_notify_user()? As the comment suggests on top, it purpose doesn't
seem to be irq related but to mainly notify the user.
Also, the boolean would probably make more sense then:
True -> Notified the user
False -> Did not notify the user
> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> > @@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
> > * Can be called from interrupt context, but not from machine check/NMI
> > * context.
> > */
> > -int mce_notify_irq(void)
> > +bool mce_notify_irq(void)
> > {
> > /* Not more than two messages every minute */
> > static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2); @@ -1759,9
> > +1759,9 @@ int mce_notify_irq(void)
> > if (__ratelimit(&ratelimit))
> > pr_info(HW_ERR "Machine check events logged\n");
> >
> > - return 1;
> > + return true;
> > }
> > - return 0;
> > + return false;
> > }
> > EXPORT_SYMBOL_GPL(mce_notify_irq);
> >
>
> I am slightly confused by the function name mce_notify_irq() and the boolean
> meaning. Would it better to rename this function to mce_notify_user()? As
> the comment suggests on top, it purpose doesn't seem to be irq related but
> to mainly notify the user.
> Also, the boolean would probably make more sense then:
> True -> Notified the user
> False -> Did not notify the user
Agree. mce_notify_user() better reflects what it does.
If nobody else objects to it, I'll update the function name in the next version.
Thanks!
-Qiuxu
The 'storm' variable points to this_cpu_ptr(&storm_desc). Access the
'stormy_bank_count' field through the 'storm' to avoid calling
this_cpu_*() on the same per-CPU variable twice.
This minor optimization reduces the text size by 16 bytes.
$ size threshold.o.*
text data bss dec hex filename
1395 1664 0 3059 bf3 threshold.o.old
1379 1664 0 3043 be3 threshold.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/threshold.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index 89e31e1e5c9c..f4a007616468 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -90,7 +90,7 @@ void cmci_storm_end(unsigned int bank)
storm->banks[bank].in_storm_mode = false;
/* If no banks left in storm mode, stop polling. */
- if (!this_cpu_dec_return(storm_desc.stormy_bank_count))
+ if (!--storm->stormy_bank_count)
mce_timer_kick(false);
}
--
2.17.1Make mce_gen_pool_create() return explicit error codes for better
readability.
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/genpool.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
index 4284749ec803..ffa28769dea6 100644
--- a/arch/x86/kernel/cpu/mce/genpool.c
+++ b/arch/x86/kernel/cpu/mce/genpool.c
@@ -120,20 +120,20 @@ static int mce_gen_pool_create(void)
{
int mce_numrecords, mce_poolsz, order;
struct gen_pool *gpool;
- int ret = -ENOMEM;
void *mce_pool;
+ int ret;
order = order_base_2(sizeof(struct mce_evt_llist));
gpool = gen_pool_create(order, -1);
if (!gpool)
- return ret;
+ return -ENOMEM;
mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
mce_poolsz = mce_numrecords * (1 << order);
mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
if (!mce_pool) {
gen_pool_destroy(gpool);
- return ret;
+ return -ENOMEM;
}
ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
if (ret) {
@@ -144,7 +144,7 @@ static int mce_gen_pool_create(void)
mce_evt_pool = gpool;
- return ret;
+ return 0;
}
int mce_gen_pool_init(void)
--
2.17.1
> diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
> index 4284749ec803..ffa28769dea6 100644
> --- a/arch/x86/kernel/cpu/mce/genpool.c
> +++ b/arch/x86/kernel/cpu/mce/genpool.c
> @@ -120,20 +120,20 @@ static int mce_gen_pool_create(void)
> {
> int mce_numrecords, mce_poolsz, order;
> struct gen_pool *gpool;
> - int ret = -ENOMEM;
> void *mce_pool;
> + int ret;
>
Nit: Maybe move the uninitialized ret along with the other ints in the
first line?
I had suggested something very similar but Boris felt that the current
code as-is reads better. But that was in a slightly different context.
https://lore.kernel.org/lkml/20240307170901.GBZen0re6AvpscLaTM@fat_crate.local/
> order = order_base_2(sizeof(struct mce_evt_llist));
> gpool = gen_pool_create(order, -1);
> if (!gpool)
> - return ret;
> + return -ENOMEM;
>
> mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
> mce_poolsz = mce_numrecords * (1 << order);
> mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
> if (!mce_pool) {
> gen_pool_destroy(gpool);
> - return ret;
> + return -ENOMEM;
> }
> ret = gen_pool_add(gpool, (unsigned long)mce_pool, mce_poolsz, -1);
> if (ret) {
> @@ -144,7 +144,7 @@ static int mce_gen_pool_create(void)
>
> mce_evt_pool = gpool;
>
> - return ret;
> + return 0;
> }
>
> int mce_gen_pool_init(void)
> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> > diff --git a/arch/x86/kernel/cpu/mce/genpool.c
> > b/arch/x86/kernel/cpu/mce/genpool.c
> > index 4284749ec803..ffa28769dea6 100644
> > --- a/arch/x86/kernel/cpu/mce/genpool.c
> > +++ b/arch/x86/kernel/cpu/mce/genpool.c
> > @@ -120,20 +120,20 @@ static int mce_gen_pool_create(void) {
> > int mce_numrecords, mce_poolsz, order;
> > struct gen_pool *gpool;
> > - int ret = -ENOMEM;
> > void *mce_pool;
> > + int ret;
> >
>
> Nit: Maybe move the uninitialized ret along with the other ints in the first
> line?
OK, that will save one line of code.
I'll update this in the next version.
> I had suggested something very similar but Boris felt that the current code as-
> is reads better. But that was in a slightly different context.
> https://lore.kernel.org/lkml/20240307170901.GBZen0re6AvpscLaTM@fat_crat
> e.local/
Thanks for sharing this ... 😊
-Qiuxu
Convert the multiple if() statements used for vendor differentiation
into a switch() statement for better readability.
As a bonus, the size of new generated text is reduced by 16 bytes.
$ size core.o.*
text data bss dec hex filename
21364 4181 3776 29321 7289 core.o.old
21348 4181 3776 29305 7279 core.o.new
No functional changes intended.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/core.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 725c1d6fb1e5..40672fe0991a 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1892,7 +1892,8 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
}
/* This should be disabled by the BIOS, but isn't always */
- if (c->x86_vendor == X86_VENDOR_AMD) {
+ switch (c->x86_vendor) {
+ case X86_VENDOR_AMD:
if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
/*
* disable GART TBL walk error reporting, which
@@ -1925,9 +1926,9 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
if (c->x86 >= 0x17 && c->x86 <= 0x1A)
mce_flags.zen_ifu_quirk = 1;
- }
+ break;
- if (c->x86_vendor == X86_VENDOR_INTEL) {
+ case X86_VENDOR_INTEL:
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1964,9 +1965,10 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
*/
if (c->x86_vfm == INTEL_SKYLAKE_X)
mce_flags.skx_repmov_quirk = 1;
- }
- if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
+ break;
+
+ case X86_VENDOR_ZHAOXIN:
/*
* All newer Zhaoxin CPUs support MCE broadcasting. Enable
* synchronization with a one second timeout.
@@ -1975,6 +1977,8 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
if (cfg->monarch_timeout < 0)
cfg->monarch_timeout = USEC_PER_SEC;
}
+
+ break;
}
if (cfg->monarch_timeout < 0)
--
2.17.1> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 725c1d6fb1e5..40672fe0991a 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1892,7 +1892,8 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> }
>
> /* This should be disabled by the BIOS, but isn't always */
This comment is specific to the AMD and placing it before the switch
makes it seem generic to the entire switch statement. It should probably
be moved inside the AMD case just above the disable GART TLB check.
> - if (c->x86_vendor == X86_VENDOR_AMD) {
> + switch (c->x86_vendor) {
> + case X86_VENDOR_AMD:
> if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> /*
> * disable GART TBL walk error reporting, which
> @@ -1925,9 +1926,9 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> if (c->x86 >= 0x17 && c->x86 <= 0x1A)
> mce_flags.zen_ifu_quirk = 1;
>
> - }
> + break;
>
Also, why not include the unknown vendor check (right above) inside the
switch case as well?
if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
pr_info("unknown CPU type - not enabling MCE support\n");
return -EOPNOTSUPP;
}
This seems to follow the same pattern as others and can be the first
case inside the switch.
On Fri, Oct 18, 2024 at 12:44:00PM -0700, Sohil Mehta wrote:
> > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> > index 725c1d6fb1e5..40672fe0991a 100644
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> > @@ -1892,7 +1892,8 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> > }
> >
> > /* This should be disabled by the BIOS, but isn't always */
>
> This comment is specific to the AMD and placing it before the switch
> makes it seem generic to the entire switch statement. It should probably
> be moved inside the AMD case just above the disable GART TLB check.
>
> > - if (c->x86_vendor == X86_VENDOR_AMD) {
> > + switch (c->x86_vendor) {
> > + case X86_VENDOR_AMD:
> > if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> > /*
> > * disable GART TBL walk error reporting, which
> > @@ -1925,9 +1926,9 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> > if (c->x86 >= 0x17 && c->x86 <= 0x1A)
> > mce_flags.zen_ifu_quirk = 1;
> >
> > - }
> > + break;
> >
>
>
> Also, why not include the unknown vendor check (right above) inside the
> switch case as well?
>
> if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
> pr_info("unknown CPU type - not enabling MCE support\n");
> return -EOPNOTSUPP;
> }
>
> This seems to follow the same pattern as others and can be the first
> case inside the switch.
The vendor specific bits are large enough to warrant their own
static functions (as we do elsewhere in this file).
How about this (only compile-tested) patch?
-Tony
From 967d8637ac90823f28f4612cbbac305c421b4853 Mon Sep 17 00:00:00 2001
From: Tony Luck <tony.luck@intel.com>
Date: Fri, 18 Oct 2024 13:01:02 -0700
Subject: [PATCH] x86/mce: Break up __mcheck_cpu_apply_quirks()
Split each vendor specific part into its own helper function.
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
arch/x86/kernel/cpu/mce/core.c | 172 ++++++++++++++++++---------------
1 file changed, 96 insertions(+), 76 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2a938f429c4d..f51fb393d369 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1880,101 +1880,121 @@ static void __mcheck_cpu_check_banks(void)
}
}
-/* Add per CPU specific workarounds here */
-static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+static void apply_quirks_amd(struct cpuinfo_x86 *c)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
- if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
- pr_info("unknown CPU type - not enabling MCE support\n");
- return -EOPNOTSUPP;
- }
-
/* This should be disabled by the BIOS, but isn't always */
- if (c->x86_vendor == X86_VENDOR_AMD) {
- if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
- /*
- * disable GART TBL walk error reporting, which
- * trips off incorrectly with the IOMMU & 3ware
- * & Cerberus:
- */
- clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
- }
- if (c->x86 < 0x11 && cfg->bootlog < 0) {
- /*
- * Lots of broken BIOS around that don't clear them
- * by default and leave crap in there. Don't log:
- */
- cfg->bootlog = 0;
- }
+ if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
/*
- * Various K7s with broken bank 0 around. Always disable
- * by default.
+ * disable GART TBL walk error reporting, which
+ * trips off incorrectly with the IOMMU & 3ware
+ * & Cerberus:
*/
- if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].ctl = 0;
-
+ clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
+ }
+ if (c->x86 < 0x11 && cfg->bootlog < 0) {
/*
- * overflow_recov is supported for F15h Models 00h-0fh
- * even though we don't have a CPUID bit for it.
+ * Lots of broken BIOS around that don't clear them
+ * by default and leave crap in there. Don't log:
*/
- if (c->x86 == 0x15 && c->x86_model <= 0xf)
- mce_flags.overflow_recov = 1;
+ cfg->bootlog = 0;
+ }
+ /*
+ * Various K7s with broken bank 0 around. Always disable
+ * by default.
+ */
+ if (c->x86 == 6 && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].ctl = 0;
- if (c->x86 >= 0x17 && c->x86 <= 0x1A)
- mce_flags.zen_ifu_quirk = 1;
+ /*
+ * overflow_recov is supported for F15h Models 00h-0fh
+ * even though we don't have a CPUID bit for it.
+ */
+ if (c->x86 == 0x15 && c->x86_model <= 0xf)
+ mce_flags.overflow_recov = 1;
- }
+ if (c->x86 >= 0x17 && c->x86 <= 0x1A)
+ mce_flags.zen_ifu_quirk = 1;
+}
- if (c->x86_vendor == X86_VENDOR_INTEL) {
- /*
- * SDM documents that on family 6 bank 0 should not be written
- * because it aliases to another special BIOS controlled
- * register.
- * But it's not aliased anymore on model 0x1a+
- * Don't ignore bank 0 completely because there could be a
- * valid event later, merely don't write CTL0.
- */
+static void apply_quirks_intel(struct cpuinfo_x86 *c)
+{
+ struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+ struct mca_config *cfg = &mca_cfg;
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
- mce_banks[0].init = false;
+ /*
+ * SDM documents that on family 6 bank 0 should not be written
+ * because it aliases to another special BIOS controlled
+ * register.
+ * But it's not aliased anymore on model 0x1a+
+ * Don't ignore bank 0 completely because there could be a
+ * valid event later, merely don't write CTL0.
+ */
- /*
- * All newer Intel systems support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
+ if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ mce_banks[0].init = false;
- /*
- * There are also broken BIOSes on some Pentium M and
- * earlier systems:
- */
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
- cfg->bootlog = 0;
+ /*
+ * All newer Intel systems support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
+ cfg->monarch_timeout < 0)
+ cfg->monarch_timeout = USEC_PER_SEC;
- if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
- mce_flags.snb_ifu_quirk = 1;
+ /*
+ * There are also broken BIOSes on some Pentium M and
+ * earlier systems:
+ */
+ if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
+ cfg->bootlog = 0;
- /*
- * Skylake, Cascacde Lake and Cooper Lake require a quirk on
- * rep movs.
- */
- if (c->x86_vfm == INTEL_SKYLAKE_X)
- mce_flags.skx_repmov_quirk = 1;
+ if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
+ mce_flags.snb_ifu_quirk = 1;
+
+ /*
+ * Skylake, Cascacde Lake and Cooper Lake require a quirk on
+ * rep movs.
+ */
+ if (c->x86_vfm == INTEL_SKYLAKE_X)
+ mce_flags.skx_repmov_quirk = 1;
+}
+
+static void apply_quirks_zhaoxin(struct cpuinfo_x86 *c)
+{
+ struct mca_config *cfg = &mca_cfg;
+
+ /*
+ * All newer Zhaoxin CPUs support MCE broadcasting. Enable
+ * synchronization with a one second timeout.
+ */
+ if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
+ if (cfg->monarch_timeout < 0)
+ cfg->monarch_timeout = USEC_PER_SEC;
}
+}
- if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
- /*
- * All newer Zhaoxin CPUs support MCE broadcasting. Enable
- * synchronization with a one second timeout.
- */
- if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
- if (cfg->monarch_timeout < 0)
- cfg->monarch_timeout = USEC_PER_SEC;
- }
+/* Add per CPU specific workarounds here */
+static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+{
+ struct mca_config *cfg = &mca_cfg;
+
+ switch (c->x86_vendor) {
+ case X86_VENDOR_UNKNOWN:
+ pr_info("unknown CPU type - not enabling MCE support\n");
+ return -EOPNOTSUPP;
+
+ case X86_VENDOR_AMD:
+ apply_quirks_amd(c);
+ break;
+ case X86_VENDOR_INTEL:
+ apply_quirks_intel(c);
+ break;
+ case X86_VENDOR_ZHAOXIN:
+ apply_quirks_zhaoxin(c);
+ break;
}
if (cfg->monarch_timeout < 0)
--
2.47.0
>
> From: Luck, Tony <tony.luck@intel.com>
> [...]
> On Fri, Oct 18, 2024 at 12:44:00PM -0700, Sohil Mehta wrote:
> > > diff --git a/arch/x86/kernel/cpu/mce/core.c
> > > b/arch/x86/kernel/cpu/mce/core.c index 725c1d6fb1e5..40672fe0991a
> > > 100644
> > > --- a/arch/x86/kernel/cpu/mce/core.c
> > > +++ b/arch/x86/kernel/cpu/mce/core.c
> > > @@ -1892,7 +1892,8 @@ static int __mcheck_cpu_apply_quirks(struct
> cpuinfo_x86 *c)
> > > }
> > >
> > > /* This should be disabled by the BIOS, but isn't always */
> >
> > This comment is specific to the AMD and placing it before the switch
> > makes it seem generic to the entire switch statement. It should
> > probably be moved inside the AMD case just above the disable GART TLB
> check.
> >
> > > - if (c->x86_vendor == X86_VENDOR_AMD) {
> > > + switch (c->x86_vendor) {
> > > + case X86_VENDOR_AMD:
> > > if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> > > /*
> > > * disable GART TBL walk error reporting, which @@ -
> 1925,9
> > > +1926,9 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> > > if (c->x86 >= 0x17 && c->x86 <= 0x1A)
> > > mce_flags.zen_ifu_quirk = 1;
> > >
> > > - }
> > > + break;
> > >
> >
> >
> > Also, why not include the unknown vendor check (right above) inside
> > the switch case as well?
> >
> > if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
> > pr_info("unknown CPU type - not enabling MCE support\n");
> > return -EOPNOTSUPP;
> > }
> >
> > This seems to follow the same pattern as others and can be the first
> > case inside the switch.
>
> The vendor specific bits are large enough to warrant their own static functions
> (as we do elsewhere in this file).
>
> How about this (only compile-tested) patch?
>
LGTM.
This makes the code more structured and readable.
I'll replace mine with this new patch in my next version after basic testing.
Thanks, Tony & Sohil.
-Qiuxu
On 10/18/2024 1:14 PM, Tony Luck wrote: > The vendor specific bits are large enough to warrant their own > static functions (as we do elsewhere in this file). > > How about this (only compile-tested) patch? > Yeah, it does make it easier to read. Can some of the hardcoded numbers be changed to vfm macros as well?
> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> Subject: Re: [PATCH v2 06/10] x86/mce: Convert multiple if () statements into
> a switch() statement
>
> On 10/18/2024 1:14 PM, Tony Luck wrote:
>
> > The vendor specific bits are large enough to warrant their own static
> > functions (as we do elsewhere in this file).
> >
> > How about this (only compile-tested) patch?
> >
>
> Yeah, it does make it easier to read. Can some of the hardcoded numbers be
> changed to vfm macros as well?
Convert some hard-coded numbers to VFM macros on top of Tony's patch as shown below.
Please review it for any errors or omissions.
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index f51fb393d369..3b83efa1ca65 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
+ /* Older CPUs don't need quirks. */
+ if (c->x86 < 6)
+ return;
+
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1932,23 +1936,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
* Don't ignore bank 0 completely because there could be a
* valid event later, merely don't write CTL0.
*/
-
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
mce_banks[0].init = false;
/*
* All newer Intel systems support MCE broadcasting. Enable
* synchronization with a one second timeout.
*/
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
+ if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0)
cfg->monarch_timeout = USEC_PER_SEC;
/*
* There are also broken BIOSes on some Pentium M and
* earlier systems:
*/
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0)
cfg->bootlog = 0;
if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
On 10/18/2024 10:46 PM, Zhuo, Qiuxu wrote: > Convert some hard-coded numbers to VFM macros on top of Tony's patch as shown below. > Please review it for any errors or omissions. > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index f51fb393d369..3b83efa1ca65 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > struct mca_config *cfg = &mca_cfg; > > + /* Older CPUs don't need quirks. */ > + if (c->x86 < 6) > + return; > + As Dave mentioned, change this to make the use of vfm consistent in the entire function and probably update the comment as well to make it explicit: /* Older CPUs (prior to family 6) don't need quirks */ if (c->x86_vfm < INTEL_PENTIUM_PRO) return; > /* > * SDM documents that on family 6 bank 0 should not be written > * because it aliases to another special BIOS controlled > @@ -1932,23 +1936,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > * Don't ignore bank 0 completely because there could be a > * valid event later, merely don't write CTL0. > */ > - > - if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0) > + if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0) > mce_banks[0].init = false; > > /* > * All newer Intel systems support MCE broadcasting. Enable > * synchronization with a one second timeout. > */ > - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && > - cfg->monarch_timeout < 0) > + if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0) > cfg->monarch_timeout = USEC_PER_SEC; > Instead of keeping this open-ended we could tweak this a bit as follows: if (!(c->x86_vfm < INTEL_CORE_YONAH)) && cfg->monarch_timeout < 0) cfg->monarch_timeout = USEC_PER_SEC; Essentially the same: if (new_cpu) vs if (!old_cpu) Don't have a strong preference. Will leave it to you and Tony. > /* > * There are also broken BIOSes on some Pentium M and > * earlier systems: > */ > - if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0) > + if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0) > cfg->bootlog = 0; > > if (c->x86_vfm == INTEL_SANDYBRIDGE_X) > All the other checks look fine to me.
> From: Mehta, Sohil <sohil.mehta@intel.com> > [...] > As Dave mentioned, change this to make the use of vfm consistent in the > entire function and probably update the comment as well to make it explicit: > > /* Older CPUs (prior to family 6) don't need quirks */ Yes, the improved comment is better. > if (c->x86_vfm < INTEL_PENTIUM_PRO) > return; > > [...] > > - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && > > - cfg->monarch_timeout < 0) > > + if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < > > + 0) > > cfg->monarch_timeout = USEC_PER_SEC; > > > > Instead of keeping this open-ended we could tweak this a bit as follows: > > if (!(c->x86_vfm < INTEL_CORE_YONAH)) && cfg->monarch_timeout < 0) > cfg->monarch_timeout = USEC_PER_SEC; > > Essentially the same: if (new_cpu) vs if (!old_cpu) Don't have a strong > preference. Will leave it to you and Tony. > I prefer the single, straightforward '>=' operation over the '<' and then '!' two operations. - Qiuxu
> /* > * All newer Intel systems support MCE broadcasting. Enable > * synchronization with a one second timeout. > */ > - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && > - cfg->monarch_timeout < 0) > + if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0) > cfg->monarch_timeout = USEC_PER_SEC; This change is correct. But the old code makes it more explicit that CPUs in families > 6 take this action. As the author of the VFM changes it's clear to me, maybe less so to others? But maybe its OK. The comment does help a lot. Anyone else have thoughts on this? -Tony
On 10/21/2024 9:06 AM, Luck, Tony wrote: >> /* >> * All newer Intel systems support MCE broadcasting. Enable >> * synchronization with a one second timeout. >> */ >> - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && >> - cfg->monarch_timeout < 0) >> + if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0) >> cfg->monarch_timeout = USEC_PER_SEC; > > This change is correct. But the old code makes it more explicit that > CPUs in families > 6 take this action. As the author of the VFM changes > it's clear to me, maybe less so to others? > > But maybe its OK. The comment does help a lot. Anyone else have thoughts on this? > I am not very familiar with the intricacies of the VFM checks. I did take me a few minutes to figure out why the modified code is correct. But looking at the prior or the later checks, I see the '<' operator used directly on platform names. So, the new check seems inline with that i.e. in this case, any model or family after the said platform supports MCE broadcasting. > /* > * There are also broken BIOSes on some Pentium M and > * earlier systems: > */ > - if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0) > + if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0) > cfg->bootlog = 0;
> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> On 10/21/2024 9:06 AM, Luck, Tony wrote:
> [...]
> > This change is correct. But the old code makes it more explicit that
> > CPUs in families > 6 take this action. As the author of the VFM
> > changes it's clear to me, maybe less so to others?
> >
> > But maybe its OK. The comment does help a lot. Anyone else have thoughts
> on this?
> >
>
> I am not very familiar with the intricacies of the VFM checks. I did take me a
> few minutes to figure out why the modified code is correct.
OK. So, back to your original question below, what is your answer to it now? :-)
"Can some of the hardcoded numbers be changed to vfm macros as well?"
On 10/22/2024 7:08 PM, Zhuo, Qiuxu wrote: > OK. So, back to your original question below, what is your answer to it now? :-) > > "Can some of the hardcoded numbers be changed to vfm macros as well?" > Even though it takes a tiny bit of reading to understand the VFM macros, the pros significantly outweigh the cons. I still feel we should go ahead and make the changes. I have responded with minor comments to your patch. Sohil
> From: Mehta, Sohil <sohil.mehta@intel.com> > [...] > On 10/22/2024 7:08 PM, Zhuo, Qiuxu wrote: > > OK. So, back to your original question below, what is your answer to > > it now? :-) > > > > "Can some of the hardcoded numbers be changed to vfm macros as > well?" > > > > Even though it takes a tiny bit of reading to understand the VFM macros, the > pros significantly outweigh the cons. I still feel we should go ahead and make > the changes. Thanks for letting me know your thoughts. To me, the VFM-based checks can make the code more compact. So, if nobody else objects, I'll include this VFM-based check version in the next version. - Qiuxu
> I am not very familiar with the intricacies of the VFM checks. I did > take me a few minutes to figure out why the modified code is correct. Hence my concern :-) > But looking at the prior or the later checks, I see the '<' operator > used directly on platform names. So, the new check seems inline with > that i.e. in this case, any model or family after the said platform > supports MCE broadcasting. Intel model number allocation policies aren't necessarily sequential. So range checks need to be used with caution. They should be safe enough when done to simplify code that checks very old models. Range checks across families may be even more problematic. Again these old checks that assume all future families will not reintroduce quirks from 20-year-old CPUs should be safe (I hope!!). But, spoiler alert, Intel is planning to begin use of two families in parallel. Family 19 (first model Diamond Rapids) is already in <asm/intel-family.h>). But there are going to be some CPUs in family 18 too. I'll be surprised if there are any use cases for ranges that span between families 18 and 19. -Tony
On 10/21/2024 10:51 AM, Luck, Tony wrote:
>> But looking at the prior or the later checks, I see the '<' operator
>> used directly on platform names. So, the new check seems inline with
>> that i.e. in this case, any model or family after the said platform
>> supports MCE broadcasting.
>
> Intel model number allocation policies aren't necessarily sequential.
Model numbers are assumed to be sequential at least within family 6.
if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
cfg->monarch_timeout < 0)
There is another check in early_init_intel() which does this:
if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> So range checks need to be used with caution. They should be safe
> enough when done to simplify code that checks very old models.
>
...
> Range checks across families may be even more problematic.
I agree. Maybe we should avoid range checks across families altogether
forward (>) or backward (<).
For example, does the following change from Qiuxu, unintentionally
become applicable to Quark CPUs with family -> 5?
> /*
> * There are also broken BIOSes on some Pentium M and
> * earlier systems:
> */
> - if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
> + if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0)
> cfg->bootlog = 0;
>> Intel model number allocation policies aren't necessarily sequential. > > Model numbers are assumed to be sequential at least within family 6. Assumption can only be applied retroactively to simpler times. Looking at the timelines and model numbers for pure-Atom, pure-Core, Hybrid, and Xeon, they are somewhat jumbled. > For example, does the following change from Qiuxu, unintentionally > become applicable to Quark CPUs with family -> 5? Qiuxu starts the function with: + /* Older CPUs don't need quirks. */ + if (c->x86 < 6) + return; So Quark leaves the function early. -Tony
On 10/21/2024 11:40 AM, Luck, Tony wrote:
>>> Intel model number allocation policies aren't necessarily sequential.
>>
>> Model numbers are assumed to be sequential at least within family 6.
>
> Assumption can only be applied retroactively to simpler times. Looking
> at the timelines and model numbers for pure-Atom, pure-Core, Hybrid,
> and Xeon, they are somewhat jumbled.
>
Agreed. Using range checks within a family with extreme care and
avoiding cross-family ones seems like the saner thing to do.
Maybe everything in the future is enumerated and VFM checks would not be
needed :)
Trying to understand more, I have more questions than answers. With the
introduction of Family 0x19, do we need to reevaluate some of the
existing model checks?
early_init_intel():
if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
(c->x86 == 0x6 && c->x86_model >= 0x0e))
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
It seems "constant_tsc" wouldn't show on Diamond rapids. Do we need it to?
> Qiuxu starts the function with:
>
> + /* Older CPUs don't need quirks. */
> + if (c->x86 < 6)
> + return;
>
> So Quark leaves the function early.
>
Ah! My bad, I missed that.
> -Tony
>
>
On 10/21/24 15:57, Sohil Mehta wrote:
> On 10/21/2024 11:40 AM, Luck, Tony wrote:
>>>> Intel model number allocation policies aren't necessarily sequential.
>>> Model numbers are assumed to be sequential at least within family 6.
>> Assumption can only be applied retroactively to simpler times. Looking
>> at the timelines and model numbers for pure-Atom, pure-Core, Hybrid,
>> and Xeon, they are somewhat jumbled.
>>
> Agreed. Using range checks within a family with extreme care and
> avoiding cross-family ones seems like the saner thing to do.
>
> Maybe everything in the future is enumerated and VFM checks would not be
> needed 🙂
>
> Trying to understand more, I have more questions than answers. With the
> introduction of Family 0x19, do we need to reevaluate some of the
> existing model checks?
>
> early_init_intel():
> if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> (c->x86 == 0x6 && c->x86_model >= 0x0e))
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>
> It seems "constant_tsc" wouldn't show on Diamond rapids. Do we need it to?
We only have a handful of these and they're mostly for early family 6
things. I bet there's less than half a dozen.
Let's just list them in one of our match structures:
const u32 NOT_SUPPORTED = UINT_MAX; // or another special, invalid VFM
const u32 ALL_SUPPORTED = 0;
static const struct x86_cpu_id table[] __initconst = {
X86_MATCH_FAM(INTEL, 3, NOT_SUPPORTED),
X86_MATCH_FAM(INTEL, 4, NOT_SUPPORTED),
X86_MATCH_FAM(INTEL, 5, NOT_SUPPORTED),
X86_MATCH_FAM(INTEL, 6, INTEL_CORE_YONAH),
X86_MATCH_FAM(INTEL, 0xf, INTEL_P4_WHATEVER),
X86_MATCH_VEN(INTEL, ALL_SUPPORTED),
};
... and then use it like this:
m = x86_match_cpu(table);
// Non-Intel lands here:
if (!m)
return false;
if (VFM_MODEL(c->x86_vfm) >= m->driver_data)
return true;
return false;
On 10/21/2024 5:17 PM, Dave Hansen wrote: > We only have a handful of these and they're mostly for early family 6 > things. I bet there's less than half a dozen. > You are right. There don't seem to be many unbounded model checks for Intel family 6. I could only find 3. early_init_intel() -> constant_tsc - Tony found out that it is harmless since it got it's own enumeration later on. should_io_be_busy() and acpi_processor_power_init_bm_check() also seem to be for older platforms and probably no longer applicable. I'll reach out to the power folks to confirm. Maybe if we just add an upper bound to these checks then we don't to worry about carrying them forward with the newer family 6 models and upcoming family 19 models.
> Trying to understand more, I have more questions than answers. With the
> introduction of Family 0x19, do we need to reevaluate some of the
> existing model checks?
Diamond Rapids is in Family 19, not 0x19. I was unsure in <asm/intel-family.h>
to use decimal or hex for family (since only 5 & 6 are used there, and they are
same in both bases). I picked decimal to avoid 0x prefixes everywhere.
> early_init_intel():
> if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> (c->x86 == 0x6 && c->x86_model >= 0x0e))
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>
> It seems "constant_tsc" wouldn't show on Diamond rapids. Do we need it to?
This looks to be checking for Pentium IV Prescott or newer in
family 0xf, or Yonah or newer in family 6.
You are right that it won't catch the new families. But it might
not matter if this later block sets the feature bit.
/*
* c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
* with P/T states and does not stop in deep C-states.
*
* It is also reliable across cores and sockets. (but not across
* cabinets - we turn it off in that case explicitly.)
*/
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
}
It appears that constant TSC started out model specific, but later
got a proper enumeration bit in CPUID.
-Tony
On 10/21/2024 4:31 PM, Luck, Tony wrote: > Diamond Rapids is in Family 19, not 0x19. I was unsure in <asm/intel-family.h> > to use decimal or hex for family (since only 5 & 6 are used there, and they are > same in both bases). I picked decimal to avoid 0x prefixes everywhere. > Got it. In intel-family.h it probably doesn't matter. But across x86 the family checks seem to be a mix of decimal and hexadecimal with maybe more inclination towards hex. > It appears that constant TSC started out model specific, but later > got a proper enumeration bit in CPUID. > Ah! Makes sense. > -Tony > >
On 10/21/24 09:06, Luck, Tony wrote: >> /* >> * All newer Intel systems support MCE broadcasting. Enable >> * synchronization with a one second timeout. >> */ >> - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && >> - cfg->monarch_timeout < 0) >> + if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0) >> cfg->monarch_timeout = USEC_PER_SEC; > This change is correct. But the old code makes it more explicit that > CPUs in families > 6 take this action. As the author of the VFM changes > it's clear to me, maybe less so to others? > > But maybe its OK. The comment does help a lot. Anyone else have thoughts on this? It certainly is a bit subtle. To me, the earlier check would be even better if it were: - if (c->x86 < 6) + if (c->x86_vfm < INTEL_PENTIUM_PRO) return; That at least makes it more clear that it's a range of models and avoids having a ->x86 check mixed with a ->x86_vfm one.
> From: Hansen, Dave <dave.hansen@intel.com> > [...] > On 10/21/24 09:06, Luck, Tony wrote: > >> /* > >> * All newer Intel systems support MCE broadcasting. Enable > >> * synchronization with a one second timeout. > >> */ > >> - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && > >> - cfg->monarch_timeout < 0) > >> + if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < > >> + 0) > >> cfg->monarch_timeout = USEC_PER_SEC; > > This change is correct. But the old code makes it more explicit that > > CPUs in families > 6 take this action. As the author of the VFM > > changes it's clear to me, maybe less so to others? > > > > But maybe its OK. The comment does help a lot. Anyone else have thoughts > on this? > > It certainly is a bit subtle. > > To me, the earlier check would be even better if it were: > > - if (c->x86 < 6) Thanks, Dave. OK, I'll update it in the next version. Apart from this, I'll also add a comment below, as suggested by Sohil, to make it explicit that it's for prior to family 6. /* Older CPUs (prior to family 6) don't need quirks */ > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > return; > > That at least makes it more clear that it's a range of models and avoids having > a ->x86 check mixed with a ->x86_vfm one.
> From: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> [...]
> Subject: RE: [PATCH v2 06/10] x86/mce: Convert multiple if () statements into
> a switch() statement
>
> > From: Hansen, Dave <dave.hansen@intel.com> [...] On 10/21/24 09:06,
> [...]
> >
> > It certainly is a bit subtle.
> >
> > To me, the earlier check would be even better if it were:
> >
> > - if (c->x86 < 6)
>
> Thanks, Dave.
> OK, I'll update it in the next version.
> Apart from this, I'll also add a comment below, as suggested by Sohil, to make
> it explicit that it's for prior to family 6.
>
> /* Older CPUs (prior to family 6) don't need quirks */
> > + if (c->x86_vfm < INTEL_PENTIUM_PRO)
> > return;
> >
> > That at least makes it more clear that it's a range of models and
> > avoids having a ->x86 check mixed with a ->x86_vfm one.
Hi @Luck, Tony, @Hansen, Dave, @Mehta, Sohil,
Thanks for your time discussing the VFM-based checks.
I made the patch on top of Tony's [1] based on what we've discussed.
I'd like to add the tags from you to the following patch.
Please let me know if these tags are OK with you. Thanks!
[1] https://lore.kernel.org/all/ZxLBwO4HkkJG4WYn@agluck-desk3.sc.intel.com/
From 6e88743f0619a902c6e6f985b9fc93669098b4af Mon Sep 17 00:00:00 2001
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Date: Mon, 21 Oct 2024 10:42:23 +0800
Subject: [PATCH v3 07/10] x86/mce: Convert family/model mixed checks to
VFM-based checks
Convert family/model mixed checks to VFM-based checks to make
the code more compact.
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index bb8b1000fa0a..936804a5a0b9 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
struct mca_config *cfg = &mca_cfg;
+ /* Older CPUs (prior to family 6) don't need quirks. */
+ if (c->x86_vfm < INTEL_PENTIUM_PRO)
+ return;
+
/*
* SDM documents that on family 6 bank 0 should not be written
* because it aliases to another special BIOS controlled
@@ -1932,22 +1936,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c)
* Don't ignore bank 0 completely because there could be a
* valid event later, merely don't write CTL0.
*/
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
mce_banks[0].init = false;
/*
* All newer Intel systems support MCE broadcasting. Enable
* synchronization with a one second timeout.
*/
- if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) &&
- cfg->monarch_timeout < 0)
+ if (c->x86_vfm >= INTEL_CORE_YONAH && cfg->monarch_timeout < 0)
cfg->monarch_timeout = USEC_PER_SEC;
/*
* There are also broken BIOSes on some Pentium M and
* earlier systems:
*/
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0)
cfg->bootlog = 0;
if (c->x86_vfm == INTEL_SANDYBRIDGE_X)
--
2.17.1
- if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0)
+ if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
mce_banks[0].init = false;
Updated code now matches for families before 6 (486, Pentium). 486 would never get
to this code. But I think from the comments about machine check bank 0 being magic
that Pentium had some rudimentary support.
Should this be:
if (c->x86_vfm >= INTEL_PENTIUM_PRO && c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0)
to avoid this semantic change?
- if (c->x86 == 6 && c->x86_model <= 13 && cfg->bootlog < 0)
+ if (c->x86_vfm < INTEL_CORE_YONAH && cfg->bootlog < 0)
cfg->bootlog = 0;
Same as above. New code matches for families 4 and 5.
-Tony
On 10/24/2024 9:42 AM, Luck, Tony wrote: > - if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0) > + if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0) > mce_banks[0].init = false; > > Updated code now matches for families before 6 (486, Pentium). 486 would never get > to this code. But I think from the comments about machine check bank 0 being magic > that Pentium had some rudimentary support. > As you mentioned it yourself (the last time I was concerned about family 5), the following check should cover this scenario? > @@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > struct mca_config *cfg = &mca_cfg; > > + /* Older CPUs (prior to family 6) don't need quirks. */ > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > + return; > + Sohil
On 10/24/24 14:31, Sohil Mehta wrote: > @@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > struct mca_config *cfg = &mca_cfg; > > + /* Older CPUs (prior to family 6) don't need quirks. */ > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > + return; In case anyone grumbles, this is one of those expressions that's not perfect, but I think it's quite good enough in practice. It wouldn't work if we ever (for instance) moved 'vendor' to be less significant bits than 'model'. Or on a CPU that claimed to be family=6 but model=0. But Intel never (as far as I know) sold a CPU like that, so it's probably only possible in a VM where these checks are rather worthless anyway. In short, I think >=INTEL_PENTIUM_PRO is a great check that can mean "family 6 or later" almost anywhere.
> > - if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0) > > + if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0) > > mce_banks[0].init = false; > > > > Updated code now matches for families before 6 (486, Pentium). 486 would never get > > to this code. But I think from the comments about machine check bank 0 being magic > > that Pentium had some rudimentary support. > > > > As you mentioned it yourself (the last time I was concerned about family > 5), the following check should cover this scenario? > > > @@ -1924,6 +1924,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > > struct mca_config *cfg = &mca_cfg; > > > > + /* Older CPUs (prior to family 6) don't need quirks. */ > > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > > + return; So I did. I was right about it too. Sorry for the noise. This patch looks good. -Tony
Remove the unnecessary {} from the case statement.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 40672fe0991a..e718b9bbe8e5 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2073,10 +2073,9 @@ static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
mce_intel_feature_init(c);
break;
- case X86_VENDOR_AMD: {
+ case X86_VENDOR_AMD:
mce_amd_feature_init(c);
break;
- }
case X86_VENDOR_HYGON:
mce_hygon_feature_init(c);
--
2.17.1As the entire mce structure is initialized to zero using memset(0)
within mce_gather_info(), remove the redundant zeroing assignments to
mce->misc and mce->addr.
This results in a reduction of 64 bytes in the text size.
$ size core.o.*
text data bss dec hex filename
21348 4181 3776 29305 7279 core.o.old
21284 4181 3776 29241 7239 core.o.new
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/core.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index e718b9bbe8e5..844a6f8d6f39 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -706,8 +706,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
if (!mce_banks[i].ctl || !test_bit(i, *b))
continue;
- m.misc = 0;
- m.addr = 0;
m.bank = i;
barrier();
@@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *final,
if (!mce_banks[i].ctl)
continue;
- m->misc = 0;
- m->addr = 0;
m->bank = i;
m->status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
--
2.17.1On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote: > As the entire mce structure is initialized to zero using memset(0) > within mce_gather_info(), remove the redundant zeroing assignments to > mce->misc and mce->addr. > ... > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index e718b9bbe8e5..844a6f8d6f39 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -706,8 +706,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b) > if (!mce_banks[i].ctl || !test_bit(i, *b)) > continue; > > - m.misc = 0; > - m.addr = 0; > m.bank = i; > This makes sense since mce_gather_info() happens in the same function. > barrier(); > @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *final, > if (!mce_banks[i].ctl) > continue; > > - m->misc = 0; > - m->addr = 0; > m->bank = i; > However, in this case, I am not fully convinced if the misc and addr would already be 0 when we reach here. There are potentially a lot of things that happen in do_machine_check() between mce_gather_info() and __mc_scan_banks(). Especially, mce_no_way_out() which could theoretically call mce_read_aux() in some cases. Maybe it doesn't matter, misc and addr would be overwritten anyway. But I feel some more details in the commit message would be useful. It doesn't seem as simple as the brief description makes it sound (at least to me). > m->status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
> From: Mehta, Sohil <sohil.mehta@intel.com> > [...] > > @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs > *regs, struct mce *final, > > if (!mce_banks[i].ctl) > > continue; > > > > - m->misc = 0; > > - m->addr = 0; > > m->bank = i; > > > > However, in this case, I am not fully convinced if the misc and addr would > already be 0 when we reach here. > > There are potentially a lot of things that happen in do_machine_check() > between mce_gather_info() and __mc_scan_banks(). Especially, > mce_no_way_out() which could theoretically call mce_read_aux() in some > cases. > > Maybe it doesn't matter, misc and addr would be overwritten anyway. But I > feel some more details in the commit message would be useful. It doesn't > seem as simple as the brief description makes it sound (at least to me). > Your concern is reasonable. Thanks! For both diffs, mce->misc and mce->addr can be guaranteed to be zeroed the first time they reach here. However, I didn't notice that both diffs were in a for() loop where mce->misc and mce->addr could retain the old values assigned by mce_read_aux() in the previous iteration. So need to zero mce-misc and mce->addr in each iteration to ensure they don't contain stale values. I'll drop this patch in the next version. -Qiuxu
On October 19, 2024 12:37:05 AM PDT, "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com> wrote: >> From: Mehta, Sohil <sohil.mehta@intel.com> >> [...] >> > @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs >> *regs, struct mce *final, >> > if (!mce_banks[i].ctl) >> > continue; >> > >> > - m->misc = 0; >> > - m->addr = 0; >> > m->bank = i; >> > >> >> However, in this case, I am not fully convinced if the misc and addr would >> already be 0 when we reach here. >> >> There are potentially a lot of things that happen in do_machine_check() >> between mce_gather_info() and __mc_scan_banks(). Especially, >> mce_no_way_out() which could theoretically call mce_read_aux() in some >> cases. >> >> Maybe it doesn't matter, misc and addr would be overwritten anyway. But I >> feel some more details in the commit message would be useful. It doesn't >> seem as simple as the brief description makes it sound (at least to me). >> > >Your concern is reasonable. Thanks! > >For both diffs, mce->misc and mce->addr can be guaranteed to be zeroed the first time >they reach here. However, I didn't notice that both diffs were in a for() loop where >mce->misc and mce->addr could retain the old values assigned by mce_read_aux() in >the previous iteration. So need to zero mce-misc and mce->addr in each iteration to >ensure they don't contain stale values. > > I'll drop this patch in the next version. > >-Qiuxu > Keep in mind that usually the compiler will remove redundant assignments, and if they are too obscure for the compiler to discover, they are probably too subtle for programmers to not introduce bugs in the future ...
> From: H. Peter Anvin <hpa@zytor.com> > [...] > > Keep in mind that usually the compiler will remove redundant assignments, > and if they are too obscure for the compiler to discover, they are probably too > subtle for programmers to not introduce bugs in the future ... Thanks, H.Peter. This is a good tip to quickly check whether a cleanup of removing unnecessary assignments changes the function. If there is no difference in the text before and after the cleanup, then it's OK. Otherwise, the cleanup probably changes the function in an unintended way. -Qiuxu
On October 19, 2024 1:30:04 AM PDT, "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com> wrote: >> From: H. Peter Anvin <hpa@zytor.com> >> [...] >> >> Keep in mind that usually the compiler will remove redundant assignments, >> and if they are too obscure for the compiler to discover, they are probably too >> subtle for programmers to not introduce bugs in the future ... > >Thanks, H.Peter. > >This is a good tip to quickly check whether a cleanup of removing unnecessary >assignments changes the function. If there is no difference in the text before and >after the cleanup, then it's OK. Otherwise, the cleanup probably changes the >function in an unintended way. > >-Qiuxu > Yes and no. Deleting things like redundant reinitialization should only be done if it makes the code clearer. You can think of the redundant statements as comments/asserts.
> From: H. Peter Anvin <hpa@zytor.com> > [...] > >This is a good tip to quickly check whether a cleanup of removing > >unnecessary assignments changes the function. If there is no difference > >in the text before and after the cleanup, then it's OK. Otherwise, the > >cleanup probably changes the function in an unintended way. > > > >-Qiuxu > > > > Yes and no. Deleting things like redundant reinitialization should only be done > if it makes the code clearer. You can think of the redundant statements as > comments/asserts. Thanks for the further clarification. -Qiuxu
As the variables {pos, tmp, block, first_block} are all initialized
prior to their use, remove the unnecessary NULL pointer initializations.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
Changes in v2:
- Update the commit message to add the left out variable names {block, first_block}
that this patch also fixes.
arch/x86/kernel/cpu/mce/amd.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 4dae9841ee38..aecea842dac2 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -917,8 +917,8 @@ static void log_and_reset_block(struct threshold_block *block)
*/
static void amd_threshold_interrupt(void)
{
- struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
struct threshold_bank **bp = this_cpu_read(threshold_banks);
+ struct threshold_block *first_block, *block, *tmp;
unsigned int bank, cpu = smp_processor_id();
/*
@@ -1197,8 +1197,7 @@ static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb
static int __threshold_add_blocks(struct threshold_bank *b)
{
struct list_head *head = &b->blocks->miscj;
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
int err = 0;
err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name);
@@ -1308,8 +1307,7 @@ static void deallocate_threshold_blocks(struct threshold_bank *bank)
static void __threshold_remove_blocks(struct threshold_bank *b)
{
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct threshold_block *pos, *tmp;
kobject_put(b->kobj);
--
2.17.1On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> As the variables {pos, tmp, block, first_block} are all initialized
This level of detail is generally not needed in the commit message. I
would rather just skip the {} brackets altogether.
> prior to their use, remove the unnecessary NULL pointer initializations.
>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> Subject: Re: [PATCH v2 09/10] x86/mce/amd: Remove unnecessary NULL
> pointer initializations
>
> On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> > As the variables {pos, tmp, block, first_block} are all initialized
>
> This level of detail is generally not needed in the commit message. I would
> rather just skip the {} brackets altogether.
OK. I'll take your suggestion into account in the next version.
-Qiuxu
Fix the following typos in comments:
s/callin/calling/
s/TBL/TLB/
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
arch/x86/kernel/cpu/mce/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 844a6f8d6f39..19e6730e7c22 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1118,7 +1118,7 @@ static noinstr int mce_start(int *no_way_out)
} else {
/*
* Subject: Now start the scanning loop one by one in
- * the original callin order.
+ * the original calling order.
* This way when there are any shared banks it will be
* only seen by one CPU before cleared, avoiding duplicates.
*/
@@ -1892,7 +1892,7 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
case X86_VENDOR_AMD:
if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
/*
- * disable GART TBL walk error reporting, which
+ * disable GART TLB walk error reporting, which
* trips off incorrectly with the IOMMU & 3ware
* & Cerberus:
*/
--
2.17.1On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> Fix the following typos in comments:
>
> s/callin/calling/
> s/TBL/TLB/
>
Same as before. The exact change with details doesn't need to be listed
out again in the commit message. It can easily be observed from the diff.
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> arch/x86/kernel/cpu/mce/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 844a6f8d6f39..19e6730e7c22 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1118,7 +1118,7 @@ static noinstr int mce_start(int *no_way_out)
> } else {
> /*
> * Subject: Now start the scanning loop one by one in
> - * the original callin order.
> + * the original calling order.
> * This way when there are any shared banks it will be
> * only seen by one CPU before cleared, avoiding duplicates.
> */
> @@ -1892,7 +1892,7 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
> case X86_VENDOR_AMD:
> if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
> /*
> - * disable GART TBL walk error reporting, which
> + * disable GART TLB walk error reporting, which
> * trips off incorrectly with the IOMMU & 3ware
> * & Cerberus:
> */
> From: Mehta, Sohil <sohil.mehta@intel.com> > [...] > On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote: > > Fix the following typos in comments: > > > > s/callin/calling/ > > s/TBL/TLB/ > > > > Same as before. The exact change with details doesn't need to be listed out > again in the commit message. It can easily be observed from the diff. Same as before. Will update it in the next version. -Qiuxu
© 2016 - 2025 Red Hat, Inc.