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

Feat: keccak256 #5

Merged
merged 7 commits into from
Nov 10, 2024
Merged

Feat: keccak256 #5

merged 7 commits into from
Nov 10, 2024

Conversation

0xurb
Copy link
Contributor

@0xurb 0xurb commented Nov 8, 2024

Quick resolve for KECCAK256 implementation related to #4

cargo run is passed correctly, but not too sure about this implementation

cc: @leonardoalt

lateout("a2") third,
lateout("a3") fourth,
in("t0") u32::from(Syscall::Keccak256),
options(nostack, preserves_flags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • lateout - 256 bytes of keccak hash goes to 4 registers, it's like out("a0"), out("a1") ... etc. but marked "late" to tell that it only writes to the register after all inputs are read.

  • nostack - tells the compiler this asm block doesn't use the stack.

  • preserves_flags - means that asm! block does not modify the flags register

Saw nostack in other RISC-V adapters code, e.g this, but it is not necessary I think. Don't have any experience with asm!.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea I feel like the options are not needed here. I think we should remove it if it works without

Copy link
Collaborator

@leonardoalt leonardoalt left a comment

Choose a reason for hiding this comment

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

Sick! Please undraft

@leonardoalt leonardoalt mentioned this pull request Nov 9, 2024
Comment on lines +265 to +272
let data_bytes = if size != 0 {
emu.cpu
.bus
.get_dram_slice(offset..(offset + size))
.unwrap()
} else {
&mut []
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be extracted into a function

syscalls!(
(0, Return, "return"),
(1, SLoad, "sload"),
(2, SStore, "sstore"),
(3, Call, "call"),
(4, Revert, "revert"),
(5, Caller, "caller"),
(6, Keccak256, "keccak256"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's start using EVM opcodes as syscall ids here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leonardoalt let's do on both PRs (for log also) with opcodes and then in a new PR/commit the rest

r55/src/exec.rs Outdated
@@ -256,6 +257,30 @@ fn execute_riscv(
let third_u64 = u64::from_be_bytes(padded_bytes);
emu.cpu.xregs.write(12, third_u64);
}
6 => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would also be good to replace the hardcoded ids here by the actual syscall names (in the match arms)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes thanked about that too
Done it on Odyssey's draft

@0xurb 0xurb marked this pull request as ready for review November 9, 2024 13:37
@0xurb
Copy link
Contributor Author

0xurb commented Nov 9, 2024

@leonardoalt let's merge both syscalls with ids as regarded opcodes and then make a PR with this improvements:

  • replace first five syscalls ids with a regarded opcodes
  • replace the hardcoded ids(opcodes) on

    r55/r55/src/exec.rs

    Lines 154 to 155 in 587a485

    let t0: u64 = emu.cpu.xregs.read(5);
    match t0 {

    by the actual syscall names (from Syscall enum) with unhandled syscall opcode exception before matching (use Syscall ::try_from(opcode).unwrap_or_else(||...) instead of _ => {} match arm)
  • extract

    r55/r55/src/exec.rs

    Lines 160 to 167 in 587a485

    let data_bytes = if ret_size != 0 {
    emu.cpu
    .bus
    .get_dram_slice(ret_offset..(ret_offset + ret_size))
    .unwrap()
    } else {
    &mut []
    };
    into a function

@leonardoalt
Copy link
Collaborator

@0xurb ok sounds good, if you could just remove the asm options and see if it works without, we could remove & merge

@0xurb
Copy link
Contributor Author

0xurb commented Nov 9, 2024

@leonardoalt done, cargo r works for me

@leonardoalt leonardoalt added this pull request to the merge queue Nov 10, 2024
Merged via the queue into r55-eth:main with commit b3492d5 Nov 10, 2024
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.

2 participants