tmedicci commented on code in PR #19772:
URL: https://github.com/apache/nuttx/pull/19772#discussion_r3758151828


##########
arch/xtensa/src/esp32s3/esp32s3_wcl.c:
##########
@@ -0,0 +1,127 @@
+/****************************************************************************
+ * arch/xtensa/src/esp32s3/esp32s3_wcl.c
+ *
+ * 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 <stdint.h>
+
+#include <nuttx/nuttx.h>
+
+#include "chip.h"
+#include "xtensa.h"
+#include "hardware/esp32s3_sensitive.h"
+#include "hardware/esp32s3_soc.h"
+#include "hardware/esp32s3_wcl_core.h"
+
+#include "esp32s3_wcl.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/* Maximum number of supported entry addresses */
+
+#define WCL_ENTRY_MAX       13
+
+/* Last value of the agreed sequence to be written to the address configured
+ * in WCL_CORE_0_MESSAGE_ADDR register.
+ */
+
+#define WCL_SEQ_LAST_VAL    6
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: esp32s3_wcl_set_vecbase
+ ****************************************************************************/
+
+void esp32s3_wcl_set_vecbase(enum esp32s3_pms_world_e world,
+                             uintptr_t vecbase)
+{
+  switch (world)
+    {
+      case PMS_WORLD_0:
+        {
+          modifyreg32(SENSITIVE_CORE_0_VECBASE_OVERRIDE_1_REG,
+                      SENSITIVE_CORE_0_VECBASE_OVERRIDE_WORLD0_VALUE_M,
+                      VALUE_TO_FIELD(vecbase >> 10,
+                            SENSITIVE_CORE_0_VECBASE_OVERRIDE_WORLD0_VALUE));
+        }
+        break;
+      case PMS_WORLD_1:
+        {
+          modifyreg32(SENSITIVE_CORE_0_VECBASE_OVERRIDE_2_REG,
+                      SENSITIVE_CORE_0_VECBASE_OVERRIDE_WORLD1_VALUE_M,
+                      VALUE_TO_FIELD(vecbase >> 10,
+                            SENSITIVE_CORE_0_VECBASE_OVERRIDE_WORLD1_VALUE));
+        }
+        break;
+      default:
+        {
+          PANIC();
+        }
+        break;
+    }
+
+  modifyreg32(SENSITIVE_CORE_0_VECBASE_OVERRIDE_1_REG,
+              SENSITIVE_CORE_0_VECBASE_OVERRIDE_SEL_M,
+              VALUE_TO_FIELD(0x3, SENSITIVE_CORE_0_VECBASE_OVERRIDE_SEL));
+
+  modifyreg32(SENSITIVE_CORE_0_VECBASE_OVERRIDE_0_REG,
+              SENSITIVE_CORE_0_VECBASE_WORLD_MASK_M, 0);
+}
+
+/****************************************************************************
+ * Name: esp32s3_wcl_set_world0_entry

Review Comment:
   Can you provide a complete description? (please check other functions)



##########
arch/xtensa/src/esp32s3/esp32s3_addrenv.c:
##########
@@ -0,0 +1,760 @@
+/****************************************************************************
+ * arch/xtensa/src/esp32s3/esp32s3_addrenv.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 <sched.h>
+#include <string.h>
+
+#include <nuttx/addrenv.h>
+#include <nuttx/arch.h>
+#include <nuttx/irq.h>
+#include <nuttx/pgalloc.h>
+#include <nuttx/sched.h>
+
+#include "soc/ext_mem_defs.h"
+
+#include "esp32s3_addrenv.h"
+#include "esp32s3_mmu.h"
+#include "esp32s3_spiram.h"
+
+#ifdef CONFIG_ARCH_ADDRENV
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+/* The ESP32-S3 cache MMU is a single global remap table: only one user
+ * address environment can be resident in the shared .text/.data/.heap
+ * windows at a time.  Track which one it is so up_addrenv_select() can skip
+ * the (expensive) remap when the incoming environment is already active --
+ * the common thread<->thread, ISR and syscall case.
+ */
+
+static const arch_addrenv_t *g_current_addrenv;
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: alloc_region
+ *
+ * Description:
+ *   Allocate and wipe the physical page-pool (PSRAM) pages that back a
+ *   single user region, recording them in the caller's page array.  On the
+ *   ESP32-S3 the physical pages are only recorded here; the global cache-MMU
+ *   table is (re)programmed lazily in up_addrenv_select().
+ *
+ * Input Parameters:
+ *   pages    - Destination page array (physical addresses)
+ *   maxpages - Capacity of the page array
+ *   size     - Region size in bytes
+ *   count    - Receives the number of pages actually allocated
+ *
+ * Returned Value:
+ *   Zero (OK) on success; a negated errno value on failure.  On failure
+ *   *count reflects the pages allocated so far so the caller can free them.
+ *
+ ****************************************************************************/
+
+static int alloc_region(uintptr_t *pages, unsigned int maxpages, size_t size,
+                        uint16_t *count)
+{
+  unsigned int npages = MM_NPAGES(size);
+  unsigned int i;
+
+  *count = 0;
+
+  if (npages > maxpages)
+    {
+      berr("ERROR: region needs %u pages, only %u available\n",
+           npages, maxpages);
+      return -E2BIG;
+    }
+
+  for (i = 0; i < npages; i++)
+    {
+      uintptr_t paddr = mm_pgalloc(1);
+      if (paddr == 0)
+        {
+          berr("ERROR: page pool exhausted at page %u of %u\n", i, npages);
+          *count = i;
+          return -ENOMEM;
+        }
+
+      esp32s3_pgwipe(paddr);
+      pages[i] = paddr;
+    }
+
+  *count = npages;
+  return OK;
+}
+
+/****************************************************************************
+ * Name: free_region
+ *
+ * Description:
+ *   Return every page recorded in a region's page array to the page pool.
+ *
+ ****************************************************************************/
+
+static void free_region(uintptr_t *pages, uint16_t *count)
+{
+  uint16_t i;
+
+  for (i = 0; i < *count; i++)
+    {
+      if (pages[i] != 0)
+        {
+          mm_pgfree(pages[i], 1);
+          pages[i] = 0;
+        }
+    }
+
+  *count = 0;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_addrenv_create
+ *
+ * Description:
+ *   This function is called when a new task is created in order to
+ *   instantiate an address environment for the new task group.  Physical
+ *   pages for .text, .data/.bss and the heap are allocated from the PSRAM
+ *   page pool and recorded in 'addrenv'.  The reserved OS region (heap MM
+ *   bookkeeping and signal delivery support) occupies the first page of the
+ *   data window.
+ *
+ ****************************************************************************/
+
+int up_addrenv_create(size_t textsize, size_t datasize, size_t heapsize,
+                      arch_addrenv_t *addrenv)
+{
+  size_t datatotal;
+  int    ret;
+
+  DEBUGASSERT(addrenv);
+  DEBUGASSERT(MM_ISALIGNED(CONFIG_ARCH_TEXT_VBASE));
+  DEBUGASSERT(MM_ISALIGNED(CONFIG_ARCH_DATA_VBASE));
+  DEBUGASSERT(MM_ISALIGNED(CONFIG_ARCH_HEAP_VBASE));
+
+  /* Start from a clean slate */
+
+  memset(addrenv, 0, sizeof(arch_addrenv_t));
+
+  /* The data window carries the OS reserve at its base, followed by the
+   * task's .data/.bss.  vdata is therefore reported past the reserve.
+   */
+
+  datatotal = MM_PGALIGNUP(ARCH_DATA_RESERVE_SIZE) + datasize;
+
+  addrenv->textvbase = CONFIG_ARCH_TEXT_VBASE;
+  addrenv->datavbase = CONFIG_ARCH_DATA_VBASE +
+                       MM_PGALIGNUP(ARCH_DATA_RESERVE_SIZE);
+  addrenv->heapvbase = CONFIG_ARCH_HEAP_VBASE;
+  addrenv->heapsize  = heapsize;
+
+  /* Allocate the backing pages for each region */
+
+  ret = alloc_region(addrenv->textpages, CONFIG_ARCH_TEXT_NPAGES, textsize,
+                     &addrenv->ntext);
+  if (ret < 0)
+    {
+      goto errout;
+    }
+
+  ret = alloc_region(addrenv->datapages, CONFIG_ARCH_DATA_NPAGES, datatotal,
+                     &addrenv->ndata);
+  if (ret < 0)
+    {
+      goto errout;
+    }
+
+  ret = alloc_region(addrenv->heappages, CONFIG_ARCH_HEAP_NPAGES, heapsize,
+                     &addrenv->nheap);
+  if (ret < 0)
+    {
+      goto errout;
+    }
+
+  return OK;
+
+errout:
+  up_addrenv_destroy(addrenv);
+  return ret;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_destroy
+ *
+ * Description:
+ *   This function is called when a task group is finally deleted.  Return
+ *   all of the group's physical pages to the page pool.
+ *
+ ****************************************************************************/
+
+int up_addrenv_destroy(arch_addrenv_t *addrenv)
+{
+  DEBUGASSERT(addrenv);
+
+  /* If this environment is the resident one, forget it so a later select of
+   * a different environment that happens to reuse this address does not take
+   * the fast path by mistake.
+   */
+
+  if (addrenv == g_current_addrenv)
+    {
+      g_current_addrenv = NULL;
+    }
+
+  free_region(addrenv->textpages, &addrenv->ntext);
+  free_region(addrenv->datapages, &addrenv->ndata);
+  free_region(addrenv->heappages, &addrenv->nheap);
+
+  memset(addrenv, 0, sizeof(arch_addrenv_t));
+  return OK;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_vtext
+ *
+ * Description:
+ *   Return the virtual address associated with the newly created .text
+ *   address environment.
+ *
+ ****************************************************************************/
+
+int up_addrenv_vtext(arch_addrenv_t *addrenv, void **vtext)
+{
+  DEBUGASSERT(addrenv && vtext);
+  *vtext = (void *)addrenv->textvbase;
+  return OK;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_vdata
+ *
+ * Description:
+ *   Return the virtual address associated with the newly created .bss/.data
+ *   address environment.
+ *
+ ****************************************************************************/
+
+int up_addrenv_vdata(arch_addrenv_t *addrenv, uintptr_t textsize,
+                     void **vdata)
+{
+  DEBUGASSERT(addrenv && vdata);
+  *vdata = (void *)addrenv->datavbase;
+  return OK;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_vheap
+ *
+ * Description:
+ *   Return the heap virtual address associated with the newly created
+ *   address environment.
+ *
+ ****************************************************************************/
+
+int up_addrenv_vheap(const arch_addrenv_t *addrenv, void **vheap)
+{
+  DEBUGASSERT(addrenv && vheap);
+  *vheap = (void *)addrenv->heapvbase;
+  return OK;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_heapsize
+ *
+ * Description:
+ *   Return the size of the initial heap allocation.
+ *
+ ****************************************************************************/
+
+ssize_t up_addrenv_heapsize(const arch_addrenv_t *addrenv)
+{
+  DEBUGASSERT(addrenv);
+  return (ssize_t)addrenv->heapsize;
+}
+
+/****************************************************************************
+ * Name: up_addrenv_select
+ *
+ * Description:
+ *   After an address environment has been established for a task group (via
+ *   up_addrenv_create()), this function may be called to instantiate that
+ *   address environment in the virtual address space.  On the ESP32-S3 there
+ *   is no page-table-base register to load; instead the shared user
+ *   cache-MMU windows (.text on the instruction bus, .data/.bss and heap on
+ *   the data bus) are reprogrammed to point at this environment's PSRAM
+ *   pages.  Only one user environment can be resident at a time, so while a
+ *   group runs only its own pages are visible *in the windows*: the entries
+ *   it uses are pointed at its pages and the rest are invalidated.
+ *
+ *   That is not by itself isolation between groups.  Every user page comes
+ *   from the one page pool, which the kernel keeps permanently mapped at
+ *   CONFIG_ARCH_PGPOOL_VBASE, and the external-memory permissions are
+ *   indexed by physical address, so they cannot deny the unprivileged world
+ *   that window without also denying it its own pages.  Closing that is a
+ *   separate piece of work on the kernel's side of the map.
+ *
+ *   The remap is skipped when 'addrenv' is already the resident environment
+ *   (thread<->thread within a group, ISRs, syscalls), which pays nothing.
+ *
+ * Input Parameters:
+ *   addrenv - Describes the address environment to instantiate.
+ *
+ * Returned Value:
+ *   Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+int up_addrenv_select(const arch_addrenv_t *addrenv)

Review Comment:
   Although a single address environment is enabled at a time, nothing prevents 
the userspace process from accessing the MMU registers and changing the 
mapping. Additionally, there is no protection against accessing peripherals 
from the user process. That being said, can't we use WCL and PMS to prevent 
that? Userspace applications should never access peripherals (MMU mapping 
included) directly. That would bring better process isolation and make it 
adherent to NuttX's userspace.



-- 
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