[MIPS] Restore the trusted stack in NaClSyscallSeg.

[MIPS] Restore the trusted stack in NaClSyscallSeg.

Restore the trusted stack when switching back to the trusted zone.
Also, remove the extra frame-header zone.
Additional clean-up of code that used to save gp.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= pnacl/build.sh misc-tools

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=10183

[petarj, 2012-11-01 01:05:59]

Issue found by Mark S. Thanks.

[Mark Seaborn, 2012-11-01 01:47:41]

https://codereview.chromium.org/11368024/diff/1/src/trusted/service_runtime/a...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (left):

https://codereview.chromium.org/11368024/diff/1/src/trusted/service_runtime/a...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:116: /* Reserve 16-byte for
arguments (o32 ABI) */
Should you change NaClStartThreadInApp() to explicitly reserve these 16 bytes? 
We used to reserve 32 bytes that way for the Windows x86-64 ABI (see before
http://src.chromium.org/viewvc/native_client?view=rev&revision=7168).

Currently it does this for MIPS:

  natp->user.trusted_stack_ptr = (NaClGetStackPtr() & ~0xf) + 4;

The "+ 4" there is inconsistent with NACL_STACK_PAD_BELOW_ALIGN in
nacl_config.h, BTW.

https://codereview.chromium.org/11368024/diff/1/src/trusted/service_runtime/a...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (right):

https://codereview.chromium.org/11368024/diff/1/src/trusted/service_runtime/a...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:109: sw $gp,
NACL_THREAD_CONTEXT_OFFSET_GLOBAL_PTR($a1)
Does this mean untrusted code is allowed to modify $gp?  It's not clear to me
from the sandbox design doc on
https://code.google.com/p/nativeclient/issues/detail?id=2275.  Do you need to
restore $gp too?

[petarj, 2012-11-01 17:06:40]

Second patch has been uploaded.

https://chromiumcodereview.appspot.com/11368024/diff/1/src/trusted/service_ru...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (left):

https://chromiumcodereview.appspot.com/11368024/diff/1/src/trusted/service_ru...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:116: /* Reserve 16-byte for
arguments (o32 ABI) */
On 2012/11/01 01:47:41, Mark Seaborn wrote:
> Should you change NaClStartThreadInApp() to explicitly reserve these 16 bytes?

> We used to reserve 32 bytes that way for the Windows x86-64 ABI (see before
> http://src.chromium.org/viewvc/native_client?view=rev&revision=7168).
> 
> Currently it does this for MIPS:
> 
>   natp->user.trusted_stack_ptr = (NaClGetStackPtr() & ~0xf) + 4;
> 
> The "+ 4" there is inconsistent with NACL_STACK_PAD_BELOW_ALIGN in
> nacl_config.h, BTW.

The 16 bytes are already reserved at that point. No need to explicitly reserve
them.
The inconsistency has been corrected.

https://chromiumcodereview.appspot.com/11368024/diff/1/src/trusted/service_ru...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (right):

https://chromiumcodereview.appspot.com/11368024/diff/1/src/trusted/service_ru...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:109: sw $gp,
NACL_THREAD_CONTEXT_OFFSET_GLOBAL_PTR($a1)
On 2012/11/01 01:47:41, Mark Seaborn wrote:
> Does this mean untrusted code is allowed to modify $gp?  It's not clear to me
> from the sandbox design doc on
> https://code.google.com/p/nativeclient/issues/detail?id=2275.  Do you need to
> restore $gp too?
Untrusted code can modify $gp, yet saving $gp is a cruft I have just removed
from the source along with some other $gp related code.

[Mark Seaborn, 2012-11-01 18:01:56]

LGTM

https://chromiumcodereview.appspot.com/11368024/diff/1/src/trusted/service_ru...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (left):

https://chromiumcodereview.appspot.com/11368024/diff/1/src/trusted/service_ru...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:116: /* Reserve 16-byte for
arguments (o32 ABI) */
On 2012/11/01 17:06:40, petarj wrote:
> On 2012/11/01 01:47:41, Mark Seaborn wrote:
> > Should you change NaClStartThreadInApp() to explicitly reserve these 16
bytes?
> 
> > We used to reserve 32 bytes that way for the Windows x86-64 ABI (see before
> > http://src.chromium.org/viewvc/native_client?view=rev&revision=7168).
> 
> The 16 bytes are already reserved at that point. No need to explicitly reserve
> them.

OK, good point.

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
File src/trusted/service_runtime/arch/mips/nacl_switch_to_app_mips.c (left):

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
src/trusted/service_runtime/arch/mips/nacl_switch_to_app_mips.c:46:
context->global_ptr = NaClGetGlobalPtr();
Optional:  You could remove the definition of NaClGetGlobalPtr().  This was the
only use.

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (right):

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:73: .cpload $t9
Can you add a comment here saying that this restores $gp to the trusted value
that we rely on below?

I know normally the reader of assembly code would be expected to know what
standard constructs mean, but:

- We are short on MIPS experts in the NaCl project :-)
- .cpload doesn't seem to be documented in the GNU as manual:
http://sourceware.org/binutils/docs-2.23/as/index.html
- In trusted context switch code like this, it's more important to explicitly
account for every register value we rely on.

[petarj, 2012-11-02 12:37:52]

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
File src/trusted/service_runtime/arch/mips/nacl_switch_to_app_mips.c (left):

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
src/trusted/service_runtime/arch/mips/nacl_switch_to_app_mips.c:46:
context->global_ptr = NaClGetGlobalPtr();
On 2012/11/01 18:01:57, Mark Seaborn wrote:
> Optional:  You could remove the definition of NaClGetGlobalPtr().  This was
the
> only use.
Done. Sixth sense was saying we could need that function at some point, this is
why I have not removed it in the first place. Removed now.

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
File src/trusted/service_runtime/arch/mips/nacl_syscall.S (right):

https://chromiumcodereview.appspot.com/11368024/diff/10003/src/trusted/servic...
src/trusted/service_runtime/arch/mips/nacl_syscall.S:73: .cpload $t9
On 2012/11/01 18:01:57, Mark Seaborn wrote:
> Can you add a comment here saying that this restores $gp to the trusted value
> that we rely on below?
> 
> I know normally the reader of assembly code would be expected to know what
> standard constructs mean, but:
> 
> - We are short on MIPS experts in the NaCl project :-)
> - .cpload doesn't seem to be documented in the GNU as manual:
> http://sourceware.org/binutils/docs-2.23/as/index.html
> - In trusted context switch code like this, it's more important to explicitly
> account for every register value we rely on.

Done.

[commit-bot: I haz the power, 2012-11-02 12:38:11]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11368024/6002

[commit-bot: I haz the power, 2012-11-02 12:38:17]

Presubmit check for 11368024-6002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The Native
Client Authors\. All rights reserved\.\n.*? Use of this source code is governed
by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  src/trusted/service_runtime/arch/mips/nacl_switch.S \
  src/trusted/service_runtime/arch/mips/nacl_syscall.S \
  src/trusted/service_runtime/arch/mips/sel_rt.c \
  src/trusted/service_runtime/arch/mips/sel_rt.h \
  src/trusted/service_runtime/linux/mips/sel_segments.c

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  src/trusted/service_runtime/arch/mips/nacl_switch.S \
  src/trusted/service_runtime/arch/mips/nacl_syscall.S

Presubmit checks took 1.9s to calculate.

[commit-bot: I haz the power, 2012-11-02 13:01:41]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11368024/12004

[commit-bot: I haz the power, 2012-11-02 13:01:45]

Presubmit check for 11368024-12004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  src/trusted/service_runtime/arch/mips/nacl_switch.S \
  src/trusted/service_runtime/arch/mips/nacl_syscall.S

Presubmit checks took 1.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-11-02 14:42:20]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11368024/12004

[commit-bot: I haz the power, 2012-11-02 14:42:24]

Presubmit check for 11368024-12004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  src/trusted/service_runtime/arch/mips/nacl_switch.S \
  src/trusted/service_runtime/arch/mips/nacl_syscall.S

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Mark Seaborn, 2012-11-02 20:20:42]

LGTM

[commit-bot: I haz the power, 2012-11-02 20:20:56]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11368024/12004

[commit-bot: I haz the power, 2012-11-03 02:58:36]

Change committed as 10183