Messages in this thread | | | Date | Wed, 17 Jun 2009 08:57:20 +0200 | From | Ingo Molnar <> | Subject | Re: MCE boot crash in qemu |
| |
* Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> On Mon, 2009-06-15 at 16:22 +0300, Pekka Enberg wrote: > > On Mon, 2009-06-15 at 14:52 +0200, Andi Kleen wrote: > > > x86: mce: Handle banks == 0 case in K7 quirk > > > > > > This happens on QEMU which reports MCA capability, but no banks. > > > Without this patch there is a buffer overrun and boot ops because the code > > > would try to initialize the 0 element of a zero length kmalloc() > > > buffer. > > > > > > Signed-off-by: Andi Kleen <ak@linux.intel.com> > > > > This fixes the bug for me! > > > > Tested-by: Pekka Enberg <penberg@cs.helsinki.fi> > > Ingo, I hit this again in my testing after rebasing to > linus/master so I really would like this in mainline.
yep, i've tidied up the changelog and have committed it to x86/urgent.
But the bank[] code is quirky and butt-ugly and that needs to be cleaned up - it's no wonder that bugs like this slip in.
- There's zero description about the hw model it represents and how it relates to the bank[] array - what do the banks mean, how are they organized.
- It's full of magic constants and implicitly-assumed size calculations with little explanation and little extensibility:
... if (c->x86 == 15 && banks > 4) { /* * disable GART TBL walk error reporting, which * trips off incorrectly with the IOMMU & 3ware * & Cerberus: */ clear_bit(10, (unsigned long *)&bank[4]); } ... bank = kmalloc(banks * sizeof(u64), GFP_KERNEL); ... memset(bank, 0xff, banks * sizeof(u64)); ...
- There's lots of bitmaps, arrays, flags interacting, creating a maze of logic.
Instead of this messy code, the proper approach is to introduce an abstract data structure representing the attributes of an MCE bank register:
struct mce_bank_register { int enabled; int polled; int dont_init; int msr_idx; };
( There's lots of other structural problems with the MCE code too - but now that it's unified lets first fix the most obvious ones... )
Ingo
| |