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
