Re: [PATCH 0 of 6] Fix kexec in Xen (take 3)

[Keir Fraser <keir.xen@gmail.com>]

On 25/05/2011 17:30, "Andrew Cooper" <andrew.cooper3@citrix.com> 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.

>> 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