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

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

[Benjamin Romer]

On the Unisys ES7000/ONE system, we encountered a problem where
performing a kexec reboot or dump on any cell other than cell 0 causes
the system timer to stop working, resulting in a hang during timer
calibration in the new kernel. 

We traced the problem to one line of code in disable_IO_APIC(), which
needs to restore the timer's IO-APIC configuration before rebooting. The
code is currently using the 4-bit physical destination field, rather
than using the 8-bit logical destination field, and it cuts off the
upper 4 bits of the timer's APIC ID. If we change this to use the
logical destination field, the timer works and we can kexec on the upper
cells. This was tested on two different cells (0 and 2) in an ES7000/ONE
system.

For reference, the relevant Intel xAPIC spec is kept at
ftp://download.intel.com/design/chipsets/e8501/datashts/30962001.pdf,
specifically on page 334.

-- Ben

--- linux-2.6.19.2.orig/arch/x86_64/kernel/io_apic.c    2007-01-10
14:10:37.000000000 -0500
+++ linux-2.6.19.2/arch/x86_64/kernel/io_apic.c 2007-01-17
09:20:24.000000000 -0500
@@ -1261,7 +1261,7 @@
                entry.dest_mode       = 0; /* Physical */
                entry.delivery_mode   = dest_ExtINT; /* ExtInt */
                entry.vector          = 0;
-               entry.dest.physical.physical_dest =
+               entry.dest.logical.logical_dest =
                                        GET_APIC_ID(apic_read(APIC_ID));
 
                /*

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

[Eric W. Biederman]

Benjamin Romer <benjamin.romer@unisys.com> writes:

> On the Unisys ES7000/ONE system, we encountered a problem where
> performing a kexec reboot or dump on any cell other than cell 0 causes
> the system timer to stop working, resulting in a hang during timer
> calibration in the new kernel. 
>
> We traced the problem to one line of code in disable_IO_APIC(), which
> needs to restore the timer's IO-APIC configuration before rebooting. The
> code is currently using the 4-bit physical destination field, rather
> than using the 8-bit logical destination field, and it cuts off the
> upper 4 bits of the timer's APIC ID. If we change this to use the
> logical destination field, the timer works and we can kexec on the upper
> cells. This was tested on two different cells (0 and 2) in an ES7000/ONE
> system.
>
> For reference, the relevant Intel xAPIC spec is kept at
> ftp://download.intel.com/design/chipsets/e8501/datashts/30962001.pdf,
> specifically on page 334.

Looks like good bug hunting.  I will have to look but it might
make more sense to simply fix: struct IO_APIC_route_entry,
or use whatever technique we normally use to generate the io_apic
vectors.

I don't recall enough off of the top of my head to recall what
the discrimination rule between logical and physical is but
I think setting the system in physical mode is a good clue :)

Eric

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

[Vivek Goyal]

On Wed, Jan 17, 2007 at 12:08:48PM -0700, Eric W. Biederman wrote:
> Benjamin Romer <benjamin.romer@unisys.com> writes:
> 
> > On the Unisys ES7000/ONE system, we encountered a problem where
> > performing a kexec reboot or dump on any cell other than cell 0 causes
> > the system timer to stop working, resulting in a hang during timer
> > calibration in the new kernel.
> >
> > We traced the problem to one line of code in disable_IO_APIC(), which
> > needs to restore the timer's IO-APIC configuration before rebooting. The
> > code is currently using the 4-bit physical destination field, rather
> > than using the 8-bit logical destination field, and it cuts off the
> > upper 4 bits of the timer's APIC ID. If we change this to use the
> > logical destination field, the timer works and we can kexec on the upper
> > cells. This was tested on two different cells (0 and 2) in an ES7000/ONE
> > system.
> >
> > For reference, the relevant Intel xAPIC spec is kept at
> > ftp://download.intel.com/design/chipsets/e8501/datashts/30962001.pdf,
> > specifically on page 334.
> 
> Looks like good bug hunting.  I will have to look but it might
> make more sense to simply fix: struct IO_APIC_route_entry,
> or use whatever technique we normally use to generate the io_apic
> vectors.
> 
> I don't recall enough off of the top of my head to recall what
> the discrimination rule between logical and physical is but
> I think setting the system in physical mode is a good clue :)

Hi Eric,

In physical destination mode, the destination APIC is determined by 
APIC ID and in logical destination mode, destination apics are determined
by the configurations based on LDR and DFR registers in APIC (Depending
on Flat mode or cluster mode).

Looks like previously one supported only 4bit apic ids if system is
operating in physical mode and 8bit ids if IOAPIC is put in logical
destination mode. That's why, struct IO_APIC_route_entry is containing
4bits for physical apic id.

http://www.intel.com/design/chipsets/datashts/290566.htm

And now newer systems have switched to 8bit apic ids in physical mode.
That's why if somebody is crashing on a cpu whose apic id is more than
16, kexec/kdump code will fail as 4bits are not sufficient.

Hence above change makes sense. Given the fact that logical and physical
apic id is basically a union, it will work even for older systems where
physical apic ids were 4bits only.

OTOH, I think down the line we can get rid of physical dest
field all together in struct IO_APIC_route_entry and use logical dest
field.

Thanks
Vivek

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

[Vivek Goyal]

On Thu, Jan 18, 2007 at 09:11:53AM +0530, Vivek Goyal wrote:
> On Wed, Jan 17, 2007 at 12:08:48PM -0700, Eric W. Biederman wrote:
> > Benjamin Romer <benjamin.romer@unisys.com> writes:
> >
> > > On the Unisys ES7000/ONE system, we encountered a problem where
> > > performing a kexec reboot or dump on any cell other than cell 0 causes
> > > the system timer to stop working, resulting in a hang during timer
> > > calibration in the new kernel.
> > >
> > > We traced the problem to one line of code in disable_IO_APIC(), which
> > > needs to restore the timer's IO-APIC configuration before rebooting. The
> > > code is currently using the 4-bit physical destination field, rather
> > > than using the 8-bit logical destination field, and it cuts off the
> > > upper 4 bits of the timer's APIC ID. If we change this to use the
> > > logical destination field, the timer works and we can kexec on the upper
> > > cells. This was tested on two different cells (0 and 2) in an ES7000/ONE
> > > system.
> > >
> > > For reference, the relevant Intel xAPIC spec is kept at
> > > ftp://download.intel.com/design/chipsets/e8501/datashts/30962001.pdf,
> > > specifically on page 334.
> >
> > Looks like good bug hunting.  I will have to look but it might
> > make more sense to simply fix: struct IO_APIC_route_entry,
> > or use whatever technique we normally use to generate the io_apic
> > vectors.
> >
> > I don't recall enough off of the top of my head to recall what
> > the discrimination rule between logical and physical is but
> > I think setting the system in physical mode is a good clue :)
> 
> Hi Eric,
> 
> In physical destination mode, the destination APIC is determined by
> APIC ID and in logical destination mode, destination apics are determined
> by the configurations based on LDR and DFR registers in APIC (Depending
> on Flat mode or cluster mode).
> 
> Looks like previously one supported only 4bit apic ids if system is
> operating in physical mode and 8bit ids if IOAPIC is put in logical
> destination mode. That's why, struct IO_APIC_route_entry is containing
> 4bits for physical apic id.
> 
> http://www.intel.com/design/chipsets/datashts/290566.htm
> 
> And now newer systems have switched to 8bit apic ids in physical mode.
> That's why if somebody is crashing on a cpu whose apic id is more than
> 16, kexec/kdump code will fail as 4bits are not sufficient.
> 
> Hence above change makes sense. Given the fact that logical and physical
> apic id is basically a union, it will work even for older systems where
> physical apic ids were 4bits only.
> 
> OTOH, I think down the line we can get rid of physical dest
> field all together in struct IO_APIC_route_entry and use logical dest
> field.
> 

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.

Thanks
Vivek

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

[Eric W. Biederman]

Vivek Goyal <vgoyal@in.ibm.com> writes:


>> Hi Eric,
>> 
>> In physical destination mode, the destination APIC is determined by
>> APIC ID and in logical destination mode, destination apics are determined
>> by the configurations based on LDR and DFR registers in APIC (Depending
>> on Flat mode or cluster mode).
>> 
>> Looks like previously one supported only 4bit apic ids if system is
>> operating in physical mode and 8bit ids if IOAPIC is put in logical
>> destination mode. That's why, struct IO_APIC_route_entry is containing
>> 4bits for physical apic id.
>> 
>> http://www.intel.com/design/chipsets/datashts/290566.htm
>> 
>> And now newer systems have switched to 8bit apic ids in physical mode.
>> That's why if somebody is crashing on a cpu whose apic id is more than
>> 16, kexec/kdump code will fail as 4bits are not sufficient.
>> 
>> Hence above change makes sense. Given the fact that logical and physical
>> apic id is basically a union, it will work even for older systems where
>> physical apic ids were 4bits only.
>> 
>> OTOH, I think down the line we can get rid of physical dest
>> field all together in struct IO_APIC_route_entry and use logical dest
>> field.
>> 
>
> 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.

Eric

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

[Vivek Goyal]

On Thu, Jan 18, 2007 at 12:10:55AM -0700, Eric W. Biederman wrote:
> Vivek Goyal <vgoyal@in.ibm.com> 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

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

[Benjamin Romer]

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@in.ibm.com> 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

Sure! I'll fix it up and get a revised patch out as soon as I can. :) 

-- Ben

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

[Benjamin Romer]

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@in.ibm.com> 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. :)

-- 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));
 
 /*

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

[Eric W. Biederman]

Benjamin Romer <benjamin.romer@unisys.com> 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@in.ibm.com> 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?

>  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));
>  
>  /*

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

[Benjamin Romer]

On Thu, 2007-01-18 at 11:14 -0700, Eric W. Biederman wrote:
> Benjamin Romer <benjamin.romer@unisys.com> 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@in.ibm.com> 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));
 
 /*

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

[Vivek Goyal]

On Thu, Jan 18, 2007 at 02:13:39PM -0500, Benjamin Romer wrote:
[..]
> > >
> > > 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
> 

This patch looks good to me. You might want to provide some description
too for changelog.

Thanks
Vivek

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

[Benjamin Romer]

On Fri, 2007-01-19 at 09:19 +0530, Vivek Goyal wrote:
> On Thu, Jan 18, 2007 at 02:13:39PM -0500, Benjamin Romer wrote:
> [..]
> > > >
> > > > 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
> > 
> 
> This patch looks good to me. You might want to provide some description
> too for changelog.
> 
> Thanks
> Vivek

A simple description would look something like:

- Updated 4-bit IO-APIC physical dest field to 8-bit dest field for
xAPIC; fixes ES7000/ONE kexec timer hang

Is there a changelog file in the kernel for kexec somewhere, or does
this belong in the source file's comments somewhere? I can fix up the
patch as soon as I know where the right spot for the description is. :)

-- Ben

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

[Randy Dunlap]

On Fri, 19 Jan 2007 10:11:10 -0500 Benjamin Romer wrote:

> On Fri, 2007-01-19 at 09:19 +0530, Vivek Goyal wrote:
> > On Thu, Jan 18, 2007 at 02:13:39PM -0500, Benjamin Romer wrote:
> > [..]
> > > > >
> > > > > 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
> > > 
> > 
> > This patch looks good to me. You might want to provide some description
> > too for changelog.
> > 
> > Thanks
> > Vivek
> 
> A simple description would look something like:
> 
> - Updated 4-bit IO-APIC physical dest field to 8-bit dest field for
> xAPIC; fixes ES7000/ONE kexec timer hang
> 
> Is there a changelog file in the kernel for kexec somewhere, or does
> this belong in the source file's comments somewhere? I can fix up the
> patch as soon as I know where the right spot for the description is. :)

We use a source code management system for changelogs. (git)

Patch comments (changelog info) go in the email at the top of the
patch.  See Documentation/SubmittingPatches and
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
and http://linux.yyz.us/patch-format.html for more info.

---
~Randy

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

[Eric W. Biederman]

Benjamin Romer <benjamin.romer@unisys.com> writes:

> On Fri, 2007-01-19 at 09:19 +0530, Vivek Goyal wrote:
>> On Thu, Jan 18, 2007 at 02:13:39PM -0500, Benjamin Romer wrote:
>> [..]
>> > > >
>> > > > 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
>> > 
>> 
>> This patch looks good to me. You might want to provide some description
>> too for changelog.
>> 
>> Thanks
>> Vivek
>
> A simple description would look something like:
>
> - Updated 4-bit IO-APIC physical dest field to 8-bit dest field for
> xAPIC; fixes ES7000/ONE kexec timer hang
>
> Is there a changelog file in the kernel for kexec somewhere, or does
> this belong in the source file's comments somewhere? I can fix up the
> patch as soon as I know where the right spot for the description is. :)

Just send a fresh email:

> Subject: [PATCH] short summary of the patch.
> 
> Description of the patch
> 
> 
> ---
> diffstat
> 
> diff

When imported into git the leading description of the patch becomes
the changelog comments.  What you send the first time that got
this thread started was great.

Eric

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

[Benjamin Romer]

On Fri, 2007-01-19 at 07:43 -0800, Randy Dunlap wrote:
> On Fri, 19 Jan 2007 10:11:10 -0500 Benjamin Romer wrote:
> 
> > On Fri, 2007-01-19 at 09:19 +0530, Vivek Goyal wrote:
> > > On Thu, Jan 18, 2007 at 02:13:39PM -0500, Benjamin Romer wrote:
> > > [..]
> > > > > >
> > > > > > 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
> > > > 
> > > 
> > > This patch looks good to me. You might want to provide some description
> > > too for changelog.
> > > 
> > > Thanks
> > > Vivek
> > 
> > A simple description would look something like:
> > 
> > - Updated 4-bit IO-APIC physical dest field to 8-bit dest field for
> > xAPIC; fixes ES7000/ONE kexec timer hang
> > 
> > Is there a changelog file in the kernel for kexec somewhere, or does
> > this belong in the source file's comments somewhere? I can fix up the
> > patch as soon as I know where the right spot for the description is. :)
> 
> We use a source code management system for changelogs. (git)
> 
> Patch comments (changelog info) go in the email at the top of the
> patch.  See Documentation/SubmittingPatches and
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
> and http://linux.yyz.us/patch-format.html for more info.
> 
> ---
> ~Randy

Thanks for the pointers! I've bookmarked them for future reference, and
sent a new patch that follows the guidelines. :)

-- Ben