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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/05/11 22:35, Keir Fraser wrote:
> 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.
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