xiaoxiang781216 commented on code in PR #19772:
URL: https://github.com/apache/nuttx/pull/19772#discussion_r3757513278
##########
arch/Kconfig:
##########
@@ -493,6 +493,7 @@ config ARCH_HAVE_VFORK
config ARCH_HAVE_FORK
bool
+ default y if ARCH_XTENSA && BUILD_KERNEL
Review Comment:
let's select under ARCH_XTENSA
##########
arch/xtensa/src/esp32s3/esp32s3_addrenv.c:
##########
@@ -577,6 +579,153 @@ int up_addrenv_clone(const arch_addrenv_t *src,
arch_addrenv_t *dest)
return OK;
}
+/****************************************************************************
+ * Name: copy_region
+ *
+ * Description:
+ * Copy one region's pages from a parent environment into a child's. Both
+ * sides are reached through the kernel's scratch region, which is why it
+ * has two slots: source and destination are mapped at the same time so
+ * this is one memcpy rather than a bounce through kernel memory.
+ *
+ * sched_lock() is held across each page pair for the reason every scratch
+ * mapping is: those addresses are ordinary external memory to the
+ * permission control, so no unprivileged task may run while a page of
+ * somebody's memory is parked at one.
+ *
+ ****************************************************************************/
+
+static int copy_region(const uintptr_t *src, uintptr_t *dest, uint16_t count)
+{
+ uint16_t i;
+
+ for (i = 0; i < count; i++)
+ {
+ uintptr_t svaddr;
+ uintptr_t dvaddr;
+
+ sched_lock();
+
+ svaddr = esp32s3_pgmap(src[i]);
+ dvaddr = esp32s3_pgmap(dest[i]);
+
+ if (svaddr == 0 || dvaddr == 0)
+ {
+ if (svaddr != 0)
+ {
+ esp32s3_pgunmap(svaddr);
+ }
+
+ if (dvaddr != 0)
+ {
+ esp32s3_pgunmap(dvaddr);
+ }
+
+ sched_unlock();
+ berr("ERROR: no scratch mapping for page %u\n", i);
+ return -EFAULT;
+ }
+
+ memcpy((void *)dvaddr, (const void *)svaddr, MM_PGSIZE);
+
+ esp32s3_pgunmap(dvaddr);
+ esp32s3_pgunmap(svaddr);
+
+ sched_unlock();
+ }
+
+ return OK;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_fork
+ *
+ * Description:
+ * Duplicate an address environment for fork(): allocate the child pages
+ * to match the parent's regions and copy the parent's contents into them.
+ *
+ * The copy is eager and complete. There is no copy-on-write and no demand
+ * fill, because this chip provides no synchronous restartable write fault
+ * to build them on -- proven, not assumed. So a fork costs a full copy of
+ * the process image.
+ *
+ * The child's pages land at the same *virtual* addresses as the parent's,
+ * which is the property the whole thing rests on: a copied stack is full
+ * of pointers into itself, and they are only still correct because the
+ * copy is addressed identically.
+ *
+ ****************************************************************************/
+
+int up_addrenv_fork(const arch_addrenv_t *src, arch_addrenv_t *dest)
Review Comment:
could we move to common and generalize it instead
##########
arch/xtensa/src/common/xtensa_fork.c:
##########
@@ -0,0 +1,488 @@
+/****************************************************************************
+ * arch/xtensa/src/common/xtensa_fork.c
Review Comment:
could you split the pr into small ones:
1. vfork
2. mmu in common arch code
3. code in esp32
##########
arch/xtensa/src/esp32s3/esp32s3_pgalloc.c:
##########
@@ -0,0 +1,227 @@
+/****************************************************************************
+ * arch/xtensa/src/esp32s3/esp32s3_pgalloc.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 <inttypes.h>
+
+#include <nuttx/addrenv.h>
+#include <nuttx/arch.h>
+#include <nuttx/pgalloc.h>
+#include <nuttx/sched.h>
+
+#include "sched/sched.h"
+
+#include "esp32s3_addrenv.h"
+
+#ifdef CONFIG_ESP32S3_SPIRAM
+# include "esp32s3_spiram.h"
+#endif
+
+#ifdef CONFIG_MM_PGALLOC
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_allocate_pgheap
+ *
+ * Description:
+ * If there is a page allocator in the configuration, then this function
+ * must be provided by the platform-specific code. The OS initialization
+ * logic will call this function early in the initialization sequence to
+ * get the page heap information needed to configure the page allocator.
+ *
+ * On the ESP32-S3 the page pool is a slice of the external octal PSRAM.
+ * The pool is described by its *physical* base -- for the cache MMU that
+ * is the zero-based offset into the PSRAM device -- because mm_pgalloc()
+ * hands out physical page addresses. The whole pool is also permanently
+ * mapped into the kernel (WORLD0) data-bus window at
+ * CONFIG_ARCH_PGPOOL_VBASE so the kernel can reach any page it allocates
+ * (see esp32s3_pgvaddr()).
+ *
+ * Input Parameters:
+ * heap_start - Receives the physical base address of the page pool.
+ * heap_size - Receives the size of the page pool in bytes.
+ *
+ ****************************************************************************/
+
+void up_allocate_pgheap(void **heap_start, size_t *heap_size)
Review Comment:
can we move the most code into common folder
##########
arch/xtensa/src/common/crt0.c:
##########
@@ -55,6 +55,56 @@ int main(int argc, char *argv[]);
* Private Functions
****************************************************************************/
+#ifdef CONFIG_BUILD_KERNEL
+
+/****************************************************************************
+ * Name: sig_trampoline
+ *
+ * Description:
+ * The user-space signal handler trampoline. A kernel build cannot reach
+ * the one in xtensa_signal_handler.S -- that lives in libarch, which user
+ * programs do not link -- so it is carried here in crt0 instead, and
+ * _start() publishes it to the kernel through ARCH_DATA_RESERVE. The
+ * kernel enters it from the SYS_signal_handler case of xtensa_swint().
+ *
+ * Written as file-scope assembly rather than as a naked function because
+ * GCC does not implement the naked attribute on Xtensa: it would emit a
+ * window-rotating prologue and quietly invalidate the register assignments
+ * below.
+ *
+ * Input Parameters:
+ * a2 = sighand, the user-space signal handling function
+ * a3, a4, a5 = signo, info and ucontext, its arguments
+ *
+ * Returned Value:
+ * None. This function does not return in the normal sense; it returns
+ * via the SYS_signal_handler_return syscall.
+ *
+ ****************************************************************************/
+
+#define _SIGTRAMP_STR(x) #x
+#define _SIGTRAMP_XSTR(x) _SIGTRAMP_STR(x)
Review Comment:
use macro from nuttx/macro.h
##########
arch/xtensa/src/common/xtensa_addrenv_kstack.c:
##########
@@ -0,0 +1,129 @@
+/****************************************************************************
+ * arch/xtensa/src/common/xtensa_addrenv_kstack.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 <nuttx/arch.h>
+#include <nuttx/kmalloc.h>
+#include <nuttx/sched.h>
+
+#include "xtensa.h"
+
+#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_ARCH_KERNEL_STACK)
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/* The Xtensa windowed ABI requires 16-byte stack alignment */
+
+#define KSTACK_ALIGNMENT 16
+#define KSTACK_ALIGN_DOWN(a) ((a) & ~(KSTACK_ALIGNMENT - 1))
Review Comment:
why not use STACK_ALIGNMENT from xtensa_internal.h
##########
arch/xtensa/src/esp32s3/esp32s3_pgalloc.c:
##########
@@ -76,40 +206,62 @@ void up_allocate_pgheap(void **heap_start, size_t
*heap_size)
{
DEBUGASSERT(heap_start && heap_size);
-#ifdef CONFIG_ESP32S3_SPIRAM
+#if defined(CONFIG_ESP32S3_SPIRAM) && defined(CONFIG_ARCH_ADDRENV)
/* Where the kernel's PSRAM window lands is decided at run time:
* esp32s3_spiram.c maps PSRAM immediately after the last cache-MMU entry
* the flash mappings occupy, so it moves as the kernel image grows. The
- * page pool is described to the OS by compile-time constants, so the two
- * have to be checked against each other -- and loudly, because getting it
- * wrong is otherwise silent: an unmapped cache window swallows writes and
- * reads back as zero without faulting, so a misplaced pool would simply
- * lose every page handed out of it.
+ * pool is described by a compile-time *physical* base, so ask the cache
+ * MMU where that physical page currently is rather than deriving it from
+ * a constant -- which is how CONFIG_ARCH_PGPOOL_PBASE went stale twice,
+ * and the second time by exactly one page, which left one unwiped page in
+ * every region of every new process.
*/
{
uintptr_t ramstart = (uintptr_t)esp_spiram_allocable_vaddr_start();
uintptr_t ramend = (uintptr_t)esp_spiram_allocable_vaddr_end();
+ uintptr_t poolvbase;
+ uint32_t rampbase;
- _info("PSRAM window %08" PRIxPTR "-%08" PRIxPTR ", "
- "page pool %08x-%08x\n",
- ramstart, ramend,
- CONFIG_ARCH_PGPOOL_VBASE, CONFIG_ARCH_PGPOOL_VEND);
+ if (!esp32s3_mmu_paddr(ramstart, &rampbase))
+ {
+ _err("ERROR: PSRAM window base %08" PRIxPTR " maps nothing\n",
+ ramstart);
+ PANIC();
+ }
- if ((uintptr_t)CONFIG_ARCH_PGPOOL_VBASE < ramstart ||
- (uintptr_t)CONFIG_ARCH_PGPOOL_VEND > ramend)
+ if (ESP32S3_PGPOOL_PBASE < rampbase ||
Review Comment:
why not continue use the general Kconfig
##########
arch/xtensa/src/common/xtensa_swint.c:
##########
@@ -296,6 +345,55 @@ int xtensa_swint(int irq, void *context, void *arg)
regs[REG_A3] = regs[REG_A4]; /* signal */
regs[REG_A4] = regs[REG_A5]; /* info */
regs[REG_A5] = regs[REG_A6]; /* ucontext */
+
+#ifdef CONFIG_ARCH_KERNEL_STACK
+ /* The handler runs in user mode, so it has to run on the user
+ * stack. Signal dispatch always reaches here on the thread's
+ * kernel stack -- up_schedule_sigaction() builds the dispatch
+ * context below the interrupted one -- so put that stack pointer
+ * aside and hand the thread its own stack back for the duration.
+ *
+ * Having a kernel stack at all is what says this is a user
+ * process. Testing xcp.ustkptr instead would be wrong: that
+ * holds the user stack pointer only while a system call is in
+ * progress, so a signal caught in user code would leave the
+ * handler running on the kernel stack.
+ */
+
+ if (rtcb->xcp.kstack != NULL)
+ {
+ uintptr_t usp;
+
+ rtcb->xcp.kstkptr = (uint32_t *)regs[REG_A1];
+
+ /* The thread's own stack pointer is the one the system call
+ * saved if it was in one, and otherwise the one it was
+ * interrupted with, which up_schedule_sigaction() kept.
+ */
+
+ usp = rtcb->xcp.ustkptr != NULL ?
+ (uintptr_t)rtcb->xcp.ustkptr :
+ (uintptr_t)rtcb->xcp.saved_regs[REG_A1];
+
+ /* The siginfo passed in lives on the kernel stack, which the
+ * handler must not reach -- and cannot, once the permission
+ * control is programmed. Copy it onto the user stack and
+ * hand the handler that copy.
+ *
+ * Skip the base save area the windowed ABI keeps in the
+ * 16 bytes below a stack pointer: it belongs to the frame
+ * that was interrupted.
+ */
+
+ usp = (usp - SIGTRAMP_SAVE_AREA - sizeof(siginfo_t)) &
+ ~(SIGTRAMP_STACK_ALIGN - 1);
Review Comment:
why not use STACK_ALIGMENT
##########
arch/xtensa/src/common/xtensa_addrenv_kstack.c:
##########
@@ -0,0 +1,129 @@
+/****************************************************************************
+ * arch/xtensa/src/common/xtensa_addrenv_kstack.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 <nuttx/arch.h>
+#include <nuttx/kmalloc.h>
+#include <nuttx/sched.h>
+
+#include "xtensa.h"
+
+#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_ARCH_KERNEL_STACK)
Review Comment:
remove
--
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]
