Resurrect Petr's 64-bit dynamic code modification CL:...

src/trusted/validator_x86/nccopycode.c

Index: src/trusted/validator_x86/nccopycode.c
===================================================================
--- src/trusted/validator_x86/nccopycode.c	(revision 3931)
+++ src/trusted/validator_x86/nccopycode.c	(working copy)
@@ -23,9 +23,17 @@
 #include <errno.h>
 #include <string.h>
 #include <assert.h>
+#include "native_client/src/shared/platform/nacl_check.h"
+#if NACL_TARGET_SUBARCH == 32
 #include "native_client/src/trusted/validator_x86/ncdecode.h"
 #include "native_client/src/trusted/validator_x86/ncvalidate.h"
-#include "native_client/src/shared/platform/nacl_check.h"
+#elif NACL_TARGET_SUBARCH == 64
+#include "native_client/src/trusted/validator_x86/nc_inst_iter.h"
+#include "native_client/src/trusted/validator_x86/nc_segment.h"
+#include "native_client/src/trusted/validator_x86/nc_inst_state_internal.h"
+#else
+#error "Unknown Platform"
+#endif
 
 /* x86 HALT opcode */
 static const uint8_t kNaClFullStop = 0xf4;
@@ -158,13 +166,11 @@
  * Copy a single instruction, avoiding the possibility of other threads
  * executing a partially changed instruction.
  */
-void CopyInstruction(const struct NCDecoderState *mstate_old,
-                     const struct NCDecoderState *mstate_new) {
-  uint8_t* dst = mstate_old->inst.maddr;
-  uint8_t* src = mstate_new->inst.maddr;
-  int sz = mstate_old->inst.length;
+void CopyInstructionInternal(uint8_t *dst,
+                             uint8_t *src,
+                             uint8_t sz) {
   intptr_t offset = 0;
-  CHECK(mstate_new->inst.length == mstate_old->inst.length);
+  uint8_t *firstbyte_p = dst;
 
   while (sz > 0 && dst[0] == src[0]) {
     /* scroll to first changed byte */
@@ -182,7 +188,7 @@
   }
 
   if (sz == 1) {
-    /* we assume a 1-byte change it atomic */
+    /* we assume a 1-byte change is atomic */
     *dst = *src;
   } else if (IsTrustedWrite(dst, sz, 4, &offset)) {
     uint8_t tmp[4];
@@ -196,13 +202,13 @@
     onestore_memmove8(dst-offset, tmp);
   } else {
     /* the slow path, first flip first byte to halt*/
-    uint8_t firstbyte = mstate_old->inst.maddr[0];
-    mstate_old->inst.maddr[0] = kNaClFullStop;
+    uint8_t firstbyte = firstbyte_p[0];
+    firstbyte_p[0] = kNaClFullStop;
 
     SerializeAllProcessors();
 
     /* copy the rest of instruction */
-    if (dst == mstate_old->inst.maddr) {
+    if (dst == firstbyte_p) {
       /* but not the first byte! */
       firstbyte = *src;
       dst++, src++, sz--;
@@ -212,10 +218,25 @@
     SerializeAllProcessors();
 
     /* flip first byte back */
-    mstate_old->inst.maddr[0] = firstbyte;
+    firstbyte_p[0] = firstbyte;
   }
 }
 
+#if NACL_TARGET_SUBARCH == 32
+
+/*
+ * Copy a single instruction, avoiding the possibility of other threads
+ * executing a partially changed instruction.
+ */
+void CopyInstruction(const struct NCDecoderState *mstate_old,
+                     const struct NCDecoderState *mstate_new) {
+  CHECK(mstate_new->inst.length == mstate_old->inst.length);
+
+  CopyInstructionInternal(mstate_old->inst.maddr,
+                          mstate_new->inst.maddr,
+                          mstate_old->inst.length);
+}
+
 int NCCopyCode(uint8_t *dst, uint8_t *src, NaClPcAddress vbase,
                size_t sz, int bundle_size) {
   struct NCValidatorState *vstate;
@@ -226,3 +247,44 @@
   return 0;
 }
 
+#elif NACL_TARGET_SUBARCH == 64
+
+int NaClCopyCodeIter(uint8_t *dst, uint8_t *src,
+                     NaClPcAddress vbase, size_t size) {
+  NaClSegment segment_old, segment_new;
+  NaClInstIter *iter_old, *iter_new;
+  NaClInstState *istate_old, *istate_new;
+
+  NaClSegmentInitialize(dst, vbase, size, &segment_old);
+  NaClSegmentInitialize(src, vbase, size, &segment_new);
+
+  iter_old = NaClInstIterCreate(&segment_old);
+  iter_new = NaClInstIterCreate(&segment_new);
+  while (NaClInstIterHasNext(iter_old) &&
+         NaClInstIterHasNext(iter_new)) {
+    istate_old = NaClInstIterGetState(iter_old);

[bsy, 2010/12/15 04:06:50]

does this get pseudo-atomic sequences as a single NaClnstState?  comment on
assumptions made by this code... e.g., this is only used after
NaClValidateCodeReplacement

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

Comment added to clarify what we're doing.

On 2010/12/15 04:06:50, bsy wrote:

+    istate_new = NaClInstIterGetState(iter_new);
+    if (istate_old->length != istate_new->length ||
+        istate_new->vpc != istate_old->vpc) {
+      NaClLog(LOG_ERROR,
+              "Segment replacement: copied instructions misaligned\n");

[bsy, 2010/12/15 04:06:50]

is this precondition violation that should never happen, due to
NaClValidateCodeReplacement?  if so, a comment and a LOG_FATAL would be more
appropriate; if not, then NaClInstIterDestroys are necessary to properly clean
up.

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

Changed to LOG_FATAL and commented as this should be guaranteed not to happen.

On 2010/12/15 04:06:50, bsy wrote:

+      return 1;
+    }
+    CopyInstructionInternal(istate_old->mpc,

[bsy, 2010/12/15 04:06:50]

add a comment here that replacing all affected instructions' first bytes with
HLT, then a SerializeAllProcessors, then the rest of the bytes with the desired
replacement values, another SerializeAllProcessors, then replace the first byte
of each instruction with the desired values would save overhead, rather than
doing 2 calls to SerializeAllProcessors per instruction.

[elijahtaylor (use chromium), 2010/12/15 21:26:37]

Done.

On 2010/12/15 04:06:50, bsy wrote:

+                            istate_new->mpc,
+                            istate_old->length);
+    NaClInstIterAdvance(iter_old);
+    NaClInstIterAdvance(iter_new);
+  }
+
+  CHECK(!NaClInstIterHasNext(iter_old) && !NaClInstIterHasNext(iter_new));
+
+  NaClInstIterDestroy(iter_old);
+  NaClInstIterDestroy(iter_new);
+  return 0;
+}
+
+#else
+#error "Unknown Platform"
+#endif
+