-
Notifications
You must be signed in to change notification settings - Fork 78
Conversation
This implements the `r0::init_data` and `r0::zero_bss` routines in assembly. There is a generic implementation for `riscv32` and `riscv64`, since `riscv64` deals with alignment problems. The routines are kept at their old calling site so that only one hardware thread calls them. Consequently they are also inlined into the `start_rust` function. [Issue rust-embedded#122]
Thanks! I'll review it ASAP |
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 left you a few comments. I had some doubts with the loops, maybe I'm missing something.
Regarding RISC-V 64 specific routines, we can use feature gates to select one or another method. However, the current implementation should work for both RISC-V 32 and 64 (I guess).
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.
LGTM
Is there anything you want me to do about the RV64 specific routine? I can add that. I just need to modify the |
I've been reviewing the current |
BTW! Please, add your changes to |
So, I now added the RV64 version. Could you please check to make sure I didn't make any mistakes? |
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.
LGTM.
The only "improvement" I can propose now depends on whether the new assertion is only required in RISC-V 64 targets. If so, we can add link-32.x
and link-64.x
linker files with arch-specific assertions. Then, build.rs
would add to the compilation the correct additional linker file with a generic name such as link-arch.x
, which will be included in the current link.x
file. The hifive1
crate does something similar for a set of boards.
Another improvement would be modifying start_rust
to look more like in esp-riscv-rt
. I'd love to update riscv-rt
using some of the ideas of esp-riscv-rt
. Maybe @MabezDev can give us more insights about this :)
In any case, these improvements can be tackled in future PRs
link.x
Outdated
@@ -150,6 +150,10 @@ BUG(riscv-rt): .data is not 4-byte aligned"); | |||
ASSERT(_sidata % 4 == 0, " | |||
BUG(riscv-rt): the LMA of .data is not 4-byte aligned"); | |||
|
|||
/* Make sure that we can safely perform .data initialization on RV64 */ | |||
ASSERT(_sidata % 8 == _sdata % 8, " |
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.
Is this assertion necessary for RISC-V 32 targets?
CI fails... this RISC-V 64 optimization seems difficult to add without modifying the alignment rules. I propose reverting the RISC-V 64 optimization and leaving the 32-bit-compliant version for now. Then, we can develop a slightly smarter build script that applies different alignment rules depending on the target pointer width and add this optimization again. |
Maybe, this is better. Now, I added separate linker scripts for One added benefit of doing this is that the riscv64 assembly code can be massively simplified. |
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.
LGTM. Let's see if the CI passes now
This implements the
r0::init_data
andr0::zero_bss
routines in assembly. There is a generic implementation forriscv32
andriscv64
, since ariscv64
would deal with alignment problems. The routines are kept at their old calling site so that only one hardware thread calls them. Consequently they are also inlined into thestart_rust
function.One special consideration I ran into is the alignment constraints for the
.bss
and.data
sections. In the documentation forr0
, it notes that the.bss
section should also be aligned to 4 byte. This makes a lot of sense since it was usingu32
s as enforcing alignment of the sections. The currentlink.x
file does not have this constraint. It is added for the.data
section and therefore I am assuming it to be a bug. I added it in since it is also required for this patch to work correctly.Ideally, for the
riscv64
implement we could use the 64-bit instructions load and store instructions. This is currently not possible. We could overcome the 4 byte alignment without changing thelink.x
script by using the follow code.I did try this but it might still lead to problematic behavior since now there is a chance that the
_sidata
is differently aligned than_sdata
. In which case, we use load and store instructions on unaligned addresses. This might cause exceptions according to the RISC-V Manual. Therefore, we also need to ensure the alignment of the_sidata
section. We could assert this, but again, this might be undesirable.As proposed as well in #122 we could add a feature to zero out the entire RAM, but I believe that to be a separate issue onto itself.
Fixes #122