user_iram_memory_is_enabled leads to pvportmalloc returning invalid memory addresses

[gilpinheiro]

When user_iram_memory_is_enabled() { return true; }"

Calls to os_malloc (aka pvPortMalloc(size, file, lineno, true)) return buffers that seem to exceed the boundaries of the memory types.

First my assumptions:

• IRAM ends at 0x40108000 (0x40100000-0x40107fff) , and the space after that (starting at 0x40108000) should not be written to.
• DRAM valid addresses are >= 0x3ffe8000 and <= 0x3fffbff

I've run tests and as the memory range is exhausted, pvportmalloc returns buffers that cross those boundaries...

[gilpinheiro]

Test Code:

/* Range check pointers returned from allocation */
int8_t print_address_info(void *p, uint32_t size) {
    if (p == NULL) {
        os_printf("ptr is NULL\n");
        print_heap_remaining();
        return -1;
    } else {
        uint32_t address = (uint32_t)p;
        if ((address >= 0x3ffe8000) && (address <= 0x3fffbfff)) {
            os_printf("0x%x (DRAM) ", address);
            if ((size != 0) && ((address + size -1) >= 0x3fffbfff)) {
                os_printf(" !! RANGE CHECK FAILED (size: %d, address+size: 0x%x)\n", size, address+size);
                return -1;
            } else {
                os_printf("\n");
                return 1;
            }
        } else if ((address >= 0x40100000) && (address <= 0x40107fff)) {
            os_printf("0x%x (IRAM) ", address);
            if ((size != 0) && ((address + size -1) >= 0x40107fff)) {
                os_printf(" !! RANGE CHECK FAILED (size: %d, address+size: 0x%x)\n", size, address+size);
                return -2;
            } else {
                os_printf("\n");
                return 2;
            }
        } else if ((address >= 0x40200000) && (address <= 0x402fffff)) {
            os_printf("0x%x (SPI_FLASH) ", address);
            if ((size != 0) && ((address + size -1) >= 0x402fffff)) {
                os_printf(" !! RANGE CHECK FAILED (size: %d, address+size: 0x%x)\n", size, address+size);
                return -3;
            } else {
                os_printf("\n");
                return 3;
            }
        } else {
            os_printf("0x%x (!!!) !! unexpected memory region\n", address);
            return -4;
        }
    }
}

void run_test() {
    uint8_t *buffer;
    uint32_t size=200;
    ets_wdt_disable();
    while (true) {
        buffer = os_malloc(size); /* or pvPortMalloc(..., true) or any of the other IRAM variants like  pvPortCallocIram */
        print_address_info(buffer, size);
        system_soft_wdt_feed();
    }
}

Annotated Results:

70272 heap free
0x40106fc8 (IRAM) 
0x401070a0 (IRAM) 
0x40107178 (IRAM) 
[ truncated ]
0x40107e20 (IRAM) 
0x40107ef8 (IRAM) 
0x40107fd0 (IRAM)  !! RANGE CHECK FAILED (size: 200, address+size: 0x40108098)
0x3ffefdb8 (DRAM) 
0x3ffefe90 (DRAM) 
0x3ffeff68 (DRAM) 
0x3fff0040 (DRAM) 
0x3fff0118 (DRAM) 
...

When it runs out of dram it actually locks up (the malloc call never returns, as far as I can tell)

I've also encountered range check failures when dram becomes exhausted (the range of the memory buffer continues into memory beyond dram)

These issues don't seem to happen with iram_memory is disabled.

[gilpinheiro]

@xcguang hopefully this issue can be investigated before the next release. Based on the documentation that I just read, the address may be fine (returning address in a region that is was formerly cache), but something is still not right because when it crosses that boundary bad things happen, and don't allocate from the region again (based on the docs, we should expect ~16k more usable space from the former cache). Maybe cache is not actually disabled?

In practice, it manifests as general instability - it appears that some allocations lead to infinite loop within the malloc routines.

On a related note, part of this functionality might also be useful for those suffering from lack of iram (aka https://github.com/espressif/ESP8266_NONOS_SDK/issues/209). From the documentation:

The iRAM is further segmented into two blocks. A 32 KB iRAM block stores code marked with IRAM_ATTR and the other 32 KB block is used to cache code from flash, i.e. code marked with ICACHE_FLASH_ATTR.

v3.0, we have added a new feature to enable using iRAM as memory, which can provide about 17 KB extra memory (but the cache size will decrease to be only 16 KB).

Which leads me to think that it might be possible to provide an option to disable the additional cache, and modify the LD file to use the additional space for code (instead of heap).

[xcguang]

@gilpinheiro Thanks,We will reproduce it, if the issue exists, we will fix it before next release.

[xcguang]

Hi @gilpinheiro , If user_iram_memory_is_enabled() { return true; }, the iram ended 0x4010c000 can be malloced, so the log you provided looks well. If you do not want to use IRAM to malloc, you can remove user_iram_memory_is_enabled() { return true; } or change into user_iram_memory_is_enabled() { return false; }

[xcguang]

Hi @gilpinheiro , Does it work well? I will close this issue, and you can reopen it if the problem persists.

[gilpinheiro]

@xcguang Well, no, but I can tell you why, and how I fixed it. I spent a good deal of reverse engineering what is going on.

I have to use rboot, and the user_iram_memory_is_enabled function isn't being used - rboot replaces Cache_Read_Enable_New from libmain.a to do its own memory mapping.

The consequence is that cache_read_enable is always called with the third parameter = 0, which leads to cache staying on (and mallocs failing to work properly).

I'd like to confirm a few followup questions:

• calling Cache_Read_Enable(x,y, 0) in Cache_Read_Enable_New, and having user_iram_memory_is_enabled return true, are enough to enable the user_iram_is_enabled functionality. e.g. there isn't something else that needs to be initialized.

• Cache_Read_Enable(x,y, 0) without using user_iram_is_enabled should allow us to grow the iram segment to 0xc000 (48k) in the LD file from 32k, and provide additional space for code in iram. The key thing to confirm is that the SDK doesn't use that space for anything else by default when cache is disabled.

Overall it'd be nice to confirm rburton and pfalcon's reverse engineering work on the nature of the parameters to Cache_Read_Enable: https://richard.burtons.org/2015/06/12/esp8266-cache_read_enable/

[gilpinheiro]

@xcguang I can't re-open this issue myself, without copy-pasting the content into an new issue. Hopefully this doesn't get forgotten.