Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bring up wamr on esp32-s3 device #2348

Conversation

dongsheng28849455
Copy link
Contributor

esp32-s3's instruction memory and data memory can be accessed through mutual mirroring way, so we define a feature named as WASM_MEM_DUAL_BUS_MIRROR

first stage
1, define the iram checking function
2, define the ibus-dbus converting offset
3, modify address allocated by malloc
4, free the modified address allocated by malloc
into in_ibus_ext
add os_get_dbus_mirror(void *ibus) as system interface
only when ibus in its range, dbus has the offset
add compiler switch for os_mmap
format using clang-format
@dongsheng28849455 dongsheng28849455 force-pushed the dev/bringup-wamr-on-esp32-s3-device branch from 5c5113c to 6aba288 Compare July 6, 2023 08:50
#if (WASM_MEM_DUAL_BUS_MIRROR != 0)
if ((prot & MMAP_PROT_EXEC) != 0) {
d_addr = malloc((uint32)size);
i_addr = d_addr + MEM_DUAL_BUS_OFFSET;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc, "NULL + x" is undefined in C

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had better not (void *) + offset, it may cause compiler error or warning.
Had better i_addr = (void *)((uint8 *)d_addr + MEM_DUAL_BUS_OFFSET);

use clang-format12 to format
check NULL when malloc
@@ -219,6 +222,9 @@ apply_relocation(AOTModule *module, uint8 *target_section_addr,
initial_addend = *(int32 *)insn_addr;
*(uintptr_t *)insn_addr = (uintptr_t)symbol_addr + initial_addend
+ (intptr_t)reloc_addend;
#if (WASM_MEM_DUAL_BUS_MIRROR != 0)
os_dcache_flush();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to flush cache in each relocation, when this done, load_from_sections will do this when loading sections is done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding some comments in this PR? developer may be confused about the code.

@@ -286,6 +296,9 @@ apply_relocation(AOTModule *module, uint8 *target_section_addr,
#pragma GCC diagnostic pop
#endif

#if (WASM_MEM_DUAL_BUS_MIRROR != 0)
os_dcache_flush();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as upper comment.

return NULL;
}
i_addr = d_addr + MEM_DUAL_BUS_OFFSET;
return in_ibus_ext(i_addr) ? i_addr : d_addr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By a way, not only external RAM(PSRAM) use mirror bus address, but also internal RAM(SRAM) is the same. So maybe you use this solution for internal RAM, too.

Copy link
Contributor Author

@dongsheng28849455 dongsheng28849455 Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean we should indicate from where to allocate before we do it? and do they have different allocator?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to make nuttx platform easier to use, we could make it support to load AOT to sram and run.

and add some comments for the PR motivation
esp32-s3 requires special steps for cache flush
align the void* into int8*
unit8 * not int8*
@dongsheng28849455
Copy link
Contributor Author

@donghengqaz @wenyongh @yamt any other comments?

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, several minor comments

core/iwasm/aot/aot_loader.c Show resolved Hide resolved
core/iwasm/aot/arch/aot_reloc_xtensa.c Show resolved Hide resolved
core/iwasm/aot/arch/aot_reloc_xtensa.c Show resolved Hide resolved
@@ -10,6 +10,46 @@
#include <nuttx/arch.h>
#endif

#if defined(CONFIG_ARCH_CHIP_ESP32S3) && (CONFIG_ARCH_CHIP_ESP32S3 != 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"#ifdef CONFIG_ARCH_CHIP_ESP32S3" is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the case of "#define CONFIG_ARCH_CHIP_ESP32S3 0"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition is generated from Kconfig automatically, if ARCH_CHIP_ESP32S3 not selected in Kconfig, it should delete this line instead of define it as 0, I guess.

@donghengqaz
Copy link
Contributor

When this PR is merged, I plan to add WAMR to espressif official components management, this can help more people to use WAMR more easily.

Only check if the compilation switch exists on nuttx to
determine the function on or off
@wenyongh wenyongh merged commit fbe072c into bytecodealliance:main Jul 20, 2023
@shreeve
Copy link

shreeve commented Nov 17, 2023

@donghengqaz - Phenomenal work on this! You mentioned you would be adding WAMR to espressif official components management. Do you have an update on that?

@donghengqaz
Copy link
Contributor

@donghengqaz - Phenomenal work on this! You mentioned you would be adding WAMR to espressif official components management. Do you have an update on that?

Hi @shreeve I have been busy with some other development work, so have not started to do this. And after these things are done, I will start this.

@yamt
Copy link
Collaborator

yamt commented May 8, 2024

@donghengqaz can you share nuttx kconfig you used for this?

@yamt
Copy link
Collaborator

yamt commented May 8, 2024

@donghengqaz can you share nuttx kconfig you used for this?

well, i meant @dongsheng28849455 . wrong completion, sorry.

@dongsheng28849455
Copy link
Contributor Author

@donghengqaz can you share nuttx kconfig you used for this?

well, i meant @dongsheng28849455 . wrong completion, sorry.

it is CONFIG_INTERPRETERS_WAMR_MEM_DUAL_BUS_MIRROR

@yamt
Copy link
Collaborator

yamt commented May 10, 2024

@donghengqaz can you share nuttx kconfig you used for this?

well, i meant @dongsheng28849455 . wrong completion, sorry.

it is CONFIG_INTERPRETERS_WAMR_MEM_DUAL_BUS_MIRROR

there is no such a config in https://github.com/apache/nuttx-apps.
i guess you are talking about your private fork?

also, i want to see the whole config.
(because i was struggling to find out a combination of memory configs which can work.)

@dongsheng28849455
Copy link
Contributor Author

i guess you are talking about your private fork?

It's defined in https://github.com/midokura/incubator-nuttx-apps fork

because i was struggling to find out a combination of memory configs which can work

What is whole config you mean? is that something could enable several hidden APIs in esp32-s3?

    extern void cache_writeback_all(void);
    extern uint32_t Cache_Disable_ICache(void);
    extern void Cache_Enable_ICache(uint32_t autoload);

@yamt
Copy link
Collaborator

yamt commented May 11, 2024

i guess you are talking about your private fork?

It's defined in https://github.com/midokura/incubator-nuttx-apps fork

ok. it's a private fork.
do you have any plan to upstream necessary changes?

because i was struggling to find out a combination of memory configs which can work

What is whole config you mean? is that something could enable several hidden APIs in esp32-s3?

    extern void cache_writeback_all(void);
    extern uint32_t Cache_Disable_ICache(void);
    extern void Cache_Enable_ICache(uint32_t autoload);

i meant a complete working ".config" file.

specifically i wanted to know values for:
CONFIG_ARCH_USE_TEXT_HEAP
CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP
CONFIG_ESP32S3_RTC_HEAP
CONFIG_MM_KERNEL_HEAP
CONFIG_ESP32S3_SPIRAM
CONFIG_BUILD_FLAT
CONFIG_BUILD_PROTECTED
CONFIG_ESP32S3_SPIRAM_COMMON_HEAP
etc

but now i suspect this PR is not expected to work with upstream nuttx. is it the case?

@dongsheng28849455
Copy link
Contributor Author

the feature quite depends on esp32-s3's implementation, including the config. that might or not delivered into nuttx upstream. I don't have a plan to abstract an interface for Nuttx to use here.

yamt added a commit to yamt/incubator-nuttx-apps that referenced this pull request May 13, 2024
@yamt
Copy link
Collaborator

yamt commented May 13, 2024

the feature quite depends on esp32-s3's implementation, including the config. that might or not delivered into nuttx upstream. I don't have a plan to abstract an interface for Nuttx to use here.

i needed the following changes to make it work on esp32s3-devkit.
#3421
apache/nuttx#12328
apache/nuttx-apps#2385

@xiaoxiang781216
Copy link

the feature quite depends on esp32-s3's implementation, including the config. that might or not delivered into nuttx upstream. I don't have a plan to abstract an interface for Nuttx to use here.

why adding esp32s3 specific code to the common wamr runtime? @no1wudi please review the arch specific change carefully, we need ensure ALL chips on nuttx get the equal support.

xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request May 14, 2024
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
esp32-s3's instruction memory and data memory can be accessed through mutual mirroring way,
so we define a new feature named as WASM_MEM_DUAL_BUS_MIRROR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants