Benjamin Romer <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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)); > > /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/