[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.