Re: [PATCH v5 2/2] x86/mce: Add support for Extended Physical Address MCA changes

[Borislav Petkov]

On Tue, Apr 12, 2022 at 10:40:38AM -0500, Smita Koralahalli wrote:
> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index f809eacac523..4f2744324d9b 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -722,6 +722,17 @@ bool amd_mce_is_memory_error(struct mce *m)
>  	return m->bank == 4 && xec == 0x8;
>  }
>  
> +void smca_feature_init(void)
> +{
> +	unsigned int bank;
> +	u64 mca_cfg;
> +
> +	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
> +		rdmsrl(MSR_AMD64_SMCA_MCx_CONFIG(bank), mca_cfg);
> +		this_cpu_ptr(mce_banks_array)[bank].lsb_in_status = !!(mca_cfg & BIT(8));
> +	}
> +}

We have smca_configure() for SMCA banks init and there it even reads
MCx_CONFIG.

Do you guys not see this?

Or integrating new stuff into the existing code doesn't really matter -
just bolt it on wherever it works?!

> diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
> index 64dbae6b8a09..0f4934fb3d93 100644
> --- a/arch/x86/kernel/cpu/mce/internal.h
> +++ b/arch/x86/kernel/cpu/mce/internal.h
> @@ -177,6 +177,22 @@ struct mce_vendor_flags {
>  
>  extern struct mce_vendor_flags mce_flags;
>  
> +struct mce_bank {
> +	u64			ctl;			/* subevents to enable */
> +
> +	__u64 init			: 1,		/* initialise bank? */
> +
> +	/*
> +	 * (AMD) MCA_CONFIG[McaLsbInStatusSupported]: This bit indicates
> +	 * the LSB field is found in MCA_STATUS, when set.
> +	 */
> +	      lsb_in_status		: 1,
> +
> +	      __reserved_1		: 62;
> +};

Put those comments over the members, while at it:

diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 0f4934fb3d93..770a31120fd2 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -178,17 +178,18 @@ struct mce_vendor_flags {
 extern struct mce_vendor_flags mce_flags;
 
 struct mce_bank {
-       u64                     ctl;                    /* subevents to enable */
+       /* subevents to enable */
+       u64                     ctl;
 
-       __u64 init                      : 1,            /* initialise bank? */
+       /* initialise bank? */
+       __u64 init              : 1,
 
        /*
         * (AMD) MCA_CONFIG[McaLsbInStatusSupported]: This bit indicates
         * the LSB field is found in MCA_STATUS, when set.
         */
-             lsb_in_status             : 1,
-
-             __reserved_1              : 62;
+             lsb_in_status     : 1,
+             __reserved_1      : 62;
 };

> +DECLARE_PER_CPU_READ_MOSTLY(struct mce_bank[MAX_NR_BANKS], mce_banks_array);
> +
>  enum mca_msr {
>  	MCA_CTL,
>  	MCA_STATUS,
> @@ -190,7 +206,9 @@ extern bool filter_mce(struct mce *m);
>  #ifdef CONFIG_X86_MCE_AMD
>  extern bool amd_filter_mce(struct mce *m);
>  
> -/* Extract [55:<lsb>] where lsb is the LS-*valid* bit of the address bits. */
> +/* If MCA_CONFIG[McaLsbInStatusSupported] is set, extract ErrAddr in bits
> + * [56:0], else in bits [55:0] of MCA_ADDR.
> + */

verify_comment_style: Warning: Multi-line comment needs to start text on the second line:
 [+/* If MCA_CONFIG[McaLsbInStatusSupported] is set, extract ErrAddr in bits]

Documentation/process/maintainer-tip.rst, Section "Comment style".

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette