Re: [PATCH] PowerPC: Add Program Priority Register support

["Ryan S. Arnold" <ryan dot arnold at gmail dot com>]

On Tue, Aug 14, 2012 at 8:03 AM, Adhemerval Zanella
<azanella@linux.vnet.ibm.com> wrote:
I saw one more thing.

I think that the functionality of the saved PPR value in the TLS
address space is correct but I think that some of the comments are
incorrect.

> diff --git a/nptl/sysdeps/powerpc/tcb-offsets.sym b/nptl/sysdeps/powerpc/tcb-offsets.sym
> index 8ac133d..7f65b9f 100644
> --- a/nptl/sysdeps/powerpc/tcb-offsets.sym
> +++ b/nptl/sysdeps/powerpc/tcb-offsets.sym
> @@ -14,6 +14,7 @@ MULTIPLE_THREADS_OFFSET               thread_offsetof (header.multiple_threads)
>  #endif
>  PID                            thread_offsetof (pid)
>  TID                            thread_offsetof (tid)
> +PPR_OFFSET                     (offsetof (tcbhead_t, ppr) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>  POINTER_GUARD                  (offsetof (tcbhead_t, pointer_guard) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>  #ifndef __ASSUME_PRIVATE_FUTEX
>  PRIVATE_FUTEX_OFFSET           thread_offsetof (header.private_futex)

Notice how the PPR_OFFSET (as you've placed it) is a negative offset
from the tcbhead (which is, itself negative offset from the value in
the thread-pointer register by TLS_TCB_OFFSET).  This space prior to
the start of the TCB is defined by the TLS ABI as a place for "other
thread library information".

> diff --git a/nptl/sysdeps/powerpc/tls.h b/nptl/sysdeps/powerpc/tls.h
> index 4c09eec..6ed6e65 100644
> --- a/nptl/sysdeps/powerpc/tls.h
> +++ b/nptl/sysdeps/powerpc/tls.h
> @@ -1,5 +1,5 @@
>  /* Definition for thread-local data handling.  NPTL/PowerPC version.
> -   Copyright (C) 2003, 2005, 2006, 2007, 2011 Free Software Foundation, Inc.
> +   Copyright (C) 2003-2012 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -61,6 +61,8 @@ typedef union dtv
>     are private.  */
>  typedef struct
>  {
> +  /* Program Priority Register saved value.  */
> +  uint64_t  ppr;
>    uintptr_t pointer_guard;
>    uintptr_t stack_guard;
>    dtv_t *dtv;

The Power TLS ABI defines the start of the TCB as the DTV pointer.
The stack guard and pointer guard are accessed by negative offsets
from the head of the TCB.  This means, that you've put the PPR save
value in the correct place because you preserve the existing offsets
to the pointer_guard and stack_guard off the TCB, which preserves
binary compatibility.

...

> diff --git a/sysdeps/powerpc/powerpc32/sysdep.h b/sysdeps/powerpc/powerpc32/sysdep.h
> index 02917a8..a1a5709 100644
> --- a/sysdeps/powerpc/powerpc32/sysdep.h
> +++ b/sysdeps/powerpc/powerpc32/sysdep.h
> @@ -89,9 +89,26 @@ GOT_LABEL:                   ;                                             \
>    cfi_endproc;                                                               \
>    ASM_SIZE_DIRECTIVE(name)
>
> +/* The Priority Program Register (PPR) is saved and restored by moving its
> +   value to ppr field in TBC. Since it is the loader responsible to setup the
> +   TBC, the code is only built if not within the loader.  */

With my explanation in mind, I think this comment isn't quite correct.
 It should read as something like the following:

/* The value in the Priority Program Register (PPR) is saved and restored
   by moving its value to, and fetching its value from, a field preceding the
   TCB that is reserved by the TLS ABI for "other thread library information".
   Since it is the loader's responsibility to setup the TCB (and the fields in
   the reserved space), the PPR save/restore facility is only available outside
   of the loader.  */

You could add a comment that the loader always, therefore, runs in the
default thread priority.
...

> diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
> index ed96478..79566be 100644
> --- a/sysdeps/powerpc/powerpc64/sysdep.h
> +++ b/sysdeps/powerpc/powerpc64/sysdep.h
> @@ -205,9 +205,26 @@ LT_LABELSUFFIX(name,_name_end): ; \
>    TRACEBACK_MASK(name,mask)    \
>    END_2(name)
>
> +/* The Priority Program Register (PPR) is saved and restored by moving its
> +   value to ppr field in TBC. Since it is the loader responsible to setup the
> +   TBC, the code is only built if not within the loader.  */

Same comment applies as for ppc32.

Ryan S. Arnold