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

extend Memory backend to dealing with various needs #147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KotorinMinami
Copy link
Contributor

We know that qtrvsim uses a memorytree with a fixed depth of 6, which causes its memory model to only accept 32-bit memory access instructions, thus restricting 64-bit access. This limitation presents an issue in terms of educational rigor. Therefore, this PR makes corresponding adjustments so that the memory class adapts its memory access bit width according to xlen, while also addressing the #146

@ppisa
Copy link
Member

ppisa commented Aug 4, 2024

Hello @KotorinMinami, thanks for contribution. But I see more drawbacks there. The first and most important principle for QtRvSim is to make it illustrative for students us much as possible and the next to keep it in state, that it can be reasonably maintained and each added complexity should be backed by real needs.

  • there is inconsistency that you extend address width in data (MemoryDock) but not in ProgramDock
  • the 16 digits number is hard to sense for people so it would lead to more difficult orientation
  • I am not sure of moving overlay principle in memory views which is implemented to overcome Qt table index limitation is fully prepared for 64-bit range, this would require testing and analysis
  • I expect that you have not tested with 64-bit ELF build of temple.S by full featured assembler, this set correctly the address with upper 32-bits zeroed by li. The addi a1, zero, text_1 has to be replaced by la a1, text_1 for GASM build and should go to mainline too.
  • the deeper memory trees make simulator slower even more, which starts to be problem, but your configurable depth is probably solution
  • I would prefer Xlen type argument (enum) as argument to Memory(machine_config.get_simulated_endian(), 64) which can be directly assigned to this->machine_config.get_simulated_xlen() or the address width argument where address bit size is obtained from Xlen by some Xlen.toBits() method. Duplication, ternary and if, else makes code more complex, harder to read.
  • the same problem with memory_bus_insert_range. No if, else be used, there should be defined some peripherals base address same as the last memory address. Alternative is to insert peripherals the second time as alias in 64-bit case which would solve serial port availability for external assembler build

So in general, there are more problems where I do not see clean profit. As for the pull request, I would prefer to divide changes to hexedit and docks change as separate commit, memory three height commit and then machine setup adjustments as separate ones.

I do not see this change as priority, which is branch predictor for me and I wait for @jiristefan and @jdupak to resolve issues with me to have at least solid tool for branch prediction demonstration on single-cycle core. The pipelined cannot misbehave in the actual program results (which is actually in correct state), but tuning for more appropriate statistic in pipelined version is complex task and without predecode in cache can be hardly solved ideal way.

Back to 64-bit mode. I do not see it as important for teaching, for sure, the core should behave correctly, which I hope it does, but 64-bit addresses are harder to follow, we do not have enough space in signal views, there would be great some ellipses for numbers with many zeros or F's as 0xf....f123 etc. Even in 64-bit mode, I see 4GB limit in physical address space as enough for education and experiments. Even riscv64-linux-gnu-gcc -march=rv64imd -mabi=lp64d --static build maps text at 0x00010554 for me. So even 2GB range should be OK. There is more problems with missing compressed instruction set. But again make it clean in program view etc. is quite lot of work and consideration. So I do not count even that a priority. We can use riscv64-unknown-elf, newlib and or syscalls stub to provide simple binaries running under GNU/Linux and QtRvSim. The range is limited by minimal set of syscalls supported by QtRvSim, missing memmap where I have idea with rewrite syscalls and serial port to QFile. It is even partially implemented in work unfinished/abandoned by @hollmmax .

Can you send link to your teaching materials or describe idea where full 64-bit address space is real advantage?

In the fact, I would be more interested in sv39 and even more in some simpler sv32 MMU and full range wirtual address space and keep physical addressing at 32 bits for now. But my time and budget resources are very limited and realiable function of the base is more important for me than features.

@@ -33,12 +37,16 @@ Machine::Machine(MachineConfig config, bool load_symtab, bool load_executable)
}
mem = new Memory(*mem_program_only);
} else {
mem = new Memory(machine_config.get_simulated_endian());
mem = (this->machine_config.get_simulated_xlen() == Xlen::_64 ? new Memory(machine_config.get_simulated_endian(), 64) : new Memory(machine_config.get_simulated_endian(), 32));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Making a function xlen to bit size would be nicer.

@@ -48,7 +48,7 @@ HexLineEdit::HexLineEdit(
set_value(0);
}

void HexLineEdit::set_value(uint32_t value) {
void HexLineEdit::set_value(uint64_t value) {
QString s, t = "";
last_set = value;
s = QString::number(value, base);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with storing 64bit values here, but we should make more a 32 bit friendly visualization.

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.

3 participants