Re: PATCH: Update disable_IO_APIC to use 8-bit destination field (X86_64)

From: Benjamin Romer
Date: Thu Jan 18 2007 - 14:14:51 EST

On Thu, 2007-01-18 at 11:14 -0700, Eric W. Biederman wrote:
> Benjamin Romer <benjamin.romer@xxxxxxxxxx> writes:
>
> > On Thu, 2007-01-18 at 13:30 +0530, Vivek Goyal wrote:
> >> On Thu, Jan 18, 2007 at 12:10:55AM -0700, Eric W. Biederman wrote:
> >> > Vivek Goyal <vgoyal@xxxxxxxxxx> writes:
> >> > >
> >> > > Or how about making physical_dest field also 8bit like logical_dest field.
> >> > > This will work both for 4bit and 8bit physical apic ids at the same time
> >> > > code becomes more intutive and it is easier to know whether IOAPIC is
> > being
> >> > > put in physical or destination logical mode.
> >> >
> >> > Exactly what I was trying to suggest.
> >> >
> >> > Looking closer at the code I think it makes sense to just kill the union and
> >> > stop the discrimination between physical and logical modes and just have a
> >> > dest field in the structure. Roughly as you were suggesting at first.
> >> >
> >> > The reason we aren't bitten by this on a regular basis is the normal code
> >> > path uses logical.logical_dest in both logical and physical modes.
> >> > Which is a little confusing.
> >> >
> >> > Since there really isn't a distinction to be made we should just stop
> >> > trying, which will make maintenance easier :)
> >> >
> >> > Currently there are several non-common case users of physical_dest
> >> > that are probably bitten by this problem under the right
> >> > circumstances.
> >> >
> >> > So I think we should just make the structure:
> >> >
> >> > struct IO_APIC_route_entry {
> >> > __u32 vector : 8,
> >> > delivery_mode : 3, /* 000: FIXED
> >> > * 001: lowest prio
> >> > * 111: ExtINT
> >> > */
> >> > dest_mode : 1, /* 0: physical, 1: logical */
> >> > delivery_status : 1,
> >> > polarity : 1,
> >> > irr : 1,
> >> > trigger : 1, /* 0: edge, 1: level */
> >> > mask : 1, /* 0: enabled, 1: disabled */
> >> > __reserved_2 : 15;
> >> >
> >> > __u32 __reserved_3 : 24,
> >> > __dest : 8;
> >> > } __attribute__ ((packed));
> >> >
> >> > And fixup the users. This should keep us from getting bit by this bug
> >> > in the future. Like when people start introducing support for more
> >> > than 256 cores and the low 24bits start getting used.
> >> >
> >> > Or when someone new starts working on the code and thinks the fact
> >> > the field name says logical we are actually using the apic in logical
> >> > mode.
> >>
> >> This makes perfect sense to me. Ben, interested in providing a patch
> >> for this?
> >>
> >> Thanks
> >> Vivek
> >
> > OK, here's the updated patch that uses the new definition and fixes up
> > the other places that use it. I built and tested this on the ES7000/ONE
> > and it works well. :)
>
> Cool.
>
> I hate to pick nits by why the double underscore on dest?
>

It was defined that way in the updated structure definition you sent in
a previous mail, so I figured you wanted it that way. Here's another
revision with the double underscores removed. :)

-- Ben

diff -ru linux-2.6.19.2-orig/arch/x86_64/kernel/io_apic.c
linux-2.6.19.2/arch/x86_64/kernel/io_apic.c
--- linux-2.6.19.2-orig/arch/x86_64/kernel/io_apic.c 2007-03-18
12:29:52.000000000 -0400
+++ linux-2.6.19.2/arch/x86_64/kernel/io_apic.c 2007-03-18
13:15:26.000000000 -0400
@@ -814,7 +814,7 @@
entry.delivery_mode = INT_DELIVERY_MODE;
entry.dest_mode = INT_DEST_MODE;
entry.mask = 0; /* enable IRQ */
- entry.dest.logical.logical_dest = cpu_mask_to_apicid(TARGET_CPUS);
+ entry.dest = cpu_mask_to_apicid(TARGET_CPUS);

idx = find_irq_entry(apic,pin,mp_INT);
if (idx == -1) {
@@ -832,7 +832,7 @@
if (irq_trigger(idx)) {
entry.trigger = 1;
entry.mask = 1;
- entry.dest.logical.logical_dest = cpu_mask_to_apicid(TARGET_CPUS);
+ entry.dest = cpu_mask_to_apicid(TARGET_CPUS);
}

irq = pin_2_irq(idx, apic, pin);
@@ -847,7 +847,7 @@
if (vector < 0)
continue;

- entry.dest.logical.logical_dest = cpu_mask_to_apicid(mask);
+ entry.dest = cpu_mask_to_apicid(mask);
entry.vector = vector;

ioapic_register_intr(irq, vector, IOAPIC_AUTO);
@@ -888,7 +888,7 @@
*/
entry.dest_mode = INT_DEST_MODE;
entry.mask = 0; /* unmask IRQ now */
- entry.dest.logical.logical_dest = cpu_mask_to_apicid(TARGET_CPUS);
+ entry.dest = cpu_mask_to_apicid(TARGET_CPUS);
entry.delivery_mode = INT_DELIVERY_MODE;
entry.polarity = 0;
entry.trigger = 0;
@@ -988,18 +988,17 @@

printk(KERN_DEBUG ".... IRQ redirection table:\n");

- printk(KERN_DEBUG " NR Log Phy Mask Trig IRR Pol"
- " Stat Dest Deli Vect: \n");
+ printk(KERN_DEBUG " NR Dst Mask Trig IRR Pol"
+ " Stat Dmod Deli Vect: \n");

for (i = 0; i <= reg_01.bits.entries; i++) {
struct IO_APIC_route_entry entry;

entry = ioapic_read_entry(apic, i);

- printk(KERN_DEBUG " %02x %03X %02X ",
+ printk(KERN_DEBUG " %02x %03X ",
i,
- entry.dest.logical.logical_dest,
- entry.dest.physical.physical_dest
+ entry.dest
);

printk("%1d %1d %1d %1d %1d %1d %1d %02X\n",
@@ -1261,8 +1260,7 @@
entry.dest_mode = 0; /* Physical */
entry.delivery_mode = dest_ExtINT; /* ExtInt */
entry.vector = 0;
- entry.dest.logical.logical_dest =
- GET_APIC_ID(apic_read(APIC_ID));
+ entry.dest = GET_APIC_ID(apic_read(APIC_ID));

/*
* Add it to the IO-APIC irq-routing table:
@@ -1524,7 +1522,7 @@

entry1.dest_mode = 0; /* physical delivery */
entry1.mask = 0; /* unmask IRQ now */
- entry1.dest.physical.physical_dest = hard_smp_processor_id();
+ entry1.dest = hard_smp_processor_id();
entry1.delivery_mode = dest_ExtINT;
entry1.polarity = entry0.polarity;
entry1.trigger = 0;
@@ -2092,7 +2090,7 @@

entry.delivery_mode = INT_DELIVERY_MODE;
entry.dest_mode = INT_DEST_MODE;
- entry.dest.logical.logical_dest = cpu_mask_to_apicid(mask);
+ entry.dest = cpu_mask_to_apicid(mask);
entry.trigger = triggering;
entry.polarity = polarity;
entry.mask = 1; /* Disabled (masked) */
diff -ru linux-2.6.19.2-orig/include/asm-x86_64/io_apic.h
linux-2.6.19.2/include/asm-x86_64/io_apic.h
--- linux-2.6.19.2-orig/include/asm-x86_64/io_apic.h 2007-03-18
12:30:07.000000000 -0400
+++ linux-2.6.19.2/include/asm-x86_64/io_apic.h 2007-03-18
12:58:37.000000000 -0400
@@ -72,31 +72,21 @@
};

struct IO_APIC_route_entry {
- __u32 vector : 8,
- delivery_mode : 3, /* 000: FIXED
- * 001: lowest prio
- * 111: ExtINT
- */
- dest_mode : 1, /* 0: physical, 1: logical */
- delivery_status : 1,
- polarity : 1,
- irr : 1,
- trigger : 1, /* 0: edge, 1: level */
- mask : 1, /* 0: enabled, 1: disabled */
- __reserved_2 : 15;
-
- union { struct { __u32
- __reserved_1 : 24,
- physical_dest : 4,
- __reserved_2 : 4;
- } physical;
-
- struct { __u32
- __reserved_1 : 24,
- logical_dest : 8;
- } logical;
- } dest;
+ __u32 vector : 8,
+ delivery_mode : 3, /* 000: FIXED
+ * 001: lowest prio
+ * 111: ExtINT
+ */
+ dest_mode : 1, /* 0: physical, 1: logical */
+ delivery_status : 1,
+ polarity : 1,
+ irr : 1,
+ trigger : 1, /* 0: edge, 1: level */
+ mask : 1, /* 0: enabled, 1: disabled */
+ __reserved_2 : 15;

+ __u32 __reserved_3 : 24,
+ dest : 8;
} __attribute__ ((packed));

/*


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/