Message-ID: <m1bp9jgyww.fsf@fess.ebiederm.org>
Date:	Tue, 03 Aug 2010 14:38:07 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Dave Airlie <airlied@...il.com>,
	Iranna D Ankad <iranna.ankad@...ibm.com>,
	Gary Hade <garyhade@...ibm.com>,
	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Thomas Renninger <trenn@...e.de>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: oops in ioapic_write_entry

Yinghai Lu <yinghai@...nel.org> writes:

> On 08/03/2010 04:08 AM, Eric W. Biederman wrote:
>> 
>> For the common case I think we still do the right thing, even now, for
>> these broken bios tables.  There is likely an uncommon case for which
>> something like your shared_legacy_irq deserves to be used, especially
>> at it preserves our well tested historical behavior.
>
> Dave, Irnna, Gary:
>
> can you check this patch on your systems?



>
> Thanks
>
> Yinghai
>
> [PATCH] x86: check if apic/pin is shared with legacy one
>
> fix system that external device that have io apic on apic0/pin(0-15)

Nacked-by: "Eric W. Biederman" <ebiederm@...ssion.com>

Your patch addresses what appears to be a theoretical issue, caused by
a BIOS bug.  So far you have not presented a credible scenario where
this would affect anything in real life except the user visible irq
number.

Will you please stop, think, and describe what is going on clearly
and how you expect this patch to affect anything, and please stop
selling this patch as the solution to all of the world's ills.  You
are being sloppy and wasting everyone's time.

This patch definitely does not solve Dave Airlie's problem.  That was
clearly caused by a (ACPI being disabled compared to the test case)
and a bad BIOS provided MP table with an invalid apic_id, and our
attempt to lookup the ioapic and failed.

This patch doesn't have much of a chance of solving the no VGA output
on boot problem.  It comes much much to late affect anything there.


> also
> for the io apic out of order system:
> <6>ACPI: IOAPIC (id[0x10] address[0xfecff000] gsi_base[0])
> <6>IOAPIC[0]: apic_id 16, version 0, address 0xfecff000, GSI 0-2
> <6>ACPI: IOAPIC (id[0x0f] address[0xfec00000] gsi_base[3])
> <6>IOAPIC[1]: apic_id 15, version 0, address 0xfec00000, GSI 3-38
> <6>ACPI: IOAPIC (id[0x0e] address[0xfec01000] gsi_base[39])
> <6>IOAPIC[2]: apic_id 14, version 0, address 0xfec01000, GSI 39-74
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 1 global_irq 4 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 5 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 3 global_irq 6 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 4 global_irq 7 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 6 global_irq 9 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 7 global_irq 10 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 8 global_irq 11 low edge)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 12 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 12 global_irq 15 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 13 global_irq 16 dfl dfl)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 14 global_irq 17 low edge)
> <6>ACPI: INT_SRC_OVR (bus 0 bus_irq 15 global_irq 18 dfl dfl)
>
> after this patch will get
>
> apic0, pin0, GSI 0: irq 0+75
> apic0, pin1, GSI 1: irq 1+75
> apic0, pin2, GSI 2: irq 2
> apic1, pin0, GSI 3: irq 3+75
> apic1, pin5, GSI 8: irq 8+75
> apic1, pin10,GSI 13: irq 13+75
> apic1, pin11,GSI 14: irq 14+75

If your description of what this patch will do is correct we very
definitely do not want this patch.  For the previously mentioned
acpi tables we want GSI17 -> apic1, pin 14 -> irq 14.

Not that your description is correct, but a buggy description is an
equally valid reason to reject this incarnation of the patch.

> because mp_config_acpi_legacy_irqs will put apic0, pin2, irq2 in mp_irqs...
> so pin_2_irq_legacy will report 2.
> irq_to_gsi will still report 2. so it is right.
> gsi_to_irq will report 2.
>
> for GSI 0, 1, 3, 8, 13, 14: still right as before.

YH you have a point that a certain class of buggy firmware exists,
that reports some ioapic pins as both edge triggered ISA irqs
and level triggered PCI irqs, and assigning different irqs to the
same ioapic pin is confusing and suboptimal.

In practice I don't see that behavior affecting how we program that
pin, especially since I don't expect any bios that exports that setup
to actually use the ISA irq.  So we should just have an unused irq
number.

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.

Eric


> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
>
> ---
>  arch/x86/kernel/apic/io_apic.c |   31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
> +++ linux-2.6/arch/x86/kernel/apic/io_apic.c
> @@ -1013,6 +1013,28 @@ static inline int irq_trigger(int idx)
>  	return MPBIOS_trigger(idx);
>  }
>  
> +static int pin_2_irq_leagcy(int apic, int pin)
> +{
> +	int i;
> +
> +	for (i = 0; i < mp_irq_entries; i++) {
> +		int bus = mp_irqs[i].srcbus;
> +
> +		if (!test_bit(bus, mp_bus_not_pci))
> +			continue;
> +
> +		if (mp_ioapics[apic].apicid != mp_irqs[i].dstapic)
> +			continue;
> +
> +		if (mp_irqs[i].dstirq != pin)
> +			continue;
> +
> +		return mp_irqs[i].srcbusirq;
> +	}
> +
> +	return -1;
> +}
> +
>  static int pin_2_irq(int idx, int apic, int pin)
>  {
>  	int irq;
> @@ -1029,10 +1051,13 @@ static int pin_2_irq(int idx, int apic,
>  	} else {
>  		u32 gsi = mp_gsi_routing[apic].gsi_base + pin;
>  
> -		if (gsi >= NR_IRQS_LEGACY)
> +		if (gsi >= NR_IRQS_LEGACY) {
>  			irq = gsi;
> -		else
> -			irq = gsi_top + gsi;
> +		} else {
> +			irq = pin_2_irq_legacy(apic, pin);
> +			if (irq < 0)
> +				irq = gsi_top + gsi;
> +		}
>  	}
>  
>  #ifdef CONFIG_X86_32
--
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