From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 0 of 6] Fix kexec in Xen (take 3) Date: Thu, 26 May 2011 10:12:01 +0100 Message-ID: <4DDE1961.80303@citrix.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 25/05/11 22:35, Keir Fraser wrote: > On 25/05/2011 17:30, "Andrew Cooper" wrote: > >>> I was about to reply to the individual patches, but they just seem >>> too inconsistent to me (comments not matching code, without it >>> being clear whether code or comment is wrong; functions introduced >>> that have no callers). Can you work on getting them into a >>> state suitable for reviewing? >> I was splitting the patches up to make them smaller and modular. With >> the patches as a full series, there are no functions without callers. >> >> Which comments don't match the code? > + /* If we are the boot processor, stick the local apic back to how we > found > + * it on boot. */ > + if( smp_processor_id() != 0 ) > > It's a pretty fundamental one. Oops - point taken. I should never attempt to do a trivial cleanup of code. >>> Further I don't buy your pseudo-quoting of the MP spec saying >>> that secondary CPUs' local APICs have to be disabled. Keir already >>> pointed out on your previous submission that in order for them to >>> receive the INIT and Startup IPIs they must be enabled. >> What Keir said and what the MP spec states are in direct contraction. >> Please do correct me if I have misread/misinterpreted the spec, but: >> >> Section 3.8 states that all local APICs are disabled when the BIOS hands >> over to the OS. >> >> and >> >> Section 3.7.3 states that the INIT IPI twiddles the APIC reset lines, >> which enabled them when they come out of reset, thus receiving and >> handling the IPI. > You are quoting from a spec that is nearly 15 years old, and particularly > addressing 486 and older systems with discrete APICs. Xen has never run on > such systems. > > A better reference for APIC behaviour is Chapter 10 of Volume 3A of the > Intel Software Developer Manual. See 10.4.7.1 particularly. The APIC is > software disabled on startup -- meaning that the enable bit in the SPIV > register is clear. That is quite different from *hardware* disable (via the > APICBASE MSR) which your patch attempts to deal with. In this latter case > the APIC would be totally shut down and it would not be possible to > INIT-SIPI the secondary processor. The software disable (via SPIV) is very > much a semi-disabled state (and disable_local_APIC() already returns an APIC > to that state). > > -- Keir > Ok - I will read up on this more, and then I guess I have some code to change. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com