| Message ID | 20211019233641.140275-4-Smita.KoralahalliChannabasappa@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mce: Handle error injection failures in mce-inject module | expand |
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.
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.
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, >
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)
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 <Smita.KoralahalliChannabasappa@amd.com> --- 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(-)