casaroli commented on code in PR #19795:
URL: https://github.com/apache/nuttx/pull/19795#discussion_r3879684331


##########
arch/xtensa/src/common/xtensa_fork.c:
##########
@@ -0,0 +1,455 @@
+/****************************************************************************
+ * arch/xtensa/src/common/xtensa_fork.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <assert.h>
+#include <debug.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <arch/syscall.h>
+
+#include <nuttx/arch.h>
+#include <nuttx/compiler.h>
+#include <nuttx/sched.h>
+
+#include "sched/sched.h"
+#include "xtensa.h"
+#include "chip_macros.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/* The base save area:  the 16 bytes below a frame's stack pointer, holding
+ * the spilled a0-a3 of that frame's caller.  Window overflow writes them and
+ * underflow reads them back, so it is the link that makes the frame chain
+ * walkable, and the reason a copy starting at the stack pointer is missing
+ * its first link.
+ */
+
+#define BASE_SAVE_AREA   16
+#define BASE_SAVE_A1     1   /* a0, a1, a2, a3 -- a1 is the caller's SP */
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* Everything a child is built from:  the register context of the thread that
+ * called, and where that thread was.  There are two ways to come by it, and
+ * they differ in more than provenance -- see xtensa_fork_direct() below.
+ */
+
+struct fork_snapshot_s
+{
+  FAR const uint32_t *regs;  /* The caller's full register context */
+  uintptr_t           usp;   /* The caller's user stack pointer */
+  uintptr_t           pc;    /* Where the child resumes */
+  uint32_t            a2;    /* What the child sees returned in A2 */
+#ifndef CONFIG_BUILD_FLAT
+  uintptr_t           ctx;   /* The caller's privilege, from its syscall */
+#endif
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: xtensa_fork_rebase
+ *
+ * Description:
+ *   Rebase the frame chain of a relocated stack copy.
+ *
+ *   A windowed ABI stores each frame's caller stack pointer absolutely, in
+ *   the base save area at [sp - 16), so a copy taken at a different address
+ *   still names the parent and the child's first retw would underflow onto
+ *   the parent's stack.  Each link gets the relocation offset added.
+ *
+ *   Spilled a4-a15 and any other stack addresses in the copy are left alone.
+ *   Those are data;  the chain is what the child needs to return at all.
+ *
+ * Input Parameters:
+ *   newsp    - The child's stack pointer
+ *   usp      - The parent's stack pointer, which newsp is a relocation of
+ *   stacktop - The top of the parent's stack; the walk ends there
+ *   offset   - newsp - usp, the amount every link moves by
+ *
+ ****************************************************************************/
+
+static void xtensa_fork_rebase(uintptr_t newsp, uintptr_t usp,
+                               uintptr_t stacktop, intptr_t offset)
+{
+  uintptr_t csp = newsp;   /* The frame being fixed, in the child's copy */
+  uintptr_t psp = usp;     /* The same frame, as the parent addresses it */
+
+  while (psp < stacktop)
+    {
+      FAR uint32_t *save = (FAR uint32_t *)(csp - BASE_SAVE_AREA);
+      uintptr_t caller = save[BASE_SAVE_A1];
+
+      /* The chain grows towards the top of the stack and ends there.  Stop
+       * on anything else rather than following it:  the outermost frame's
+       * save area was never written by an overflow, so what is in it is
+       * whatever the stack was coloured with.
+       */
+
+      if (caller <= psp || caller > stacktop)
+        {
+          break;
+        }
+
+      save[BASE_SAVE_A1] = (uint32_t)(uintptr_t)((intptr_t)caller + offset);
+
+      psp = caller;
+      csp = (uintptr_t)((intptr_t)caller + offset);
+    }
+}
+
+/****************************************************************************
+ * Name: xtensa_fork_stack
+ *
+ * Description:
+ *   Give the child its stack pointer, copying the parent's frames if needed.
+ *
+ *   A fork() child keeps the parent's stack addresses inside its own address
+ *   environment, so it has nothing to copy.  A vfork() child gets a stack of
+ *   its own, which needs the copy and xtensa_fork_rebase() with it.
+ *
+ *   The copy starts one base save area below the stack pointer:  the frame
+ *   the child resumes into keeps its caller's spilled a0-a3 there.
+ *
+ * Input Parameters:
+ *   parent - The parent task's TCB
+ *   child  - The child task's TCB
+ *   usp    - The parent's stack pointer
+ *
+ * Returned Value:
+ *   The child's stack pointer.
+ *
+ ****************************************************************************/
+
+static uintptr_t xtensa_fork_stack(FAR struct tcb_s *parent,
+                                   FAR struct tcb_s *child,
+                                   uintptr_t usp)
+{
+  uintptr_t stacktop;
+  uintptr_t stackutil;
+  uintptr_t newtop;
+  uintptr_t newsp;
+
+  stacktop = (uintptr_t)parent->stack_base_ptr + parent->adj_stack_size;
+  DEBUGASSERT(stacktop > usp);
+
+  if (child->stack_base_ptr == parent->stack_base_ptr)
+    {
+      /* The child is running at the parent's stack addresses, inside its
+       * own duplicated address environment.  There is nothing to relocate:
+       * every stack address the child inherits is still the address it
+       * names.
+       */
+
+      return usp;
+    }
+
+  DEBUGASSERT(usp - BASE_SAVE_AREA >= (uintptr_t)parent->stack_base_ptr);
+
+  stackutil = stacktop - (usp - BASE_SAVE_AREA);
+  newtop    = (uintptr_t)child->stack_base_ptr + child->adj_stack_size;
+
+  /* The copy has to fit, and the register save area goes below it when the
+   * child has no kernel stack to put it on -- see xtensa_fork().
+   */
+
+  DEBUGASSERT(newtop - stackutil >
+              (uintptr_t)child->stack_base_ptr + XCPTCONTEXT_SIZE);
+
+  newsp = newtop - stackutil + BASE_SAVE_AREA;
+
+  memcpy((FAR void *)(newsp - BASE_SAVE_AREA),
+         (FAR const void *)(usp - BASE_SAVE_AREA), stackutil);
+
+  xtensa_fork_rebase(newsp, usp, stacktop, (intptr_t)(newsp - usp));
+
+  return newsp;
+}
+
+/****************************************************************************
+ * Name: xtensa_fork
+ *
+ * Description:
+ *   The common core of the two primitives.  They differ in the flag handed
+ *   to nxtask_setup_fork(), which is where the memory semantics are decided,
+ *   and in where their snapshot of the caller comes from.
+ *
+ * Input Parameters:
+ *   vfork - true for vfork(), false for fork()
+ *   snap  - The caller's context; see struct fork_snapshot_s
+ *
+ * Returned Value:
+ *   The pid of the child, or ERROR on failure.
+ *
+ ****************************************************************************/
+
+static pid_t xtensa_fork(bool vfork, FAR const struct fork_snapshot_s *snap)
+{
+  FAR struct tcb_s *parent = this_task();
+  FAR struct tcb_s *child;
+  uintptr_t newsp;
+  uintptr_t regstop;
+
+  DEBUGASSERT(snap->regs != NULL && snap->pc != 0);
+
+  /* Allocate and initialise a TCB for the child.  The start address is only
+   * bookkeeping here: what the child actually resumes with is the register
+   * context assembled below.
+   */
+
+  child = nxtask_setup_fork((start_t)snap->pc, vfork);
+  if (child == NULL)
+    {
+      sinfo("nxtask_setup_fork failed\n");
+      return (pid_t)ERROR;

Review Comment:
   `nxtask_setup_fork()` already sets errno, and `up_fork()` is documented to 
return -1 with errno set, so I think this is right as is. arm, arm64 and x86_64 
all return `(pid_t)ERROR` here too.
   
   I will switch to `serr("ERROR: ...")` instead of `sinfo`.
   
   or did you mean something else?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to