| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 22 Mar 2010 12:14:15 +0100 (CET)
From: Thomas Gleixner <tglx@...utronix.de>
To: Yinghai Lu <yinghai@...nel.org>
cc: Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
Andrew Morton <akpm@...ux-foundation.org>,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Jesse Barnes <jbarnes@...tuousgeek.org>,
linux-kernel@...r.kernel.org, Thomas Renninger <trenn@...e.de>,
Suresh Siddha <suresh.b.siddha@...el.com>, len.brown@...el.com
Subject: Re: [PATCH 02/10] x86: fix out of order of gsi - full
On Sun, 21 Mar 2010, Yinghai Lu wrote:
> +extern int gsi_delta;
Is this really necessary ? See below.
> +int gsi_to_irq(unsigned int gsi);
unsigned int please
> +unsigned int irq_to_gsi(int irq);
Ditto
> @@ -446,11 +448,11 @@ void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger)
>
> int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> {
> - *irq = gsi;
> + *irq = gsi_to_irq(gsi);
>
> #ifdef CONFIG_X86_IO_APIC
> if (acpi_irq_model == ACPI_IRQ_MODEL_IOAPIC)
> - setup_IO_APIC_irq_extra(gsi);
> + setup_IO_APIC_irq_extra(gsi, irq);
Please do not propagate that pointer.
*irq = setup_IO_APIC_irq_extra(gsi);
Also these changes have a total lack of comments. Why do we modify
*irq here depending on the setup_IO_APIC_irq_extra() results ?
> #endif
>
> return 0;
> @@ -914,6 +916,40 @@ static void save_mp_irq(struct mpc_intsrc *m)
> panic("Max # of irq sources exceeded!!\n");
> }
>
> +/* By default isa irqs are identity mapped to gsis */
> +static unsigned int isa_irq_to_gsi[NR_IRQS_LEGACY] = {
> + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
> +};
> +
> +int gsi_delta;
Please make this static and provide a function to modify it from
probe_ioapic_i8259().
Also the variable name is horrible. What's the delta here ? It's an
offset, right ?
> +int gsi_to_irq(unsigned int gsi)
unsigned int
> +{
> + unsigned int irq = gsi;
> + unsigned int i;
> +
> + irq += gsi_delta;
Please make that:
unsigned int i, irq = gsi + gsi_delta;
> + for (i = 0; i < NR_IRQS_LEGACY; i++) {
> + if (isa_irq_to_gsi[i] == gsi) {
> + irq = i;
> + break;
> + }
> + }
> +
> + return irq;
> +}
> +
> +unsigned int irq_to_gsi(int irq)
unsigned int please
> +{
> + unsigned int gsi;
> +
> + if (irq < NR_IRQS_LEGACY)
> + gsi = isa_irq_to_gsi[irq];
> + else
> + gsi = irq - gsi_delta;
> +
> + return gsi;
> +}
> +
> void __init mp_override_legacy_irq(u8 bus_irq, u8 polarity, u8 trigger, u32 gsi)
> {
> int ioapic;
> @@ -945,6 +981,8 @@ void __init mp_override_legacy_irq(u8 bus_irq, u8 polarity, u8 trigger, u32 gsi)
> mp_irq.dstirq = pin; /* INTIN# */
>
> save_mp_irq(&mp_irq);
> +
> + isa_irq_to_gsi[bus_irq] = gsi;
> }
>
> void __init mp_config_acpi_legacy_irqs(void)
> @@ -974,7 +1012,7 @@ void __init mp_config_acpi_legacy_irqs(void)
> /*
> * Locate the IOAPIC that manages the ISA IRQs (0-15).
> */
> - ioapic = mp_find_ioapic(0);
> + ioapic = mp_find_ioapic(irq_to_gsi(0));
> if (ioapic < 0)
> return;
> dstapic = mp_ioapics[ioapic].apicid;
> @@ -1057,6 +1095,7 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
Why is mp_register_gsi global ? It's only used in
arch/x86/kernel/acpi/boot.c
> {
> int ioapic;
> int ioapic_pin;
> + int irq;
irq is unsigned int.
> struct io_apic_irq_attr irq_attr;
>
> if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
> @@ -1079,11 +1118,12 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
> gsi = ioapic_renumber_irq(ioapic, gsi);
> #endif
>
> + irq = gsi_to_irq(gsi);
> if (ioapic_pin > MP_MAX_IOAPIC_PIN) {
> printk(KERN_ERR "Invalid reference to IOAPIC pin "
> "%d-%d\n", mp_ioapics[ioapic].apicid,
> ioapic_pin);
> - return gsi;
> + return irq;
> }
>
> if (enable_update_mptable)
> @@ -1092,9 +1132,9 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
> set_io_apic_irq_attr(&irq_attr, ioapic, ioapic_pin,
> trigger == ACPI_EDGE_SENSITIVE ? 0 : 1,
> polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
> - io_apic_set_pci_routing(dev, gsi, &irq_attr);
> + io_apic_set_pci_routing(dev, irq, &irq_attr);
>
> - return gsi;
> + return irq;
> }
>
> /*
> @@ -1151,8 +1191,10 @@ static int __init acpi_parse_madt_ioapic_entries(void)
> * If BIOS did not supply an INT_SRC_OVR for the SCI
> * pretend we got one so we can set the SCI flags.
> */
> - if (!acpi_sci_override_gsi)
> - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0);
> + if (!acpi_sci_override_gsi) {
> + int irq = gsi_to_irq(acpi_gbl_FADT.sci_interrupt);
unsigned int. New line between variable declaration and code.
> + acpi_sci_ioapic_setup(irq, acpi_gbl_FADT.sci_interrupt, 0, 0);
> + }
>
> /* Fill in identity legacy mappings where no override */
> mp_config_acpi_legacy_irqs();
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index a917fdf..61b59ef 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -97,6 +97,8 @@ int mp_irq_entries;
> /* GSI interrupts */
> static int nr_irqs_gsi = NR_IRQS_LEGACY;
>
> +static int boot_ioapic_idx;
> +
> #if defined (CONFIG_MCA) || defined (CONFIG_EISA)
> int mp_bus_id_to_type[MAX_MP_BUSSES];
> #endif
> @@ -1032,7 +1034,7 @@ static inline int irq_trigger(int idx)
> int (*ioapic_renumber_irq)(int ioapic, int irq);
> static int pin_2_irq(int idx, int apic, int pin)
> {
> - int irq, i;
> + int irq;
Can we please do a cleanup of irq variables to use unsigned
int. That should be done as a separate patch.
> int bus = mp_irqs[idx].srcbus;
>
> /*
> @@ -1044,18 +1046,28 @@ static int pin_2_irq(int idx, int apic, int pin)
> if (test_bit(bus, mp_bus_not_pci)) {
> irq = mp_irqs[idx].srcbusirq;
Can we simply return mp_irqs[idx].srcbusirq here and get rid of the
else path ident level ?
> } else {
> - /*
> - * PCI IRQs are mapped in order
> - */
> - i = irq = 0;
> - while (i < apic)
> - irq += nr_ioapic_registers[i++];
> - irq += pin;
> + unsigned int gsi;
New line please
> + if (!acpi_ioapic) {
> + int i;
> + /*
> + * PCI IRQs are mapped in order
> + */
> + i = gsi = 0;
> + while (i < apic)
> + gsi += nr_ioapic_registers[i++];
> + gsi += pin;
> + } else
> + gsi = pin + mp_gsi_routing[apic].gsi_base;
> +
> +#ifdef CONFIG_X86_32
> /*
> * For MPS mode, so far only needed by ES7000 platform
> */
> if (ioapic_renumber_irq)
> - irq = ioapic_renumber_irq(apic, irq);
> + gsi = ioapic_renumber_irq(apic, gsi);
> +#endif
> +
> + irq = gsi_to_irq(gsi);
> }
>
> #ifdef CONFIG_X86_32
> @@ -1505,9 +1517,10 @@ static void __init setup_IO_APIC_irqs(void)
> struct irq_cfg *cfg;
> int node = cpu_to_node(boot_cpu_id);
>
> + apic_id = boot_ioapic_idx;
> +
> apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
>
> - for (apic_id = 0; apic_id < nr_ioapics; apic_id++)
> for (pin = 0; pin < nr_ioapic_registers[apic_id]; pin++) {
> idx = find_irq_entry(apic_id, pin, mp_INT);
> if (idx == -1) {
> @@ -1529,9 +1542,6 @@ static void __init setup_IO_APIC_irqs(void)
>
> irq = pin_2_irq(idx, apic_id, pin);
>
> - if ((apic_id > 0) && (irq > 16))
> - continue;
> -
> /*
> * Skip the timer IRQ if there's a quirk handler
> * installed and if it returns 1:
> @@ -1565,7 +1575,7 @@ static void __init setup_IO_APIC_irqs(void)
> * but could not use acpi_register_gsi()
> * like some special sci in IBM x3330
> */
> -void setup_IO_APIC_irq_extra(u32 gsi)
> +void setup_IO_APIC_irq_extra(u32 gsi, unsigned int *pirq)
> {
> int apic_id = 0, pin, idx, irq;
> int node = cpu_to_node(boot_cpu_id);
> @@ -1585,6 +1595,7 @@ void setup_IO_APIC_irq_extra(u32 gsi)
> return;
>
> irq = pin_2_irq(idx, apic_id, pin);
> + *pirq = irq;
> #ifdef CONFIG_SPARSE_IRQ
> desc = irq_to_desc(irq);
> if (desc)
> @@ -2028,6 +2039,30 @@ void __init enable_IO_APIC(void)
> clear_IO_APIC();
> }
>
> +static void __init probe_ioapic_i8259(void)
> +{
> + /* probe boot ioapic idx */
> + boot_ioapic_idx = ioapic_i8259.apic;
boot_ioapic_idx is an apic id. Why is the variable name suggesting
it's an index ?
> + if (boot_ioapic_idx < 0)
> + boot_ioapic_idx = find_isa_irq_apic(0, mp_INT);
> +#ifdef CONFIG_ACPI
> + if (!acpi_disabled && acpi_ioapic && boot_ioapic_idx < 0)
> + boot_ioapic_idx = mp_find_ioapic(irq_to_gsi(0));
> +#endif
> + if (boot_ioapic_idx < 0)
> + boot_ioapic_idx = 0;
> +
> +#ifdef CONFIG_ACPI
> + if (mp_gsi_routing[boot_ioapic_idx].gsi_base) {
> + gsi_delta = NR_IRQS_LEGACY;
> + nr_irqs_gsi += NR_IRQS_LEGACY;
> + printk(KERN_DEBUG "new nr_irqs_gsi: %d\n", nr_irqs_gsi);
> + }
> +#endif
> +
> + printk(KERN_INFO "boot_ioapic_idx: %d\n", boot_ioapic_idx);
> +}
> +
> /*
> * Not an __init, needed by the reboot code
> */
> @@ -3045,7 +3080,7 @@ static inline void __init check_timer(void)
> legacy_pic->chip->unmask(0);
> }
> if (disable_timer_pin_1 > 0)
> - clear_IO_APIC_pin(0, pin1);
> + clear_IO_APIC_pin(apic1, pin1);
How is this change related to this patch ? It looks more like an
independent fix.
Thanks,
tglx
--
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