LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: vgoyal@in.ibm.com
Cc: Benjamin Romer <benjamin.romer@unisys.com>, linux-kernel@vger.kernel.org
Subject: Re: PATCH: Update disable_IO_APIC to use 8-bit destination field (X86_64)
Date: Thu, 18 Jan 2007 00:10:55 -0700	[thread overview]
Message-ID: <m1tzyo7qtc.fsf@ebiederm.dsl.xmission.com> (raw)
In-Reply-To: <20070118043639.GA12459@in.ibm.com> (Vivek Goyal's message of "Thu, 18 Jan 2007 10:06:39 +0530")

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

  reply	other threads:[~2007-01-18  7:12 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-17 16:46 Benjamin Romer
2007-01-17 19:08 ` Eric W. Biederman
2007-01-18  3:41   ` Vivek Goyal
2007-01-18  4:36     ` Vivek Goyal
2007-01-18  7:10       ` Eric W. Biederman [this message]
     [not found]         ` <2 0070118080003.GC31860@in.ibm.com>
2007-01-18  8:00         ` Vivek Goyal
2007-01-18 14:58           ` Benjamin Romer
2007-01-18 17:23           ` Benjamin Romer
2007-01-18 18:14             ` Eric W. Biederman
     [not found]               ` <1169147619.6665. 11.camel@ustr-romerbm-2.na.uis.unisys.com>
2007-01-18 19:13               ` Benjamin Romer
2007-01-19  3:49                 ` Vivek Goyal
2007-01-19 15:11                   ` Benjamin Romer
2007-01-19 15:43                     ` Randy Dunlap
2007-01-19 17:14                       ` Benjamin Romer
2007-01-19 15:55                     ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m1tzyo7qtc.fsf@ebiederm.dsl.xmission.com \
    --to=ebiederm@xmission.com \
    --cc=benjamin.romer@unisys.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=vgoyal@in.ibm.com \
    --subject='Re: PATCH: Update disable_IO_APIC to use 8-bit destination field (X86_64)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).