-
Notifications
You must be signed in to change notification settings - Fork 81
Basic virtual memory implementation #163
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is pretty good work! Thank you
Most of my comments are style nits, so don't worry to much :) Btw, please make sure the the code is properly formated by clangformat that with config that is in the root of the project.
There are some cases I think we should move the components around to make this better encapslated.
There is one major decision that we need to make regarding the use of virtual memory. I think the current compomise is not great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please delete the accidentally committed files.
/cmake-build-debug-wsl/ | ||
/cmake-build-release-wsl/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I do this on my machine is that I configure the files to be build/Debug
and build/Release
. You can also ignore thise files localy. It should not be here.
@@ -79,6 +80,7 @@ class SimpleAsm : public QObject { | |||
private: | |||
QStringList include_stack; | |||
machine::FrontendMemory *mem {}; | |||
machine::MachineConfig config {}; | |||
machine::RelocExpressionList reloc; | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a pointer - it feels weirt to have multiple copies of machine config in the program.
@@ -286,7 +288,12 @@ bool SimpleAsm::process_line( | |||
if (error_ptr != nullptr) { *error_ptr = error; } | |||
return false; | |||
} | |||
address = machine::Address(value); | |||
qint64 offset = qint64(value); | |||
qint64 base = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please prefer standard cpp types over qt ones whenever possible.
if (config.get_vm_enabled() && config.get_vm_mode() == machine::MachineConfig::VM_SV32) { | ||
base = config.get_kernel_virt_base(); | ||
} | ||
address = machine::Address(base + offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now looking at this, wouldn't it be simpler to just pass the base to the assembler constructor and keep this logic outside?
aclint::AclintSswi *aclint_sswi = nullptr; | ||
std::optional<TLB> tlb_program; | ||
std::optional<TLB> tlb_data; | ||
FrontendMemory *instr_if_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require a deeper design decision @ppisa
I see 2 ways to go here:
- We go all in on virtual addresses. We say that the CPU always operates with virtual memory. For CPU without MMU we make the mapping 1:1. This means that the memory interface will always be an instance of TLB (possibly trivial).
- We give up on virtuall addresses, we just make TLB just another instance of FrontendMemory. We use the same type for both virtual and physical address.
This code is somewhere in the middle which IMO does not bring any benefit of special type but brings all the cons. This one is on me, I should have explained it better.
return false; | ||
} | ||
|
||
void PageFaultHandler::perform_page_allocation(const VirtualAddress &va, uint32_t raw_satp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppisa I am wondering if this should rather be in osemu.
if (machine == nullptr) { return nullptr; } | ||
if (machine->memory_data_bus() != nullptr) { return machine->memory_data_bus(); } | ||
return machine->instr_frontend(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, thos never used to point to cache.
this->machine = machine; | ||
if (machine && machine->config().get_vm_enabled() | ||
&& machine->config().get_vm_mode() == machine::MachineConfig::VM_SV32) { | ||
index0_offset = machine::Address(machine->config().get_va_base_addr()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this place is a really good ilustration why separate virtual memory type will increase clarity a lot.
if (mem == nullptr) { return QString(""); } | ||
if ((access_through_cache > 0) && (machine->cache_data() != nullptr)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need this for proper startup/reset,
Also, please consider that the next person reading this code will not be an expert on MMU like you, so try to add comments and use less cryptic names whenever possible (don't be afraid of long names too much). |
I did send direct e-mail to @nemecad and @jdupak at 2025-08-08. It has been mainly about generated content in the commits and some remarks for clarification etc. I hope that you have received it. I plan ti provide review there when the technical/format problems are resolved and updated series is posted. |
I did get it, but the generated files did not cause too much trouble for me to review this. |
Nice work, I have problem to cmake this project. I had to change CMakeList.txt in machine directory (remove memory and some others tests). Then in cli main.cpp, there is TODO at line 500, than was necessary to comment. |
This PR implements basic virtual memory support:
UI Components: