Refactoring RISC-V emulation APIs for easier adoption and porting

[ChinYikMing]

First trial of refactoring, the wasm branch's latest commit is the result.

Since state_t is a user-provided data, so all runtime defined value(often change) shall be stored there. For instance, the emulated target program's argc and argv, and the emulator's parameter. The following have been adjusted to reflect the changes:

1. state_t *state_new(void) -----> state_t *state_new(uint32_t mem_size, int argc, char **argv, bool allow_misalign, bool quiet_output)
mem_size is used for memory_new because different runtimes may have memory requirements (for example, the page size in WebAssembly is 64KiB), the default MEM_SIZE(2^32 - 1) is not appropriate for that. The rest of parameters are runtime defined value.

2. riscv_t *rv_create(const riscv_io_t *io, riscv_user_t userdata, int argc, char **args, bool output_exit_code) -----> riscv_t *rv_create(riscv_user_t userdata)
   Much cleaner function signature.

3. void rv_reset(riscv_t *rv, riscv_word_t pc, int argc, char **args) -----> void rv_reset(riscv_t *rv, riscv_word_t pc)
   We can use rv->userdata to get the required argc and argv.

4. Since memory I/O handlers are rarely changed, it makes less sense to define them during runtime (makes porting difficult). Instead, I believe it is preferable to link them during build time. If really want to change the implementations, make a new C file and link it during build time might be a better choice.
   To do this, some changes are made:

   • define memory I/O handlers in rv_create and link during build time
   • to make memory write interfaces match to compatible with the function pointers, MEM_WRITE_IMPL macro has to be changed:
   • src/io.c

// old
#define MEM_WRITE_IMPL(size, type)                                 \
    void memory_write_##size(uint32_t addr, const uint8_t *src)    \
    {                                                              \
        *(type *) (data_memory_base + addr) = *(const type *) src; \
    }

// new
#define MEM_WRITE_IMPL(size, type)                                 \
    void memory_write_##size(uint32_t addr, const type src)    \
    {                                                              \
        *(type *) (data_memory_base + addr) = src; \
    }

• the calling of memory_write_w in "src/syscall.c" shall be changed accordingly:
• src/syscall.c

// old
memory_write_w(tv + 0, (const uint8_t *) &tv_s.tv_sec);

// new
memory_write_w(tv + 0, *((const uint32_t *) &tv_s.tv_sec));

For notably change, the "pre.js" of the wasm branch do not define IO on its own anymore compare to first attempt.(more abstraction)

5. Change all uint32_t and uint16_t and uint8_t in riscv.[ch] to riscv_word_t and riscv_half_t and riscv_byte_t in function signature respectively for consistency.

6. bool elf_load(elf_t *e, riscv_t *rv, memory_t *mem); -----> bool elf_load(elf_t *e, riscv_t *rv);
   The memory instance required by elf_load can be accessed via rv's userdata.

I am wondering shall we abstract the FILE defined in state_new as a parameter of state_new. Without abstraction, the emulator always depends on standard io(e.g., stdin, stdout, stderr). What if the user want to use a file instead of stdout?

Related to: #75

[jserv]

I would like to invite @RinHizakura, @qwe661234, and @visitorckw to join the discussion and contribute to the refinement of the API.

[jserv]

I am wondering shall we abstract the FILE defined in state_new as a parameter of state_new. Without abstraction, the emulator always depends on standard io(e.g., stdin, stdout, stderr). What if the user want to use a file instead of stdout?

In the initial stages of developing this emulator, I redirected I/O operations to files for comparison purposes. However, I now recognize that this approach to the function interface was not as flexible as I had initially thought.

[ChinYikMing]

I am wondering shall we abstract the FILE defined in state_new as a parameter of state_new. Without abstraction, the emulator always depends on standard io(e.g., stdin, stdout, stderr). What if the user want to use a file instead of stdout?

In the initial stages of developing this emulator, I redirected I/O operations to files for comparison purposes. However, I now recognize that this approach to the function interface was not as flexible as I had initially thought.

So, it could be useful to provide an abstract way for defining the desired stdin, stdout, stderr, or more than just three of them. stdin, stdout, stderr can be set as default if any spefication of them is not given.

[RinHizakura]

Since memory I/O handlers are rarely changed, it makes less sense to define them during runtime (makes porting difficult). Instead, I believe it is preferable to link them during build time.

I think the distinction between modules is a little bit unclear in the current design of rv32emu. On the current design, if we regard riscv.c as the part of the library and main.c as the part of the application using the library. Although rv_create() seems to allow the application to customize memory operations through io in a pointer manner, the operation on simulated memory actually must be bound to the instance created by state_new(), which is belongs to the library side. This leads to limitations for customizing io. For example, what if you want to use a backup file to simulate memory? This design seems to make memory operations using function pointers io redundant.

I believe it is preferable to link them during build time.

So, if it doesn't matter to provide user-specific operations on memory, providing them on the build time for the library will also be a great solution.

[RinHizakura]

mem_size is used for memory_new because different runtimes may have memory requirements (for example, the page size in WebAssembly is 64KiB), the default MEM_SIZE(2^32 - 1) is not appropriate for that.

Not quite sure about whether changing the memory size directly is safe. As I remember, some implementations of rv32emu intensionally rely on the fact that the memory size is 2^32 - 1 to have some trick. Or maybe I mix up with some project else. Looking for others to answer the question.

[ChinYikMing]

mem_size is used for memory_new because different runtimes may have memory requirements (for example, the page size in WebAssembly is 64KiB), the default MEM_SIZE(2^32 - 1) is not appropriate for that.

Not quite sure about whether changing the memory size directly is safe. As I remember, some implementations of rv32emu intensionally rely on the fact that the memory size is 2^32 - 1 to have some trick. Or maybe I mix up with some project else. Looking for others to answer the question.

The built-in ELF programs do not seem to need a lot of memory so I think 2GB - 4GB is a safe region. Dynamically changing the memory size in different runtime might be needed. For example, 64KiB multiples should be used in WebAssembly. The MEM_SIZE is set to 2^32 originally in #151 as the memory size for preallocating memory to prevent extra checking when manipulating the memory region. Then, MEM_SIZE is set to 2^32 - 1 in #221 to compatible with emcc which default build target is wasm32 ( memory shall < 4GB ).

[ChinYikMing]

I would like to introduce cycles_per_step into state_t structure since it can be varied. For example, in web-based simulation, the user might want to increase the cycles_per_step to jump quicker to the desired part of execution to see the register file or memory bank status. For better abstraction, it could be possible to add a pair of getters and setters.

Then, rv_step signature can be refactored to have only one parameter: void rv_step(riscv_t *rv). The cycles_per_step can be retrieved via rv->userdata

[ChinYikMing]

rv_enables_to_output_exit_code could be renamed as something like rv_get_xxx. Same rules might be applied to other fields of state_t to improve consistency. rv_set_xxx can be the setter.

For example:

• rv_get_userdata / rv_set_userdata
• rv_get_pc / rv_set_pc
• rv_get_reg / rv_set_reg
• rv_get_halt_status / rv_set_halt_status
• rv_get_cycle_per_step / rv_set_cycle_per_step
• rv_get_output_exit_code_flag / rv_set_output_exit_code_flag
• rv_get_allow_misalign_flag / rv_set_allow_misalign_flag
• ...

[jserv]

I would like to introduce cycles_per_step into state_t structure since it can be varied. For example, in web-based simulation, the user might want to increase the cycles_per_step to jump quicker to the desired part of execution to see the register file or memory bank status. For better abstraction, it could be possible to add a pair of getters and setters.

Then, rv_step signature can be refactored to have only one parameter: void rv_step(riscv_t *rv). The cycles_per_step can be retrieved via rv->userdata

I agree. By the way, state_t might not be a very self-explanatory name. I am considering unifying it into a VM-specific structure.

[jserv]

The repository mnurzia/rv serves as an additional reference for API refinement. It features three main APIs:

• Memory Access Callback: This function processes data as input/output and returns RV_BAD in case of a fault. It's defined as:
  typedef rv_res (*rv_bus_cb)(void *user, rv_u32 addr, rv_u8 *data, rv_u32 is_store, rv_u32 width);
• CPU Initialization: This function initializes the CPU and can be called again on the cpu object to reset it. The function signature is:
  void rv_init(rv *cpu, void *user, rv_bus_cb bus_cb);
• CPU Single-Step: This function advances the CPU by one step and returns RV_E * in case of an exception. Its definition is:
  rv_u32 rv_step(rv *cpu);

These APIs collectively provide a structure for memory access, CPU initialization, and step-wise execution in the CPU simulation.

[ChinYikMing]

As previously suggested, the maximum memory (MEM_SIZE) of a virtual machine (VM) shall be determined by the application. If these modifications are made, the Makefile-defined default stack size shall also be adjusted.

Makefile:

# Set the default stack pointer
...
CFLAGS += -D DEFAULT_STACK_ADDR=0xFFFFE000
# Set the default args starting address
CFLAGS += -D DEFAULT_ARGS_ADDR=0xFFFFF000
...

Thus, adjusting stack size should be a part in public API.

[ChinYikMing]

I would like to introduce cycles_per_step into state_t structure since it can be varied. For example, in web-based simulation, the user might want to increase the cycles_per_step to jump quicker to the desired part of execution to see the register file or memory bank status. For better abstraction, it could be possible to add a pair of getters and setters.
Then, rv_step signature can be refactored to have only one parameter: void rv_step(riscv_t *rv). The cycles_per_step can be retrieved via rv->userdata

I agree. By the way, state_t might not be a very self-explanatory name. I am considering unifying it into a VM-specific structure.

I think vm_attr_t can be the candidate for the name ( inspired by pthread_attr_t ).

Currently, vm_attr_t should consist of the following:

1. vm RAM size (if previous concern is OK)
2. vm STACK size (if vm RAM size changes)
3. vm-specific argc, argv
4. error code (to represent the exit state of vm)
5. enable_outout_exit_code
6. logging level
7. union of target ELF program and target vm

union {
    rv_struct_t rv_struct;
    vm_struct_t vm_struct;
};

typedef struct rv_struct {
     char *elf_program;
} rv_struct_t;

typedef struct vm_struct {
    kernel_img;
    dtb;
    rootfs_img;
} vm_struct_t ;

8. cycle_per_step
9. enable_misaligned

I would like to introduce the sixth attribute of vm_attr_t which allows the user to select how vm should log, just like printk log level of Linux kernel. This logging level will register corresponding handler during rv_init initialization. This feature enable the user has more flexibility to observe the vm state or error reporting. The sixth attribute of vm_attr_t allows to differentiate RISC-V program or RISC-V system emulation, then rv_create return a corresponding internal structure (riscv_internal or vm_internal), of course they are forward declaration structure.

Prefix of all vm-related functions should be consistent ( more discussion ).

[jserv]

state_t might not be a very self-explanatory name. I am considering unifying it into a VM-specific structure.

I think vm_attr_t can be the candidate for the name ( inspired by pthread_attr_t ).

It sounds promising. Please send pull request(s) to refine APIs.

Currently, vm_attr_t should consist of the following:

1. vm RAM size (if previous concern is OK)
2. vm STACK size (if vm RAM size changes)
3. vm-specific argc, argv

How about envp?

4. error code (to represent the exit state of vm)
5. enable_outout_exit_code

enable_outout_exit_code looks hacky. Can you show something detailed?

[ChinYikMing]

1. vm RAM size (if previous concern is OK)
2. vm STACK size (if vm RAM size changes)
3. vm-specific argc, argv

How about envp?

Since the envp is not accessible for now, place a TODO in vm_attr_t might be decent.

5. enable_outout_exit_code

enable_outout_exit_code looks hacky. Can you show something detailed?

It is related to syscall_exit to determine whether to output the exit code. I think always output the exit code is not a bad thing, maybe this is redundant. Or, it can be determined on top of logging feature.

[jserv]

I think always output the exit code is not a bad thing, maybe this is redundant. Or, it can be determined on top of logging feature.

After streamlining the API, we can control the exit code by storing it in a specific structure, instead of displaying it directly in the console.

[ChinYikMing]

I am wondering shall we abstract the FILE defined in state_new as a parameter of state_new. Without abstraction, the emulator always depends on standard io(e.g., stdin, stdout, stderr). What if the user want to use a file instead of stdout?

In the initial stages of developing this emulator, I redirected I/O operations to files for comparison purposes. However, I now recognize that this approach to the function interface was not as flexible as I had initially thought.

So, it could be useful to provide an abstract way for defining the desired stdin, stdout, stderr, or more than just three of them. Standard stdin, stdout, stderr can be set as default if any spefication of them is not given.

For abstracting file or file descriptor, we could have an attribute called bool use_default_stdin_stdout_stderr in vm_attr_t which will use common stdin, stdout and stderr. For alternative, we could have a function called vm_register_stdxxx makes the vm to register the non-common fd (e.g., regular file) before emulation.

[RinHizakura]

For abstracting file or file descriptor, we could have an attribute called bool use_default_stdin_stdout_stderr in vm_attr_t which will use common stdin, stdout and stderr. For alternative, we could have a function called vm_register_stdxxx makes the vm to register the non-common fd (e.g., regular file) before emulation.

Since we should also have to maintain the file descriptor if vm_register_stdxxx() for redirection, how about just adding three file descriptors in vm_attr_t:

struct {
    ...
    int stdin;
    int stdout;
    int stderr;
} vm_attr_t;

Note: The variable's name should be refined. I just can't come up with a suitable and concise name now

These file descriptors are assigned to 0, 1, and 2 by default, and modified if vm_register_stdxxx() it. So we don't need a redundant variable bool use_default_stdin_stdout_stderr for this feature.

[ChinYikMing]

For abstracting file or file descriptor, we could have an attribute called bool use_default_stdin_stdout_stderr in vm_attr_t which will use common stdin, stdout and stderr. For alternative, we could have a function called vm_register_stdxxx makes the vm to register the non-common fd (e.g., regular file) before emulation.

Since we should also have to maintain the file descriptor if vm_register_stdxxx() for redirection, how about just adding three file descriptors in vm_attr_t:

struct {
    ...
    int stdin;
    int stdout;
    int stderr;
} vm_attr_t;

Note: The variable's name should be refined. I just can't come up with a suitable and concise name now

These file descriptors are assigned to 0, 1, and 2 by default, and modified if vm_register_stdxxx() it. So we don't need a redundant variable bool use_default_stdin_stdout_stderr for this feature.

Thanks for tips! I think the boolean really redundant since vm_register_stdxxx() could overwrite them.