| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 14 Jun 2011 10:27:18 +0900
From: Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>
To: Tony Luck <tony.luck@...el.com>
CC: Ingo Molnar <mingo@...e.hu>, Borislav Petkov <bp@...64.org>,
linux-kernel@...r.kernel.org, "Huang, Ying" <ying.huang@...el.com>,
Avi Kivity <avi@...hat.com>
Subject: Re: [PATCH 09/10] MCE: run through processors with more severe problems
first
(2011/06/14 7:03), Tony Luck wrote:
>>> Or how about checking rip in each mces_seen?
>>
>> This is equivalent to what I did - but I think the code
>> will be cleaner. I'll give it a try.
>
> Here's a patch on top of my previous series that just looks at
> mces_seen to choose the order. Obviously I'd fold this into the
> other patch for a final version - but this one lets you see what
> the "mce_nextcpu()" function would look like (and how removing
> the bitmaps cleans up the other parts of the code). It does look
> better to me.
>
> Seto-san: Does this fit with what you were thinking?
>
> Compile tested only.
>
> -Tony
>
> ---
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index a7a8c53..6b4176b 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -791,31 +791,47 @@ static void mce_reign(void)
>
> static atomic_t global_nwo;
>
> -/*
> - * Keep separate bitmaps for cpus that have the option return from
> - * machine check handler (MCG_STATUS.RIPV == 1) and those for that
> - * cannot.
> - */
> -static cpumask_t can_return;
> -static cpumask_t cant_return;
> -
> static int monarch;
>
> /*
> - * next cpu choosing first from cant_return, and then from can_return
> + * Find next cpu that will run through the core of do_machine_check()
> + * checking all the banks of machine check registers. We first take
> + * cpus with serious problems (as indicated by MCG_STATUS_RIPV being
> + * clear in the mcgstatus register). A second pass through mces_seen
> + * is made to process the remaining cpus.
> + * We do this because some machine check banks are shared between cpus,
> + * and it is better to find the error on the cpu that has the problem
> + * and clear the bank so that the innocent bystanders do not have to
> + * worry about errors that do not affect them.
BTW in case of "no_way_out" events, we don't clear banks because they
could be carried over to the next boot (expecting logged as bootlog).
So we may need to have some trick for some known cases; e.g. ignore
observed AR by bystanders, anyway.
> */
> -int mce_nextcpu(int this)
> +int mce_nextcpu(int cur)
> {
> - int next;
> + struct mce *m;
> + int cpu = cur;
> + u64 mask = MCG_STATUS_MCIP;
Why do you check the MCG_STATUS_MCIP too here?
What happens if there is a problematic cpu that could not read
MCG register properly so indicates "PANIC with !MCIP"?
>
> - if (this == -1 || cpumask_test_cpu(this, &cant_return)) {
> - next = cpumask_next(this, &cant_return);
> - if (next >= nr_cpu_ids)
> - next = cpumask_next(-1, &can_return);
> - return next;
> + if (cpu != -1) {
> + m = &per_cpu(mces_seen, cpu);
> + if (m->mcgstatus & MCG_STATUS_RIPV)
> + mask |= MCG_STATUS_RIPV;
> }
>
> - return cpumask_next(this, &can_return);
> + while (1) {
> + cpu = cpumask_next(cpu, cpu_possible_mask);
possible? online?
Ah, I guess you assumed that all cpus checked in should have
mces_seen with MCIP while offline cpus have cleaned mces_seen.
Though we know there might be races with cpu hotplug, now we
already use num_online_cpus() in this rendezvous mechanism,
it is OK to use cpu_online_mask here at the moment, I think.
Or we should invent new, better rendezvous mechanism...
> + if (cpu >= nr_cpu_ids) {
> + if (mask & MCG_STATUS_RIPV)
> + return cpu;
> + mask |= MCG_STATUS_RIPV;
> + cpu = -1;
> + continue;
> + }
> +
> + m = &per_cpu(mces_seen, cpu);
> + if ((m->mcgstatus & (MCG_STATUS_MCIP|MCG_STATUS_RIPV)) == mask)
> + break;
> + }
> +
> + return cpu;
> }
>
> /*
> @@ -825,7 +841,7 @@ int mce_nextcpu(int this)
> * one at a time.
> * TBD double check parallel CPU hotunplug
> */
> -static int mce_start(int *no_way_out, int noreturn)
> +static int mce_start(int *no_way_out)
> {
> int order;
> int cpus = num_online_cpus();
> @@ -841,11 +857,6 @@ static int mce_start(int *no_way_out, int noreturn)
> smp_wmb();
> order = atomic_inc_return(&mce_callin);
>
> - if (noreturn)
> - cpumask_set_cpu(smp_processor_id(), &cant_return);
> - else
> - cpumask_set_cpu(smp_processor_id(), &can_return);
> -
> /*
> * Wait for everyone.
> */
> @@ -951,8 +962,6 @@ static int mce_end(int order)
> reset:
> atomic_set(&global_nwo, 0);
> atomic_set(&mce_callin, 0);
> - cpumask_clear(&can_return);
> - cpumask_clear(&cant_return);
> barrier();
>
> /*
> @@ -1134,7 +1143,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
> * This way we don't report duplicated events on shared banks
> * because the first one to see it will clear it.
> */
> - order = mce_start(&no_way_out, kill_it);
> + order = mce_start(&no_way_out);
> for (i = 0; i < banks; i++) {
> __clear_bit(i, toclear);
> if (!mce_banks[i].ctl)
>
>
The rest looks good.
Thanks,
H.Seto
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists