Re: oops in ioapic_write_entry

[ebiederm@xmission.com (Eric W. Biederman)]

Yinghai Lu <yinghai@kernel.org> writes:

> On 08/03/2010 02:38 PM, Eric W. Biederman wrote:
>> 
>> A clean solution would be to scrub the input from the MP table before
>> we attempt to use of it, instead of scrubbing the data as we use in
>> code paths like pin_2_irq.  Just touching pin_2_irq is certainly an
>> incomplete solution because you have not resolved if the pins should
>> be edge or level triggered, and what polarity we should be sampling
>> them at.
>> 
>> Until I see a plausible scenario where not handling buggy MP tables
>> exactly as we have done in the past I don't see hacks like you
>> are proposing making much sense at all.
>
> ok, how about this one?
>
> it will try to add entries to mp_irqs[] with some checking.

This looks roughly like the right approach.
However I don't see what would force the ioapic entries before
the intsrc entries in the table.  Especially when the assumption
is we are dealing with a buggy bios.

> ---
>  arch/x86/kernel/mpparse.c |   44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
>
> Index: linux-2.6/arch/x86/kernel/mpparse.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/mpparse.c
> +++ linux-2.6/arch/x86/kernel/mpparse.c
> @@ -173,17 +173,17 @@ static int __init mp_irq_mpc_intsrc_cmp(
>  {
>  	if (mp_irq->dstapic != m->dstapic)
>  		return 1;
> -	if (mp_irq->type != m->type)
> +	if (mp_irq->dstirq != m->dstirq)
>  		return 2;
> -	if (mp_irq->irqtype != m->irqtype)
> +	if (mp_irq->srcbus != m->srcbus)
>  		return 3;
> -	if (mp_irq->irqflag != m->irqflag)
> +	if (mp_irq->type != m->type)
>  		return 4;
> -	if (mp_irq->srcbus != m->srcbus)
> +	if (mp_irq->irqtype != m->irqtype)
>  		return 5;
> -	if (mp_irq->srcbusirq != m->srcbusirq)
> +	if (mp_irq->irqflag != m->irqflag)
>  		return 6;
> -	if (mp_irq->dstirq != m->dstirq)
> +	if (mp_irq->srcbusirq != m->srcbusirq)
>  		return 7;
>  
>  	return 0;
> @@ -195,9 +195,39 @@ static void __init MP_intsrc_info(struct
>  
>  	print_MP_intsrc_info(m);
>  
> +	/*
> +	 *  Assume BUS, and IOAPIC entries come first all before
> +	 *    INTSRC entries
> +	 */
> +
> +	/* check if dstapic is right */
> +	for (i = 0; i < nr_ioapics; i++) {
> +		if (mp_ioapics[idx].apicid == m->dstapic)
> +			break;
> +	}
> +	if (i == nr_ioapics)
> +		return;

If we can make the algorithm two pass picking up the ioapic
entries first this should be fine.

>  	for (i = 0; i < mp_irq_entries; i++) {
> -		if (!mp_irq_mpc_intsrc_cmp(&mp_irqs[i], m))
> +		int ret = mp_irq_mpc_intsrc_cmp(&mp_irqs[i], m);
> +
> +		/* duplicated entries ? */
> +		if (!ret)
>  			return;
> +
> +		/* same apic/pin, but different bus */
> +		if (ret == 3) {
> +			/* overwrite wrong legacy one */

In practice your test of looking at mp_bus_not_pci is essentially what
we do.  I wonder if it could be made to be a test of polarity and edge
mismatch instead.

Also if we are ditching a non-duplicate intsrc we want to print a message
so there is a chance of debugging things if our heuristic for fixing
up broken BIOS's goes wrong, on some common configuration.

> +			if (test_bit(mp_irqs[i].srcbus, mp_bus_not_pci) &&
> +			    !test_bit(m->srcbus, mp_bus_not_pci)) {
> +				assign_to_mp_irq(m, &mp_irqs[mp_irq_entries]);

That should be.
> +				assign_to_mp_irq(m, &mp_irqs[i]);

> +				return;
> +			}
> +			/* dump this legacy one */
> +			if (!test_bit(mp_irqs[i].srcbus, mp_bus_not_pci) &&
> +			    test_bit(m->srcbus, mp_bus_not_pci))
> +				return;
> +		}
>  	}
>  
>  	assign_to_mp_irq(m, &mp_irqs[mp_irq_entries]);

Eric