feat: Connect CPU to ECALL tables #364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
generally i don't think that this works with an elf test yet for whatever reaosn
ctian1
requested changes
Mar 19, 2024
DEVELOPMENT.md
Outdated
| RUST_LOG=info RUST_BACKTRACE=1 cargo test syscall::precompiles::edwards::ed_add::tests::test_ed_add_simple --features debug --no-default-features --release -- --nocapture | ||
| ``` | ||
|
|
||
| RUST_LOG=info RUST_BACKTRACE=1 cargo test syscall::precompiles::edwards::ed_add::tests::test_ed_add_simple --no-default-features --release -- --nocapture |
core/src/air/builder.rs
Outdated
| } | ||
|
|
||
| /// Receives a ecall operation to be processed. | ||
| fn receive_ecall<EShard, EClk, Ea, Eb, Ec, EMult>( |
There was a problem hiding this comment.
should we standardize ecall / syscall naming? maybe it should be ecall everywhere?
core/src/runtime/syscall.rs
Outdated
|
|
||
| /// A system call is invoked by the the `ecall` instruction with a specific value in register t0. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, EnumIter, CheckSyscallConsistency)] |
There was a problem hiding this comment.
IMO this would be cleaner in the unit test module rather than derive, maybe like this:
#[test]
fn test_syscall_codes_consistent() {
check_syscall_consistency!(SyscallCode)
}There was a problem hiding this comment.
oh were you trying to do that I see the import in tests
|
|
||
| /// A system call is invoked by the the `ecall` instruction with a specific value in register t0. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, EnumIter, CheckSyscallConsistency)] | ||
| #[allow(non_camel_case_types)] | ||
| pub enum SyscallCode { |
There was a problem hiding this comment.
needs comment explaining the slots
Comment on lines
29
to
37
| println!("reading in ht"); | ||
| let mut hx = [0u32; 8]; | ||
| for i in 0..8 { | ||
| let (record, value) = rt.mr(w_ptr + (H_START_IDX + i as u32) * 4); | ||
| let (record, value) = rt.mr(h_ptr + i as u32 * 4); | ||
| h_read_records.push(record); | ||
| hx[i] = value; | ||
| rt.clk += 4; | ||
| } | ||
|
|
||
| println!("reading in w"); |
| @@ -31,6 +31,7 @@ impl<F: PrimeField> MachineAir<F> for ShaCompressChip { | |||
| let mut new_field_events = Vec::new(); | |||
| for i in 0..input.sha_compress_events.len() { | |||
| let mut event = input.sha_compress_events[i].clone(); | |||
| // println!("event: {:?}", event); | |||
| @@ -23,7 +23,7 @@ impl Syscall for SyscallEnterUnconstrained { | |||
| record: std::mem::take(&mut ctx.rt.record), | |||
| op_record: std::mem::take(&mut ctx.rt.memory_accesses), | |||
| }; | |||
| 1 | |||
| None | |||
There was a problem hiding this comment.
I think this needs to be 1, that's how the program knows to enter unconstrained mode. But in the VM it actually needs to be constrained to return 0. Also, the exit syscall needs to return 0.
| @@ -0,0 +1 @@ | |||
| RUST_LOG=debug cargo test --package sp1-core --lib -- syscall::precompiles::weierstrass::weierstrass_add::tests::test_secp256k1_add_simple --exact --nocapture | |||
puma314
changed the title
ctian1
approved these changes
Mar 20, 2024
DEVELOPMENT.md
Outdated
| ``` | ||
| RUST_LOG=info RUST_BACKTRACE=1 cargo test syscall::precompiles::edwards::ed_add::tests::test_ed_add_simple --features debug --no-default-features --release -- --nocapture | ||
| RUS_LOG=info RUST_BACKTRACE=1 cargo test syscall::precompiles::edwards::ed_add::tests::test_ed_add_simple --release --features debug --no-default-features -- --nocapture |
core/src/runtime/mod.rs
Outdated
| } else { | ||
| a = syscall_id; // By default just keep the register value the same as it was before. | ||
| // Default to syscall_id if no value is returned from syscall execution. | ||
| a = syscall_id; | ||
| } | ||
| (precompile_rt.next_pc, syscall_impl.num_extra_cycles()) | ||
| } else { | ||
| panic!("Unsupported syscall: {:?}", syscall); | ||
| (0, 0) |
Comment on lines
+261
to
+263
| let send_to_table = syscall_code[1]; // Does the syscall have a table that should be sent. | ||
| let num_cycles = syscall_code[2]; // How many extra cycles to increment the clk for the syscall. | ||
| let is_halt = syscall_code[3]; // Whether or not the syscall is a halt. |
There was a problem hiding this comment.
might need to check that send_to_table and is_halt are bool
jtguibas
pushed a commit
that referenced
this pull request
Mar 29, 2024
Co-authored-by: Uma Roy <[email protected]> Co-authored-by: Chris Tian <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.