RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Tuesday, April 04, 2017 11:01 AM
> To: Ghannam, Yazen <yazen.ghan...@amd.com>
> Cc: linux-e...@vger.kernel.org; Tony Luck <tony.l...@intel.com>;
> x...@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Tue, Apr 04, 2017 at 02:37:34PM +, Ghannam, Yazen wrote:
> > No, not at the moment. This patch is just to help with readability.
> 
> Then no need to have two separate functions just because stuff is logically
> independent. Add a comment over the SMCA configuration part in case we
> want to carve it out later.
> 

Okay, will do.

Thanks,
Yazen

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Tuesday, April 04, 2017 11:01 AM
> To: Ghannam, Yazen 
> Cc: linux-e...@vger.kernel.org; Tony Luck ;
> x...@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Tue, Apr 04, 2017 at 02:37:34PM +, Ghannam, Yazen wrote:
> > No, not at the moment. This patch is just to help with readability.
> 
> Then no need to have two separate functions just because stuff is logically
> independent. Add a comment over the SMCA configuration part in case we
> want to carve it out later.
> 

Okay, will do.

Thanks,
Yazen

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Tue, Apr 04, 2017 at 02:37:34PM +, Ghannam, Yazen wrote:
> No, not at the moment. This patch is just to help with readability.

Then no need to have two separate functions just because stuff is
logically independent. Add a comment over the SMCA configuration part in
case we want to carve it out later.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Tue, Apr 04, 2017 at 02:37:34PM +, Ghannam, Yazen wrote:
> No, not at the moment. This patch is just to help with readability.

Then no need to have two separate functions just because stuff is
logically independent. Add a comment over the SMCA configuration part in
case we want to carve it out later.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Tuesday, April 04, 2017 9:46 AM
> To: Ghannam, Yazen <yazen.ghan...@amd.com>
> Cc: linux-e...@vger.kernel.org; Tony Luck <tony.l...@intel.com>;
> x...@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Tue, Apr 04, 2017 at 01:34:42PM +, Ghannam, Yazen wrote:
> > I'd like to keep the functions separate since they're logically
> > independent. I can define something like smca_configure() as a wrapper
> > function that can contain current and future SMCA related functions. Is this
> okay?
> 
> Are you planning to call the one and not the other in some path?

No, not at the moment. This patch is just to help with readability.

Thanks,
Yazen

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Tuesday, April 04, 2017 9:46 AM
> To: Ghannam, Yazen 
> Cc: linux-e...@vger.kernel.org; Tony Luck ;
> x...@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Tue, Apr 04, 2017 at 01:34:42PM +, Ghannam, Yazen wrote:
> > I'd like to keep the functions separate since they're logically
> > independent. I can define something like smca_configure() as a wrapper
> > function that can contain current and future SMCA related functions. Is this
> okay?
> 
> Are you planning to call the one and not the other in some path?

No, not at the moment. This patch is just to help with readability.

Thanks,
Yazen

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Tue, Apr 04, 2017 at 01:34:42PM +, Ghannam, Yazen wrote:
> I'd like to keep the functions separate since they're logically independent. 
> I can
> define something like smca_configure() as a wrapper function that can contain
> current and future SMCA related functions. Is this okay?

Are you planning to call the one and not the other in some path?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Tue, Apr 04, 2017 at 01:34:42PM +, Ghannam, Yazen wrote:
> I'd like to keep the functions separate since they're logically independent. 
> I can
> define something like smca_configure() as a wrapper function that can contain
> current and future SMCA related functions. Is this okay?

Are you planning to call the one and not the other in some path?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -Original Message-
> From: Borislav Petkov [mailto:b...@suse.de]
> Sent: Tuesday, March 28, 2017 3:23 PM
> To: Ghannam, Yazen <yazen.ghan...@amd.com>
> Cc: linux-e...@vger.kernel.org; Tony Luck <tony.l...@intel.com>;
> x...@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Wed, Mar 22, 2017 at 02:29:31PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghan...@amd.com>
> >
> > Scalable MCA systems have a new MCA_CONFIG register that we use to
> > configure each bank. We currently use this when we set up thresholding.
> > However, this is logically separate.
> >
> > Move setup of MCA_CONFIG into a separate function.
> >
> > Signed-off-by: Yazen Ghannam <yazen.ghan...@amd.com>
> > ---
> >  arch/x86/kernel/cpu/mcheck/mce_amd.c | 48
> > 
> >  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> ...
> 
> >  /* cpu init entry point, called from mce.c with preempt off */ @@
> > -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> > int offset = -1;
> >
> > for (bank = 0; bank < mca_cfg.banks; ++bank) {
> > -   if (mce_flags.smca)
> > +   if (mce_flags.smca) {
> > get_smca_bank_info(bank);
> > +   set_smca_config(bank);
> 
> Or simply bundle those two which do something SMCA-aware per bank into a
> single:
> 
>   smca_configure(bank);
> 
> which reads almost like a sentence.
> 

I'd like to keep the functions separate since they're logically independent. I 
can
define something like smca_configure() as a wrapper function that can contain
current and future SMCA related functions. Is this okay?

Thanks,
Yazen

RE: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Ghannam, Yazen]

> -Original Message-
> From: Borislav Petkov [mailto:b...@suse.de]
> Sent: Tuesday, March 28, 2017 3:23 PM
> To: Ghannam, Yazen 
> Cc: linux-e...@vger.kernel.org; Tony Luck ;
> x...@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration
> 
> On Wed, Mar 22, 2017 at 02:29:31PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam 
> >
> > Scalable MCA systems have a new MCA_CONFIG register that we use to
> > configure each bank. We currently use this when we set up thresholding.
> > However, this is logically separate.
> >
> > Move setup of MCA_CONFIG into a separate function.
> >
> > Signed-off-by: Yazen Ghannam 
> > ---
> >  arch/x86/kernel/cpu/mcheck/mce_amd.c | 48
> > 
> >  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> ...
> 
> >  /* cpu init entry point, called from mce.c with preempt off */ @@
> > -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> > int offset = -1;
> >
> > for (bank = 0; bank < mca_cfg.banks; ++bank) {
> > -   if (mce_flags.smca)
> > +   if (mce_flags.smca) {
> > get_smca_bank_info(bank);
> > +   set_smca_config(bank);
> 
> Or simply bundle those two which do something SMCA-aware per bank into a
> single:
> 
>   smca_configure(bank);
> 
> which reads almost like a sentence.
> 

I'd like to keep the functions separate since they're logically independent. I 
can
define something like smca_configure() as a wrapper function that can contain
current and future SMCA related functions. Is this okay?

Thanks,
Yazen

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Wed, Mar 22, 2017 at 02:29:31PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam 
> 
> Scalable MCA systems have a new MCA_CONFIG register that we use to
> configure each bank. We currently use this when we set up thresholding.
> However, this is logically separate.
> 
> Move setup of MCA_CONFIG into a separate function.
> 
> Signed-off-by: Yazen Ghannam 
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 48 
> 
>  1 file changed, 27 insertions(+), 21 deletions(-)

...

>  /* cpu init entry point, called from mce.c with preempt off */
> @@ -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>   int offset = -1;
>  
>   for (bank = 0; bank < mca_cfg.banks; ++bank) {
> - if (mce_flags.smca)
> + if (mce_flags.smca) {
>   get_smca_bank_info(bank);
> + set_smca_config(bank);

Or simply bundle those two which do something SMCA-aware per bank into a single:

smca_configure(bank);

which reads almost like a sentence.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Borislav Petkov]

On Wed, Mar 22, 2017 at 02:29:31PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam 
> 
> Scalable MCA systems have a new MCA_CONFIG register that we use to
> configure each bank. We currently use this when we set up thresholding.
> However, this is logically separate.
> 
> Move setup of MCA_CONFIG into a separate function.
> 
> Signed-off-by: Yazen Ghannam 
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 48 
> 
>  1 file changed, 27 insertions(+), 21 deletions(-)

...

>  /* cpu init entry point, called from mce.c with preempt off */
> @@ -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>   int offset = -1;
>  
>   for (bank = 0; bank < mca_cfg.banks; ++bank) {
> - if (mce_flags.smca)
> + if (mce_flags.smca) {
>   get_smca_bank_info(bank);
> + set_smca_config(bank);

Or simply bundle those two which do something SMCA-aware per bank into a single:

smca_configure(bank);

which reads almost like a sentence.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

[PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Yazen Ghannam]

From: Yazen Ghannam 

Scalable MCA systems have a new MCA_CONFIG register that we use to
configure each bank. We currently use this when we set up thresholding.
However, this is logically separate.

Move setup of MCA_CONFIG into a separate function.

Signed-off-by: Yazen Ghannam 
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 48 
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c 
b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 4e459e0..95870b3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -433,7 +433,7 @@ prepare_threshold_block(unsigned int bank, unsigned int 
block, u32 addr,
int offset, u32 misc_high)
 {
unsigned int cpu = smp_processor_id();
-   u32 smca_low, smca_high, smca_addr;
+   u32 smca_low, smca_high;
struct threshold_block b;
int new;
 
@@ -457,7 +457,29 @@ prepare_threshold_block(unsigned int bank, unsigned int 
block, u32 addr,
goto set_offset;
}
 
-   smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
+   /* Gather LVT offset for thresholding: */
+   if (rdmsr_safe(MSR_CU_DEF_ERR, _low, _high))
+   goto out;
+
+   new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
+
+set_offset:
+   offset = setup_APIC_mce_threshold(offset, new);
+
+   if ((offset == new) && (mce_threshold_vector != 
amd_threshold_interrupt))
+   mce_threshold_vector = amd_threshold_interrupt;
+
+done:
+   mce_threshold_block_init(, offset);
+
+out:
+   return offset;
+}
+
+static void set_smca_config(unsigned int bank)
+{
+   u32 smca_low, smca_high;
+   u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
 
if (!rdmsr_safe(smca_addr, _low, _high)) {
/*
@@ -487,24 +509,6 @@ prepare_threshold_block(unsigned int bank, unsigned int 
block, u32 addr,
 
wrmsr(smca_addr, smca_low, smca_high);
}
-
-   /* Gather LVT offset for thresholding: */
-   if (rdmsr_safe(MSR_CU_DEF_ERR, _low, _high))
-   goto out;
-
-   new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
-
-set_offset:
-   offset = setup_APIC_mce_threshold(offset, new);
-
-   if ((offset == new) && (mce_threshold_vector != 
amd_threshold_interrupt))
-   mce_threshold_vector = amd_threshold_interrupt;
-
-done:
-   mce_threshold_block_init(, offset);
-
-out:
-   return offset;
 }
 
 /* cpu init entry point, called from mce.c with preempt off */
@@ -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
int offset = -1;
 
for (bank = 0; bank < mca_cfg.banks; ++bank) {
-   if (mce_flags.smca)
+   if (mce_flags.smca) {
get_smca_bank_info(bank);
+   set_smca_config(bank);
+   }
 
for (block = 0; block < NR_BLOCKS; ++block) {
address = get_block_address(cpu, address, low, high, 
bank, block);
-- 
2.7.4

[PATCH 2/2] x86/mce/AMD: Carve out SMCA bank configuration

[Yazen Ghannam]

From: Yazen Ghannam 

Scalable MCA systems have a new MCA_CONFIG register that we use to
configure each bank. We currently use this when we set up thresholding.
However, this is logically separate.

Move setup of MCA_CONFIG into a separate function.

Signed-off-by: Yazen Ghannam 
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 48 
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c 
b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 4e459e0..95870b3 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -433,7 +433,7 @@ prepare_threshold_block(unsigned int bank, unsigned int 
block, u32 addr,
int offset, u32 misc_high)
 {
unsigned int cpu = smp_processor_id();
-   u32 smca_low, smca_high, smca_addr;
+   u32 smca_low, smca_high;
struct threshold_block b;
int new;
 
@@ -457,7 +457,29 @@ prepare_threshold_block(unsigned int bank, unsigned int 
block, u32 addr,
goto set_offset;
}
 
-   smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
+   /* Gather LVT offset for thresholding: */
+   if (rdmsr_safe(MSR_CU_DEF_ERR, _low, _high))
+   goto out;
+
+   new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
+
+set_offset:
+   offset = setup_APIC_mce_threshold(offset, new);
+
+   if ((offset == new) && (mce_threshold_vector != 
amd_threshold_interrupt))
+   mce_threshold_vector = amd_threshold_interrupt;
+
+done:
+   mce_threshold_block_init(, offset);
+
+out:
+   return offset;
+}
+
+static void set_smca_config(unsigned int bank)
+{
+   u32 smca_low, smca_high;
+   u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(bank);
 
if (!rdmsr_safe(smca_addr, _low, _high)) {
/*
@@ -487,24 +509,6 @@ prepare_threshold_block(unsigned int bank, unsigned int 
block, u32 addr,
 
wrmsr(smca_addr, smca_low, smca_high);
}
-
-   /* Gather LVT offset for thresholding: */
-   if (rdmsr_safe(MSR_CU_DEF_ERR, _low, _high))
-   goto out;
-
-   new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
-
-set_offset:
-   offset = setup_APIC_mce_threshold(offset, new);
-
-   if ((offset == new) && (mce_threshold_vector != 
amd_threshold_interrupt))
-   mce_threshold_vector = amd_threshold_interrupt;
-
-done:
-   mce_threshold_block_init(, offset);
-
-out:
-   return offset;
 }
 
 /* cpu init entry point, called from mce.c with preempt off */
@@ -515,8 +519,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
int offset = -1;
 
for (bank = 0; bank < mca_cfg.banks; ++bank) {
-   if (mce_flags.smca)
+   if (mce_flags.smca) {
get_smca_bank_info(bank);
+   set_smca_config(bank);
+   }
 
for (block = 0; block < NR_BLOCKS; ++block) {
address = get_block_address(cpu, address, low, high, 
bank, block);
-- 
2.7.4