lkml.org 
[lkml]   [2009]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [BUGFIX -v2] x86, mce, inject: Make injected mce valid only during faked handler call
Huang Ying wrote:
>>> mce.finished is enabled only during faked MCE handler call and
>>> protected by IRQ disabling. This make it impossible for real
>>> machine_check_poll to consume it.
>> Are there the reverse case - is it possible that the faked handler
>> call might consume real error which is not handled yet by the real
>> machine_check_poll?
>
> Yes. It's possible at least in theory. But whole mce-inject.c is used
> for testing only. The faked handler call will not occur on real system.

Just I concerned that it may confuse the mce test suite.

>>> +#define MCJ_LOADED (1 << MCJ_LOADED_BIT)
>> I'd like to see a patch to replace MCJ_* to MCE_INJ_* before
>> adding new flag.
>
> MCX_ prefix is the naming convention used all over the mce.h, such as
> MCG_, MCI_, MCM_, if we want to change MCJ_ into MCE_INJ_, we should
> consider changing all these into similar style to keep consistent.

That is bad naming convention, isn't it?
I don't mind considering changing all those.

>> Why
>> clear_bit(MCJ_LOADED_BIT, (unsigned long *)&m->inject_flags);
>> set_bit(MCJ_LOADED_BIT, (unsigned long *)&i->inject_flags);
>> cannot be
>> m->inject_flags &= ~MCJ_LOADED;
>> m->inject_flags |= MCJ_LOADED;
>> ?
>
> Because they may be write on one CPU and read on another CPU, atomic
> operation is safer for this.

I think such read should not happen while write is on flight.
We already have many barriers all around.

>> I think the "finished" is not good name. (I suppose it is named
>> after "loading data to structure have been finished" or so.)
>
> No. Its name is not invented for injecting. It stands for the MCE record
> writing to mce log buffer has finished. That is, it is named according
> to normal path, not testing path.

I know it.
I just point that there is a bad name since early times.

>> I believe what you want to do here is "make mce_rdmsrl()/mce_wrmsrl()
>> to refer faked data only during faked handler call."
>> Then what we have to do is making a flag to indicate that "now
>> in faked handler call," for an example:
>>
>> 309 if (__get_cpu_var(mce_fake_in_progress)) {
>>
>> and:
>> local_irq_save(flags);
>> __get_cpu_var(mce_fake_in_progress) = 1;
>> machine_check_poll(0, &b);
>> __get_cpu_var(mce_fake_in_progress) = 0;
>> local_irq_restore(flags);
>
> I don't think this method is better than the original one. They are just
> equivalent.

No, you changed usage of .finished, and transfer the functionality of the
flag to newly introduced MCJ_LOADED.
We can keep .finished as is, and introduce one new flag for this.

>>> static void raise_exception(struct mce *m, struct pt_regs *pregs)
>>> @@ -69,9 +71,11 @@ static void raise_exception(struct mce *
>>> }
>>> /* in mcheck exeception handler, irq will be disabled */
>>> local_irq_save(flags);
>>> + m->finished = 1;
>>> do_machine_check(pregs, 0);
>>> - local_irq_restore(flags);
>>> m->finished = 0;
>>> + clear_bit(MCJ_LOADED_BIT, (unsigned long *)&m->inject_flags);
>>> + local_irq_restore(flags);
>>> }
>>>
>>> static cpumask_t mce_inject_cpumask;
>>> @@ -89,6 +93,8 @@ static int mce_raise_notify(struct notif
>>> raise_exception(m, args->regs);
>>> else if (m->status)
>>> raise_poll(m);
>>> + else
>>> + clear_bit(MCJ_LOADED_BIT, (unsigned long *)&m->inject_flags);
>>> return NOTIFY_STOP;
>>> }
>>>
>>> @@ -129,7 +135,7 @@ static int raise_local(void)
>>> mce_notify_irq();
>>> printk(KERN_INFO "Machine check poll done on CPU %d\n", cpu);
>>> } else
>>> - m->finished = 0;
>>> + clear_bit(MCJ_LOADED_BIT, (unsigned long *)&m->inject_flags);
>>>
>>> return ret;
>>> }
>>> @@ -152,10 +158,13 @@ static void raise_mce(struct mce *m)
>>> cpu_clear(get_cpu(), mce_inject_cpumask);
>>> for_each_online_cpu(cpu) {
>>> struct mce *mcpu = &per_cpu(injectm, cpu);
>>> - if (!mcpu->finished ||
>>> + if (!test_bit(MCJ_LOADED_BIT,
>>> + (unsigned long *)&mcpu->inject_flags) ||
>>> MCJ_CTX(mcpu->inject_flags) != MCJ_CTX_RANDOM)
>>> cpu_clear(cpu, mce_inject_cpumask);
>>> }
>>> + /* make sure needed data is available on other CPUs */
>>> + smp_mb();
>> What data are you taking care here for?
>
> For mce_inject_cpumask.

OK, it seems fair enough.
I'd like to see this change in a separate patch with proper description.


Thanks,
H.Seto




\
 
 \ /
  Last update: 2009-09-28 10:05    [W:1.838 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site