Merge branch 'bugfix/fix_potential_cache_msync_issue_on_psram_stack' into 'master'

cache: improve esp_cache_msync() test cases, added psram stack test

Closes IDF-7833

See merge request espressif/esp-idf!24837

components/esp_mm/test_apps/components/test_mm_utils/CMakeLists.txt

@@ -0,0 +1,4 @@
+set(srcs "test_cache_utils.c")
+
+idf_component_register(SRCS ${srcs}
+                       INCLUDE_DIRS include)

components/esp_mm/test_apps/components/test_mm_utils/include/test_mm_utils.h

@@ -0,0 +1,32 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#pragma once
+
+#include <esp_types.h>
+#include "esp_err.h"
+#include "esp_log.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+
+/**
+ * Set addr space dirty
+ *
+ * @param[in] vaddr_start  start addr of the space
+ * @param[in] size         size of the space
+ *
+ * @return
+ *        - ESP_OK
+ *        - ESP_ERR_INVALID_ARG: Currently no support for non-4B-aligned space
+ */
+esp_err_t test_set_buffer_dirty(intptr_t vaddr_start, size_t size);
+
+#ifdef __cplusplus
+}
+#endif

components/esp_mm/test_apps/components/test_mm_utils/test_cache_utils.c

@@ -0,0 +1,28 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <string.h>
+#include <inttypes.h>
+#include "test_mm_utils.h"
+
+const static char *TAG = "cache_utils";
+
+esp_err_t test_set_buffer_dirty(intptr_t vaddr_start, size_t size)
+{
+    if (((vaddr_start % 32) != 0) || ((size % 32) != 0)) {
+        ESP_LOGE(TAG, "addr not 4B aligned");
+        return ESP_ERR_INVALID_ARG;
+    }
+
+    uint32_t *vaddr = (uint32_t *)vaddr_start;
+    printf("vaddr: %p, size: 0x%zx\n", vaddr, size);
+
+    for (int i = 0; i < size / sizeof(uint32_t); i++) {
+        vaddr[i] = 0xcc;
+    }
+
+    return ESP_OK;
+}

components/esp_mm/test_apps/mm/CMakeLists.txt

@@ -1,7 +1,7 @@
 # This is the project CMakeLists.txt file for the test subproject
 cmake_minimum_required(VERSION 3.16)
 
-set(EXTRA_COMPONENT_DIRS "$ENV{IDF_PATH}/tools/unit-test-app/components")
+set(EXTRA_COMPONENT_DIRS "$ENV{IDF_PATH}/components/esp_mm/test_apps/components")
 
 include($ENV{IDF_PATH}/tools/cmake/project.cmake)
 project(mm_test)

components/esp_mm/test_apps/mm/main/CMakeLists.txt

@@ -10,5 +10,5 @@ endif()
 # In order for the cases defined by `TEST_CASE` to be linked into the final elf,
 # the component can be registered as WHOLE_ARCHIVE
 idf_component_register(SRCS ${srcs}
-                       PRIV_REQUIRES test_utils spi_flash esp_mm driver esp_timer
+                       PRIV_REQUIRES unity esp_partition spi_flash esp_mm driver esp_timer test_mm_utils
                        WHOLE_ARCHIVE)

components/esp_mm/test_apps/mm/main/test_cache_msync.c

@@ -13,6 +13,7 @@
 #include "unity.h"
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
+#include "freertos/semphr.h"
 #include "esp_rom_sys.h"
 #include "esp_memory_utils.h"
 #include "esp_heap_caps.h"
@@ -21,6 +22,7 @@
 #include "esp_timer.h"
 #include "esp_partition.h"
 #include "esp_flash.h"
+#include "test_mm_utils.h"
 
 const static char *TAG = "CACHE_TEST";
 
@@ -28,13 +30,13 @@ const static char *TAG = "CACHE_TEST";
 #define TEST_BUF                {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9}
 
 
+#define TEST_OFFSET       0x100000
 #if CONFIG_IDF_TARGET_ESP32S2
-#define TEST_SYNC_START   0x3F500000
+#define TEST_SYNC_START   (0x3F500000 + TEST_OFFSET)
-#define TEST_SYNC_SIZE    0xA80000
 #elif CONFIG_IDF_TARGET_ESP32S3
-#define TEST_SYNC_START   0x3C000000
+#define TEST_SYNC_START   (0x3C000000 + TEST_OFFSET)
-#define TEST_SYNC_SIZE    0x2000000
 #endif
+#define TEST_SYNC_SIZE    0x8000
 
 
 #define RECORD_TIME_PREPARE()   uint32_t __t1, __t2
@@ -42,82 +44,6 @@ const static char *TAG = "CACHE_TEST";
 #define RECORD_TIME_END(p_time) do{__t2 = esp_cpu_get_cycle_count(); p_time = (__t2 - __t1);} while(0)
 #define GET_US_BY_CCOUNT(t)     ((double)(t)/CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ)
 
-static const uint8_t s_test_buf[TEST_NUM] = TEST_BUF;
-static DRAM_ATTR bool diff_res;
-static DRAM_ATTR uint32_t s_check_times = 0;
-
-static void NOINLINE_ATTR IRAM_ATTR s_test_rodata_cb(void *arg)
-{
-    bool sync_flag = *(bool *)arg;
-
-    if (sync_flag) {
-        uint8_t cmp_buf[TEST_NUM] = TEST_BUF;
-        for (int i = 0; i < TEST_NUM; i++) {
-            if (cmp_buf[i] != s_test_buf[i]) {
-                diff_res |= true;
-            }
-        }
-
-        s_check_times++;
-    }
-}
-
-/**
- * This test tests if the esp_cache_msync() suspending CPU->Cache access is short enough
- * 1. Register an IRAM callback, but access rodata inside the callback
- * 2. esp_cache_msync() will suspend the CPU access to the cache
- * 3. Therefore the rodata access in `s_test_rodata_cb()` should be blocked, most of the times
- * 4. Note if the callback frequency is less, there might be few successful rodata access, as code execution needs time
- */
-TEST_CASE("test cache msync short enough when suspending an ISR", "[cache]")
-{
-    uint32_t sync_time = 0;
-    uint32_t sync_time_us = 20;
-    RECORD_TIME_PREPARE();
-
-    //Do msync first, as the first writeback / invalidate takes long time, next msyncs will be shorter and they keep unchanged almost
-    RECORD_TIME_START();
-    TEST_ESP_OK(esp_cache_msync((void *)TEST_SYNC_START, TEST_SYNC_SIZE, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE));
-    RECORD_TIME_END(sync_time);
-    sync_time_us = GET_US_BY_CCOUNT(sync_time);
-    printf("first sync_time_us: %"PRId32"\n", sync_time_us);
-
-    RECORD_TIME_START();
-    TEST_ESP_OK(esp_cache_msync((void *)TEST_SYNC_START, TEST_SYNC_SIZE, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE));
-    RECORD_TIME_END(sync_time);
-    sync_time_us = GET_US_BY_CCOUNT(sync_time);
-    printf("sync_time_us: %"PRId32"\n", sync_time_us);
-
-    bool sync_flag = false;
-    esp_timer_handle_t timer;
-    const esp_timer_create_args_t oneshot_timer_args = {
-        .callback = &s_test_rodata_cb,
-        .arg = &sync_flag,
-        .dispatch_method = ESP_TIMER_ISR,
-        .name = "test_ro_suspend"
-    };
-    TEST_ESP_OK(esp_timer_create(&oneshot_timer_args, &timer));
-
-    uint32_t period = sync_time_us / 2;
-    TEST_ESP_OK(esp_timer_start_periodic(timer, period));
-
-    RECORD_TIME_START();
-    sync_flag = true;
-    TEST_ESP_OK(esp_cache_msync((void *)TEST_SYNC_START, TEST_SYNC_SIZE, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE | ESP_CACHE_MSYNC_FLAG_UNALIGNED));
-    sync_flag = false;
-    RECORD_TIME_END(sync_time);
-
-    TEST_ESP_OK(esp_timer_stop(timer));
-    printf("s_check_times: %"PRId32"\n", s_check_times);
-    sync_time_us = GET_US_BY_CCOUNT(sync_time);
-    printf("sync time: %"PRId32" us\n", sync_time_us);
-    TEST_ASSERT((s_check_times < (sync_time_us / period)));
-    TEST_ASSERT(diff_res == false);
-
-    ESP_LOGI(TAG, "Finish");
-    TEST_ESP_OK(esp_timer_delete(timer));
-}
-
 
 static void s_test_with_msync_cb(void *arg)
 {
@@ -131,6 +57,12 @@ TEST_CASE("test cache msync short enough to be in an ISR", "[cache]")
     uint32_t sync_time_us = 200;
     RECORD_TIME_PREPARE();
 
+#if CONFIG_SPIRAM
+    //prepare the cache
+    TEST_ESP_OK(test_set_buffer_dirty(TEST_SYNC_START, TEST_SYNC_SIZE));
+#endif
+
+    //do once to record time
     RECORD_TIME_START();
     TEST_ESP_OK(esp_cache_msync((void *)TEST_SYNC_START, TEST_SYNC_SIZE, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE | ESP_CACHE_MSYNC_FLAG_UNALIGNED));
     RECORD_TIME_END(sync_time);
@@ -146,11 +78,17 @@ TEST_CASE("test cache msync short enough to be in an ISR", "[cache]")
     };
     TEST_ESP_OK(esp_timer_create(&oneshot_timer_args, &timer));
 
+#if CONFIG_SPIRAM
+    //prepare the cache
+    TEST_ESP_OK(test_set_buffer_dirty(TEST_SYNC_START, TEST_SYNC_SIZE));
+#endif
+
+    //start timer
     uint32_t period = sync_time_us * 2;
     TEST_ESP_OK(esp_timer_start_periodic(timer, period));
 
-    //1ms
+    //10ms
-    esp_rom_delay_us(1000);
+    esp_rom_delay_us(10 * 1000);
     TEST_ESP_OK(esp_timer_stop(timer));
 
     ESP_LOGI(TAG, "Finish");
@@ -174,6 +112,10 @@ TEST_CASE("test cache msync work with Flash operation when XIP from PSRAM", "[ca
     uint32_t sync_time = 0;
     RECORD_TIME_PREPARE();
 
+    //prepare the cache
+    TEST_ESP_OK(test_set_buffer_dirty(TEST_SYNC_START, TEST_SYNC_SIZE));
+
+    //do once to record time
     RECORD_TIME_START();
     TEST_ESP_OK(esp_cache_msync((void *)TEST_SYNC_START, TEST_SYNC_SIZE, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE));
     RECORD_TIME_END(sync_time);
@@ -195,9 +137,14 @@ TEST_CASE("test cache msync work with Flash operation when XIP from PSRAM", "[ca
     };
     TEST_ESP_OK(esp_timer_create(&oneshot_timer_args, &timer));
 
+    //prepare the cache
+    TEST_ESP_OK(test_set_buffer_dirty(TEST_SYNC_START, TEST_SYNC_SIZE));
+
+    //start timer
     uint32_t period = sync_time_us * 2;
     TEST_ESP_OK(esp_timer_start_periodic(timer, period));
 
+    //erase
     ESP_ERROR_CHECK(esp_flash_erase_region(part->flash_chip, part->address, part->size));
 
     TEST_ESP_OK(esp_timer_stop(timer));
@@ -205,3 +152,54 @@ TEST_CASE("test cache msync work with Flash operation when XIP from PSRAM", "[ca
     TEST_ESP_OK(esp_timer_delete(timer));
 }
 #endif  //#if CONFIG_SPIRAM_FETCH_INSTRUCTIONS && CONFIG_SPIRAM_RODATA
+
+
+#if CONFIG_SPIRAM
+/*---------------------------------------------------------------
+        Test esp_cache_msync with PSRAM stack
+---------------------------------------------------------------*/
+static void test_msync_on_psram(void *arg)
+{
+    SemaphoreHandle_t test_semphr = *(SemaphoreHandle_t *)arg;
+    extern int _instruction_reserved_end;
+    extern int _rodata_reserved_end;
+    esp_rom_printf("_instruction_reserved_end: %p\n", &_instruction_reserved_end);
+    esp_rom_printf("_rodata_reserved_end: %p\n", &_rodata_reserved_end);
+
+    StackType_t *start_addr_stack = esp_cpu_get_sp();
+    TEST_ASSERT(esp_ptr_external_ram(start_addr_stack));
+
+    TEST_ESP_OK(test_set_buffer_dirty(TEST_SYNC_START, TEST_SYNC_SIZE));
+
+    uint32_t sync_time = 0;
+    RECORD_TIME_PREPARE();
+
+    printf("doing msync...\n");
+    RECORD_TIME_START();
+    TEST_ESP_OK(esp_cache_msync((void *)TEST_SYNC_START, 0x8000, ESP_CACHE_MSYNC_FLAG_INVALIDATE));
+    RECORD_TIME_END(sync_time);
+    printf("msync done\n");
+
+    uint32_t sync_time_us = GET_US_BY_CCOUNT(sync_time);
+    printf("sync_time_us: %"PRId32"\n", sync_time_us);
+
+    xSemaphoreGive(test_semphr);
+    vTaskDelete(NULL);
+}
+
+TEST_CASE("test cache msync work with PSRAM stack", "[cache]")
+{
+    SemaphoreHandle_t test_semphr = xSemaphoreCreateBinary();
+    TEST_ASSERT(test_semphr);
+
+    int size_stack = 1024 * 4;
+    StackType_t *stack_for_task = (StackType_t *) heap_caps_calloc(1, size_stack, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT);
+    printf("init_task: current addr_stack = %p, stack_for_task = %p\n", esp_cpu_get_sp(), stack_for_task);
+    static StaticTask_t task_buf;
+    xTaskCreateStaticPinnedToCore(test_msync_on_psram, "test_msync_on_psram", size_stack, &test_semphr, 5, stack_for_task, &task_buf, 0);
+
+    xSemaphoreTake(test_semphr, portMAX_DELAY);
+    vSemaphoreDelete(test_semphr);
+    free(stack_for_task);
+}
+#endif  //#if CONFIG_SPIRAM