diff mbox series

[v4,2/3] x86/mce: Define function to extract ErrorAddr from MCA_ADDR

Message ID 20220225193342.215780-3-Smita.KoralahalliChannabasappa@amd.com (mailing list archive)
State New, archived
Headers show
Series x86/mce: Support extended MCA_ADDR address on SMCA systems | expand

Commit Message

Smita Koralahalli Feb. 25, 2022, 7:33 p.m. UTC 
Move MCA_ADDR[ErrorAddr] extraction into a separate helper function. This
will be further refactored to support extended ErrorAddr bits in MCA_ADDR
in newer AMD processors such as AMD 'Milan'.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20220211223442.254489-2-Smita.KoralahalliChannabasappa@amd.com

v2:
	No change.
v3:
	Rebased on the latest tip tree. No functional changes.
v4:
	Commit description change to be void of the patch linearity.
---
 arch/x86/include/asm/mce.h     |  2 ++
 arch/x86/kernel/cpu/mce/amd.c  | 14 +++++++++-----
 arch/x86/kernel/cpu/mce/core.c |  7 ++-----
 3 files changed, 13 insertions(+), 10 deletions(-)

Comments

Borislav Petkov March 31, 2022, 1:24 p.m. UTC | #1 
On Fri, Feb 25, 2022 at 01:33:41PM -0600, Smita Koralahalli wrote:
> Move MCA_ADDR[ErrorAddr] extraction into a separate helper function. This
> will be further refactored to support extended ErrorAddr bits in MCA_ADDR
> in newer AMD processors such as AMD 'Milan'.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> Link:
> https://lkml.kernel.org/r/20220211223442.254489-2-Smita.KoralahalliChannabasappa@amd.com
> 
> v2:
> 	No change.
> v3:
> 	Rebased on the latest tip tree. No functional changes.
> v4:
> 	Commit description change to be void of the patch linearity.
> ---
>  arch/x86/include/asm/mce.h     |  2 ++
>  arch/x86/kernel/cpu/mce/amd.c  | 14 +++++++++-----
>  arch/x86/kernel/cpu/mce/core.c |  7 ++-----
>  3 files changed, 13 insertions(+), 10 deletions(-)

So if you're going to extract functionality, make sure you extract it
all and keep it all encapsulated in a single function, see below.

Now take this one pls and do your patch 3 ontop by extending the comment
over smca_extract_err_addr() with the new functionality.

Thx.

---
From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Date: Fri, 25 Feb 2022 13:33:41 -0600
Subject: [PATCH] x86/mce: Define a function to extract ErrorAddr from MCA_ADDR

Move MCA_ADDR[ErrorAddr] extraction into a separate helper function. This
will be further refactored to support extended ErrorAddr bits in MCA_ADDR
in newer AMD CPUs.

  [ bp: Massage. ]

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Link: https://lore.kernel.org/r/20220225193342.215780-3-Smita.KoralahalliChannabasappa@amd.com
---
 arch/x86/include/asm/mce.h     |  2 ++
 arch/x86/kernel/cpu/mce/amd.c  | 23 ++++++++++++++---------
 arch/x86/kernel/cpu/mce/core.c | 11 +----------
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cc73061e7255..a1da72941f4e 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -337,12 +337,14 @@ extern int mce_threshold_remove_device(unsigned int cpu);
 
 void mce_amd_feature_init(struct cpuinfo_x86 *c);
 enum smca_bank_types smca_get_bank_type(unsigned int cpu, unsigned int bank);
+void smca_extract_err_addr(struct mce *m);
 #else
 
 static inline int mce_threshold_create_device(unsigned int cpu)		{ return 0; };
 static inline int mce_threshold_remove_device(unsigned int cpu)		{ return 0; };
 static inline bool amd_mce_is_memory_error(struct mce *m)		{ return false; };
 static inline void mce_amd_feature_init(struct cpuinfo_x86 *c)		{ }
+static inline void smca_extract_err_addr(struct mce *m)			{ }
 #endif
 
 static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c)	{ return mce_amd_feature_init(c); }
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1940d305db1c..a1a4a5dc53e8 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -722,6 +722,19 @@ bool amd_mce_is_memory_error(struct mce *m)
 	return m->bank == 4 && xec == 0x8;
 }
 
+/* Extract [55:<lsb>] where lsb is the LS-*valid* bit of the address bits. */
+void smca_extract_err_addr(struct mce *m)
+{
+	u8 lsb;
+
+	if (!mce_flags.smca)
+		return;
+
+	lsb = (m->addr >> 56) & 0x3f;
+
+	m->addr &= GENMASK_ULL(55, lsb);
+}
+
 static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
 {
 	struct mce m;
@@ -736,15 +749,7 @@ static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
 	if (m.status & MCI_STATUS_ADDRV) {
 		m.addr = addr;
 
-		/*
-		 * Extract [55:<lsb>] where lsb is the least significant
-		 * *valid* bit of the address bits.
-		 */
-		if (mce_flags.smca) {
-			u8 lsb = (m.addr >> 56) & 0x3f;
-
-			m.addr &= GENMASK_ULL(55, lsb);
-		}
+		smca_extract_err_addr(&m);
 	}
 
 	if (mce_flags.smca) {
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index d775fcd74e98..5ba2df911d19 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -632,16 +632,7 @@ static noinstr void mce_read_aux(struct mce *m, int i)
 			m->addr >>= shift;
 			m->addr <<= shift;
 		}
-
-		/*
-		 * Extract [55:<lsb>] where lsb is the least significant
-		 * *valid* bit of the address bits.
-		 */
-		if (mce_flags.smca) {
-			u8 lsb = (m->addr >> 56) & 0x3f;
-
-			m->addr &= GENMASK_ULL(55, lsb);
-		}
+		smca_extract_err_addr(m);
 	}
 
 	if (mce_flags.smca) {
Borislav Petkov April 3, 2022, 1:16 p.m. UTC | #2 
On Thu, Mar 31, 2022 at 03:24:37PM +0200, Borislav Petkov wrote:
> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index 1940d305db1c..a1a4a5dc53e8 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -722,6 +722,19 @@ bool amd_mce_is_memory_error(struct mce *m)
>  	return m->bank == 4 && xec == 0x8;
>  }
>  
> +/* Extract [55:<lsb>] where lsb is the LS-*valid* bit of the address bits. */
> +void smca_extract_err_addr(struct mce *m)

In addition:

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 9ccc2ea0ea00..4acc7959be6e 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -723,7 +723,7 @@ bool amd_mce_is_memory_error(struct mce *m)
 }
 
 /* Extract [55:<lsb>] where lsb is the LS-*valid* bit of the address bits. */
-void smca_extract_err_addr(struct mce *m)
+void __always_inline smca_extract_err_addr(struct mce *m)
 {
 	u8 lsb;
 

because some compilers cause:

vmlinux.o: warning: objtool: mce_read_aux()+0x82: call to smca_extract_err_addr() leaves .noinstr.text section
Borislav Petkov April 3, 2022, 6:50 p.m. UTC | #3 
On Sun, Apr 03, 2022 at 03:16:20PM +0200, Borislav Petkov wrote:
> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index 9ccc2ea0ea00..4acc7959be6e 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -723,7 +723,7 @@ bool amd_mce_is_memory_error(struct mce *m)
>  }
>  
>  /* Extract [55:<lsb>] where lsb is the LS-*valid* bit of the address bits. */
> -void smca_extract_err_addr(struct mce *m)
> +void __always_inline smca_extract_err_addr(struct mce *m)

Ok, flip those - the pedantic bot is not happy:

>> arch/x86/kernel/cpu/mce/amd.c:726:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
     726 | void __always_inline smca_extract_err_addr(struct mce *m)
         | ^~~~

Needs to be

__always_inline void

whateva...
Thomas Gleixner April 3, 2022, 7:58 p.m. UTC | #4 
On Sun, Apr 03 2022 at 20:50, Borislav Petkov wrote:
> On Sun, Apr 03, 2022 at 03:16:20PM +0200, Borislav Petkov wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
>> index 9ccc2ea0ea00..4acc7959be6e 100644
>> --- a/arch/x86/kernel/cpu/mce/amd.c
>> +++ b/arch/x86/kernel/cpu/mce/amd.c
>> @@ -723,7 +723,7 @@ bool amd_mce_is_memory_error(struct mce *m)
>>  }
>>  
>>  /* Extract [55:<lsb>] where lsb is the LS-*valid* bit of the address bits. */
>> -void smca_extract_err_addr(struct mce *m)
>> +void __always_inline smca_extract_err_addr(struct mce *m)
>
> Ok, flip those - the pedantic bot is not happy:
>
>>> arch/x86/kernel/cpu/mce/amd.c:726:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
>      726 | void __always_inline smca_extract_err_addr(struct mce *m)
>          | ^~~~
>
> Needs to be
>
> __always_inline void
>
> whateva...

How is __always_inline supposed to work across compilation units w/o
LTO? The callsite is in core.c ...

Thanks,

        tglx
Borislav Petkov April 3, 2022, 8:44 p.m. UTC | #5 
On Sun, Apr 03, 2022 at 09:58:07PM +0200, Thomas Gleixner wrote:
> How is __always_inline supposed to work across compilation units w/o
> LTO? The callsite is in core.c ...

Hmm, right.

So even with patch 3 adding more changes to that function I think it is
still simple enough so that we can move it up into the mce/internal.h
header so that the inlining can work.

Thx.
Smita Koralahalli April 4, 2022, 8:55 p.m. UTC | #6 
On 4/3/2022 1:44 PM, Borislav Petkov wrote:
> On Sun, Apr 03, 2022 at 09:58:07PM +0200, Thomas Gleixner wrote:
>> How is __always_inline supposed to work across compilation units w/o
>> LTO? The callsite is in core.c ...
> Hmm, right.
>
> So even with patch 3 adding more changes to that function I think it is
> still simple enough so that we can move it up into the mce/internal.h
> header so that the inlining can work.
Ok will incorporate all changes.

I didn't quite understand what needs to be moved to mce/internal.h. Was that
addressed to me?  The function call smca_extract_err_addr() is in mce/core.c
and the definition in mce.h
>
> Thx.
>
Borislav Petkov April 4, 2022, 9:56 p.m. UTC | #7 
On Mon, Apr 04, 2022 at 01:55:21PM -0700, Smita Koralahalli wrote:
> I didn't quite understand what needs to be moved to mce/internal.h. Was that
> addressed to me?  The function call smca_extract_err_addr() is in mce/core.c
> and the definition in mce.h

In your current v4, the function definition is in
arch/x86/kernel/cpu/mce/amd.c

However, since it needs to be inlined into both callsites because
mce_read_aux() is marked noinstr, the definition should be

static __always_inline void smca_extract_err_addr(struct mce *m)

and that definition should be in the header mce/internal.h

Thx.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cc73061e7255..99a4c32cbdfa 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -337,12 +337,14 @@  extern int mce_threshold_remove_device(unsigned int cpu);
 
 void mce_amd_feature_init(struct cpuinfo_x86 *c);
 enum smca_bank_types smca_get_bank_type(unsigned int cpu, unsigned int bank);
+void smca_extract_err_addr(struct mce *m);
 #else
 
 static inline int mce_threshold_create_device(unsigned int cpu)		{ return 0; };
 static inline int mce_threshold_remove_device(unsigned int cpu)		{ return 0; };
 static inline bool amd_mce_is_memory_error(struct mce *m)		{ return false; };
 static inline void mce_amd_feature_init(struct cpuinfo_x86 *c)		{ }
+static inline void smca_extract_err_addr(struct mce *m) { }
 #endif
 
 static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c)	{ return mce_amd_feature_init(c); }
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1940d305db1c..981d718851a2 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -722,6 +722,13 @@  bool amd_mce_is_memory_error(struct mce *m)
 	return m->bank == 4 && xec == 0x8;
 }
 
+void smca_extract_err_addr(struct mce *m)
+{
+	u8 lsb = (m->addr >> 56) & 0x3f;
+
+	m->addr &= GENMASK_ULL(55, lsb);
+}
+
 static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
 {
 	struct mce m;
@@ -740,11 +747,8 @@  static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
 		 * Extract [55:<lsb>] where lsb is the least significant
 		 * *valid* bit of the address bits.
 		 */
-		if (mce_flags.smca) {
-			u8 lsb = (m.addr >> 56) & 0x3f;
-
-			m.addr &= GENMASK_ULL(55, lsb);
-		}
+		if (mce_flags.smca)
+			smca_extract_err_addr(&m);
 	}
 
 	if (mce_flags.smca) {
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index c0e9aa9c8749..313058dc129f 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -645,11 +645,8 @@  static noinstr void mce_read_aux(struct mce *m, int i)
 		 * Extract [55:<lsb>] where lsb is the least significant
 		 * *valid* bit of the address bits.
 		 */
-		if (mce_flags.smca) {
-			u8 lsb = (m->addr >> 56) & 0x3f;
-
-			m->addr &= GENMASK_ULL(55, lsb);
-		}
+		if (mce_flags.smca)
+			smca_extract_err_addr(m);
 	}
 
 	if (mce_flags.smca) {