Re: [XenPPC] [PATCH] Handshake with secondary processors (take 2)

[Jimi Xenidis <jimix@xxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-ppc-devel

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