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

Replace MethodIDs with ImageIDs #326

Merged
merged 13 commits into from Jan 17, 2023
Merged

Replace MethodIDs with ImageIDs #326

merged 13 commits into from Jan 17, 2023

Conversation

flaub
Copy link
Member

@flaub flaub commented Jan 10, 2023

No description provided.

* Drop unused regions
* Expand most regions to 32M, with exception of stack/data at 16M
* Use full 2^28 address range available
This is an initial milestone for supporting upcoming continuations.
@flaub flaub self-assigned this Jan 10, 2023
Copy link
Member

@tzerrell tzerrell left a comment

Choose a reason for hiding this comment

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

🎉

LGTM (of course, I'm not really suited for reviewing the more circuit-integration-y parts of this, but the bits I could review well look good)

Copy link
Contributor

@jbruestle jbruestle left a comment

Choose a reason for hiding this comment

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

Very excited about this change, comments inline.

input : ORIGIN = 0x04000000, LENGTH = 32M
stack : ORIGIN = 0x02000000, LENGTH = 16M
data (RW) : ORIGIN = 0x03000000, LENGTH = 16M
heap : ORIGIN = 0x04000000, LENGTH = 32M
prog (X) : ORIGIN = 0x06000000, LENGTH = 32M
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to put prog @ 0x08... - 0x0c...? I was of the impression that system memory starts only after 0x0c and that nondet was now below @0x02. It seems like 64M for heap and 64M for code would be better than 32M for heap and 96M for program. Or maybe even more room for heap.

Copy link
Member Author

Choose a reason for hiding this comment

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

That all makes great sense. I was also thinking maybe prog should live above the system region? Like maybe reading from the PC could use a special memory op.

risc0/zkp/src/verify/mod.rs Outdated Show resolved Hide resolved
pub const STACK: Region = Region::new(0x0200_0000, mb(16));
pub const DATA: Region = Region::new(0x0300_0000, mb(16));
pub const HEAP: Region = Region::new(0x0400_0000, mb(32));
pub const TEXT: Region = Region::new(0x0600_0000, mb(32));
pub const OUTPUT: Region = Region::new(0x0800_0000, mb(32));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to have a specific output + commit region anymore? As far as I can tell there is no reason output can't just send heap pointers to the host. Also for the commits, cant we just have a single SHA256 state and a single 16 word staging buffer (both in the heap), and just write words to the staging buffer and kick off a compress whenever it's full?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, that's a good point on both counts. Now that we have sys_sha_buffer, we can probably drop the COMMIT region. I also don't think we need an OUTPUT region anymore!


let elf_sha = Sha256::new()
// Method ID calculation is slow, so only recalculate it if we
// actually get a different ELF file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct to assume that imageId calculation is much more efficient so we no longer need to cache it or should we still consider caching the imageID to avoid recalculating?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to be pretty fast without having to cache it. Caching comes with all sorts of complexity and potential for bugs. If we find out it's still too slow we can bring caching back.

@flaub flaub enabled auto-merge (squash) January 17, 2023 00:05
@flaub flaub merged commit 3480396 into main Jan 17, 2023
@flaub flaub deleted the flaub/page_fault branch January 17, 2023 00:29
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.

None yet

4 participants