On Aug 10, 2006, at 10:09 PM, Amos Waterland wrote:

Take two.
ok, I think we need a take three...


Note that the processors on a JS20 are noticably slower to handshake
than those on a JS21.  I had to rid of the hard-coded 1024 timebase
ticks and replace it with calculating five seconds from the timebase
frequency, as the timeout logic was firing on my JS20 blade.

What should we do about CPUs that successfully start but take too long to ack, not sure where they'll end up.
This is prolly overkill but perhaps we shoud:
  boot_of.c (in psuedo)
        *ack = cpuid;
        do { ... } while (*ack == 0)

  exceptions.S:
        spin_start:
                /* Our physical cpu number is passed in r3.  */
                LOADADDR(r4, __spin_ack)
        1:
                ldw r5, 0(r4)
                cmpw r6,r3,r5
                beq r6, 2f
                sync
                b 1b
        2:      li r5, 0
                stw r5, 0(r4)
                sync
        barrier:
                b .

Or am I just being paranoid?

Anyway... useful comments below.
-JX


diff -r 058f2e27476d xen/arch/powerpc/boot_of.c
--- a/xen/arch/powerpc/boot_of.c        Mon Aug 07 17:49:16 2006 -0500
+++ b/xen/arch/powerpc/boot_of.c        Thu Aug 10 21:56:18 2006 -0400
@@ -30,6 +30,9 @@
 #include <asm/io.h>
 #include "exceptions.h"
 #include "of-devtree.h"
+
+/* Secondary processors use this for handshaking with main processor. */
+volatile unsigned int __spin_ack;


This volatile does not help you since you don't actually use __spin_ack directly, see below.

 static ulong of_vec;
 static ulong of_msr;
@@ -928,7 +931,7 @@ static int __init boot_of_cpus(void)
 static int __init boot_of_cpus(void)
 {
     int cpus;
-    int cpu;
+    int cpu, bootcpu;
     int result;
     u32 cpu_clock[2];

@@ -953,10 +956,52 @@ static int __init boot_of_cpus(void)
     cpu_khz /= 1000;
     of_printf("OF: clock-frequency = %ld KHz\n", cpu_khz);

- /* FIXME: should not depend on the boot CPU bring the first child */
+    /* Look up which CPU we are running on right now.  */
+ result = of_getprop(bof_chosen, "cpu", &bootcpu, sizeof (bootcpu));
+    if (result == OF_FAILURE)
+        of_panic("Failed to look up boot cpu\n");
+
     cpu = of_getpeer(cpu);
     while (cpu > 0) {
-        of_start_cpu(cpu, (ulong)spin_start, 0);
+        unsigned int cpuid, ping, pong;
+        unsigned long now, then, timeout;
+        unsigned int *ack = (unsigned int *)&__spin_ack;

You needed this cast because gcc complained that you were going to lose a qualifier, right. Gcc is correct (as usual) and what you really want is to make sure that "ack" is volatile _not_ __spin_ack.
So this should be:
        unsigned int volatile *ack = &__spin_ack;

Note: the mb() stuf may not require the volatile at all, but I'm ok with using it, you do however need the mb() for its "sync" properties.

+
+        if (cpu == bootcpu) {
+            of_printf("skipping boot cpu!\n");
+            continue;
+        }
+
+        result = of_getprop(cpu, "reg", &cpuid, sizeof(cpuid));
+        if (result == OF_FAILURE)
+            of_panic("cpuid lookup failed\n");
+
+        of_printf("spinning up secondary processor #%d: ", cpuid);
+
+        *ack = ~0x0;
+        ping = *ack;
+        of_printf("ping = 0x%x: ", ping);
+
+        mb();
+        of_start_cpu(cpu, (ulong)spin_start, cpuid);

We have privately discussed the fact that there is a bug in of_start_cpu() that will cause it to always return OF_FAILURE. Once that is fixed, please put in code that checks for success for OF on this method.


+
+ /* We will give the secondary processor five seconds to reply. */
+        then = mftb();
+        timeout = then + (5 * timebase_freq);
+
+        do {
+            now = mftb();
+            if (now >= timeout) {
+                of_printf("BROKEN: ");
+                pong = 0x0;
+                break;
+            }
+
+            mb();
+            pong = *ack;
+        } while (pong == ping);
+        of_printf("pong = 0x%x\n", pong);
+
         cpu = of_getpeer(cpu);
     }
     return 1;
@@ -1004,6 +1049,7 @@ multiboot_info_t __init *boot_of_init(
     boot_of_rtas();

     /* end of OF */
+    of_printf("Quiescing Open Firmware ...\n");
     of_call("quiesce", 0, 0, NULL);

     return &mbi;
diff -r 058f2e27476d xen/arch/powerpc/powerpc64/exceptions.S
--- a/xen/arch/powerpc/powerpc64/exceptions.S Mon Aug 07 17:49:16 2006 -0500 +++ b/xen/arch/powerpc/powerpc64/exceptions.S Thu Aug 10 22:03:15 2006 -0400
@@ -514,6 +514,14 @@ _GLOBAL(sleep)
     mtmsrd r3
     blr

+/* Firmware is told to spin up secondary processors at this address. We + * only need a function entry point instead of a descriptor since this is
+ * never called from C code.
+ */    
     .globl spin_start
 spin_start:
+    /* Our physical cpu number is passed in r3.  */
+    LOADADDR(r4, __spin_ack)
+    stw r3, 0(r4)
+    sync
     b .

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


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

Reply via email to