Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().
Also, restructure the code to avoid multiple initializations for MCA
registers. SMCA machines define a different set of MSRs for MCA registers
and mca_msr_reg() returns the proper MSR address for SMCA and legacy
processors.
Initialize MCA_MISC and MCA_SYND registers at the end after initializing
MCx_{STATUS, DESTAT} which is further explained in the next patch.
Make mca_msr_reg() exportable in order to be accessible from mce-inject
module.
Signed-off-by: Smita Koralahalli <[email protected]>
---
v2:
Replaced msr_ops -> mca_msr_reg().
---
arch/x86/kernel/cpu/mce/core.c | 1 +
arch/x86/kernel/cpu/mce/inject.c | 27 +++++++++++++--------------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 6ed365337a3b..fb4d8ac1cb4f 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -194,6 +194,7 @@ u32 mca_msr_reg(int bank, enum mca_msr reg)
return 0;
}
+EXPORT_SYMBOL_GPL(mca_msr_reg);
static void __print_mce(struct mce *m)
{
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index a993dc3d0333..40d0bebe0cd2 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -461,22 +461,21 @@ static void prepare_msrs(void *info)
wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
- if (boot_cpu_has(X86_FEATURE_SMCA)) {
- if (m.inject_flags == DFR_INT_INJ) {
- wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
- wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
- } else {
- wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
- wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
- }
+ if (boot_cpu_has(X86_FEATURE_SMCA) &&
+ m.inject_flags == DFR_INT_INJ) {
+ wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
+ wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
+ goto out;
+ }
+
+ wrmsrl(mca_msr_reg(b, MCA_STATUS), m.status);
+ wrmsrl(mca_msr_reg(b, MCA_ADDR), m.addr);
- wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m.misc);
+out:
+ wrmsrl(mca_msr_reg(b, MCA_MISC), m.misc);
+
+ if (boot_cpu_has(X86_FEATURE_SMCA))
wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
- } else {
- wrmsrl(MSR_IA32_MCx_STATUS(b), m.status);
- wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
- wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
- }
}
static void do_inject(void)
--
2.17.1
On Tue, Oct 19, 2021 at 06:36:39PM -0500, Smita Koralahalli wrote:
> Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().
And this is where your commit message and patch should end. It is a bad
idea to do textual replacements *and* functional changes in a single
patch: it is hard to review and debug if there are possible issues. So
you do the textual replacements in the first one and then the functional
changes in subsequent patches.
> Also, restructure the code to avoid multiple initializations for MCA
> registers.
What multiple initializations?
> SMCA machines define a different set of MSRs for MCA registers
> and mca_msr_reg() returns the proper MSR address for SMCA and legacy
> processors.
>
> Initialize MCA_MISC and MCA_SYND registers at the end after initializing
> MCx_{STATUS, DESTAT} which is further explained in the next patch.
And this should be *in* the next patch.
Also, there's no concept of "next patch" when you do git log on the
upstream tree and use different sorting etc. So a patch should be
self-contained and do one change only.
There's very good documentation in Documentation/process/, expecially
Documentation/process/submitting-patches.rst, which explains how a patch
should look like.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/27/21 6:41 AM, Borislav Petkov wrote:
> On Tue, Oct 19, 2021 at 06:36:39PM -0500, Smita Koralahalli wrote:
>> Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().
> And this is where your commit message and patch should end. It is a bad
> idea to do textual replacements *and* functional changes in a single
> patch: it is hard to review and debug if there are possible issues. So
> you do the textual replacements in the first one and then the functional
> changes in subsequent patches.
Okay I will break this down and send v3 as suggested.
>> Also, restructure the code to avoid multiple initializations for MCA
>> registers.
> What multiple initializations?
Multiple initialization here I mean: Initializing the MCA registers twice.
Prior to mca_msr_reg() replacement, the MCA registers were initialized
separately for SMCA and legacy processors. However, this is not required
after replacing with mca_msr_reg() as it does the job of returning the
proper MSR addresses.
Probably, my wording is more confusing here. Does this seem better?
"Do not initialize MCx_{STATUS, ADDR, MISC} separately for SMCA and
legacy processors as mca_msr_reg() returns the appropriate MSR addresses
for both."
I will split this into second patch.
>> SMCA machines define a different set of MSRs for MCA registers
>> and mca_msr_reg() returns the proper MSR address for SMCA and legacy
>> processors.
>>
>> Initialize MCA_MISC and MCA_SYND registers at the end after initializing
>> MCx_{STATUS, DESTAT} which is further explained in the next patch.
> And this should be *in* the next patch.
Okay, basically break it down into three. One for replacing, one for
cleaning up the multiple initialization of MCA registers and the last for
moving MCA_MISC and MCA_SYND to the end.
Will do it as suggested..
>
> Also, there's no concept of "next patch" when you do git log on the
> upstream tree and use different sorting etc. So a patch should be
> self-contained and do one change only.
>
> There's very good documentation in Documentation/process/, expecially
> Documentation/process/submitting-patches.rst, which explains how a patch
> should look like.
>
> Thx.
Ok will take a look at this again to correct my mistakes. Thanks for the
inputs.
Thanks,
Smita
On Wed, Oct 27, 2021 at 03:19:51PM -0500, Koralahalli Channabasappa, Smita wrote:
> Multiple initialization here I mean: Initializing the MCA registers twice.
> Prior to mca_msr_reg() replacement, the MCA registers were initialized
> separately for SMCA and legacy processors. However, this is not required
> after replacing with mca_msr_reg() as it does the job of returning the
> proper MSR addresses.
You mean, there was a simple if-else statement
if (SMCA)
prepare MSRs
else
prepare MSRs for !SMCA
which did the init for each type of system in one go.
But frankly, your change doesn't make it more readable but less - you
have a goto label now and another SMCA feature check at the end. Vs a
simple if-else which is trivial to read.
So I don't see any advantage in this change.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/28/21 3:53 AM, Borislav Petkov wrote:
> On Wed, Oct 27, 2021 at 03:19:51PM -0500, Koralahalli Channabasappa, Smita wrote:
>> Multiple initialization here I mean: Initializing the MCA registers twice.
>> Prior to mca_msr_reg() replacement, the MCA registers were initialized
>> separately for SMCA and legacy processors. However, this is not required
>> after replacing with mca_msr_reg() as it does the job of returning the
>> proper MSR addresses.
> You mean, there was a simple if-else statement
>
> if (SMCA)
>
> prepare MSRs
>
> else
>
> prepare MSRs for !SMCA
>
> which did the init for each type of system in one go.
>
> But frankly, your change doesn't make it more readable but less - you
> have a goto label now and another SMCA feature check at the end. Vs a
> simple if-else which is trivial to read.
>
> So I don't see any advantage in this change.
Okay. I will rework and remove this patch in my next series.
Thanks,
>