Re: [PATCH v7 1/5] x86/mce: Add wrapper for struct mce to export vendor specific info

["Naik, Avadhut" <avadnaik@amd.com>]

On 10/30/2024 08:32, Borislav Petkov wrote:
> On Tue, Oct 22, 2024 at 07:36:27PM +0000, Avadhut Naik wrote:
>> Currently, exporting new additional machine check error information
>> involves adding new fields for the same at the end of the struct mce.
>> This additional information can then be consumed through mcelog or
>> tracepoint.
>>
>> However, as new MSRs are being added (and will be added in the future)
>> by CPU vendors on their newer CPUs with additional machine check error
>> information to be exported, the size of struct mce will balloon on some
>> CPUs, unnecessarily, since those fields are vendor-specific. Moreover,
>> different CPU vendors may export the additional information in varying
>> sizes.
>>
>> The problem particularly intensifies since struct mce is exposed to
>> userspace as part of UAPI. It's bloating through vendor-specific data
>> should be avoided to limit the information being sent out to userspace.
>>
>> Add a new structure mce_hw_err to wrap the existing struct mce. The same
>> will prevent its ballooning since vendor-specifc data, if any, can now be
> 
> Unknown word [vendor-specifc] in commit message.
> 
> Please introduce a spellchecker into your patch creation workflow.
> 
Will fix this.

> Also:
> 
> The tip-tree preferred ordering of variable declarations at the
> beginning of a function is reverse fir tree order::
> 
> 	struct long_struct_name *descriptive_name;
> 	unsigned long foo, bar;
> 	unsigned int tmp;
> 	int ret;
> 
> The above is faster to parse than the reverse ordering::
> 
> 	int ret;
> 	unsigned int tmp;
> 	unsigned long foo, bar;
> 	struct long_struct_name *descriptive_name;
> 
> And even more so than random ordering::
> 
> 	unsigned long foo, bar;
> 	int ret;
> 	struct long_struct_name *descriptive_name;
> 	unsigned int tmp;
> 
> diff ontop of yours:
> 
> ---
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 3611366d56b7..28e28b69d84d 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1030,11 +1030,11 @@ static noinstr int mce_timed_out(u64 *t, const char *msg)
>   */
>  static void mce_reign(void)
>  {
> -	int cpu;
>  	struct mce_hw_err *err = NULL;
>  	struct mce *m = NULL;
>  	int global_worst = 0;
>  	char *msg = NULL;
> +	int cpu;
>  
>  	/*
>  	 * This CPU is the Monarch and the other CPUs have run
> @@ -1291,8 +1291,8 @@ __mc_scan_banks(struct mce_hw_err *err, struct pt_regs *regs,
>  {
>  	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
>  	struct mca_config *cfg = &mca_cfg;
> -	struct mce *m = &err->m;
>  	int severity, i, taint = 0;
> +	struct mce *m = &err->m;
>  
>  	for (i = 0; i < this_cpu_read(mce_num_banks); i++) {
>  		arch___clear_bit(i, toclear);
> @@ -1419,8 +1419,8 @@ static void kill_me_never(struct callback_head *cb)
>  
>  static void queue_task_work(struct mce_hw_err *err, char *msg, void (*func)(struct callback_head *))
>  {
> -	struct mce *m = &err->m;
>  	int count = ++current->mce_count;
> +	struct mce *m = &err->m;
>  
>  	/* First call, save all the details */
>  	if (count == 1) {
> diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
> index 504d89724ecd..d0be6dda0c14 100644
> --- a/arch/x86/kernel/cpu/mce/genpool.c
> +++ b/arch/x86/kernel/cpu/mce/genpool.c
> @@ -73,9 +73,9 @@ struct llist_node *mce_gen_pool_prepare_records(void)
>  
>  void mce_gen_pool_process(struct work_struct *__unused)
>  {
> -	struct mce *mce;
> -	struct llist_node *head;
>  	struct mce_evt_llist *node, *tmp;
> +	struct llist_node *head;
> +	struct mce *mce;
>  
>  	head = llist_del_all(&mce_event_llist);
>  	if (!head)
> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
> index c65a5c4e2f22..313fe682db33 100644
> --- a/arch/x86/kernel/cpu/mce/inject.c
> +++ b/arch/x86/kernel/cpu/mce/inject.c
> @@ -502,9 +502,9 @@ static void prepare_msrs(void *info)
>  
>  static void do_inject(void)
>  {
> +	unsigned int cpu = i_mce.extcpu;
>  	struct mce_hw_err err;
>  	u64 mcg_status = 0;
> -	unsigned int cpu = i_mce.extcpu;
>  	u8 b = i_mce.bank;
>  
>  	i_mce.tsc = rdtsc_ordered();
> 

Ack for the suggestions.

-- 
Thanks,
Avadhut Naik