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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/05/11 22:35, Keir Fraser wrote:
> On 25/05/2011 17:30, "Andrew Cooper"<andrew.cooper3@xxxxxxxxxx>  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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel