Re: MCE boot crash in qemu

[Ingo Molnar <mingo@elte.hu>]

* 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