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

Hint IO in the SDK #631

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Hint IO in the SDK #631

wants to merge 41 commits into from

Conversation

matthiasgoergens
Copy link
Collaborator

@matthiasgoergens matthiasgoergens commented Nov 26, 2024

Here we are implementing hints for the SDK. See this example Ceno application for how to use the new functionality.

By Ceno application, we mean the combination of guest program and host support.

See #607

Closes #639

Works towards #179

Builds on #754

@matthiasgoergens matthiasgoergens changed the title Prototypes for hint IO Prototypes for hint IO (WIP) Nov 26, 2024
ceno_host/src/lib.rs Outdated Show resolved Hide resolved
ceno_host/src/lib.rs Outdated Show resolved Hide resolved
let (prefix, lengths, suffix) = unsafe { buf.align_to::<u32>() };
assert!(prefix.is_empty());
assert!(suffix.is_empty());
let mut iter = lengths.iter().map(|len| {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would be part of our library, and not visible in normal guest code.

Ok(())
}

pub fn finalise(&self) -> SerialisedCenoStdin {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, this is a bit of a mess. I think it might be comparatively easier to understand the memory format by looking at the readers first, and then you can try to decipher this function. (Or you can wait for the cleanup up version. :)

The main complexity here comes from me having mixed all the padding logic throughout. What we are actually doing ain't very complicated conceptually.

impl<'b> SerialisedCenoStdinIter<'b> {
pub fn read<'a, T>(&'a mut self) -> &'b T
where
T: Portable + for<'c> CheckBytes<HighValidator<'c, Error>>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the guest we would change Error to Failure here. rancor::Error keeps track of error messages, which is useful for the prototype and in the host. rancor::Failure only keeps track of whether we have an error or not, so that's simpler and faster for the guest.

println!("buf.len() after offset padding: {}", buf.len());
for (offset, item) in izip!(offsets, &self.items) {
buf.extend_from_slice(item);
buf.extend_from_slice(&vec![0; buf.len().next_multiple_of(16) - buf.len()]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it will be more neat if just call extend_from_slice once directly to buf.len().next_multiple_of(16) then use copy from slice to fill the prefix data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, let me look into that.

}

pub fn finalise(&self) -> SerialisedCenoStdin {
// TODO: perhaps don't hardcode 16 here.
Copy link
Collaborator

@hero78119 hero78119 Nov 28, 2024

Choose a reason for hiding this comment

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

IIUC 16 means align with 16 "byte", which means 4 u32 word, I guess it's for rkyv format. But I am wondering does rkyv support configurable 4 bytes, i.e. 1 word? in this case all memory access still align u32 so we can use "LW" instruction to do everything and overall format still more compact

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rkyv specifically wants 16 bytes (unless you use the unaligned feature), which is 4 words on u32.

No worries, the alignment to 16 bytes is only a requirement for the backing storage. Internally rkyv only uses one eg 4 bytes to store a u32 in a struct or array somewhere, they don't blow it up to 16 bytes for every single element.

ceno_host/src/lib.rs Outdated Show resolved Hide resolved
where
T: Portable + for<'c> CheckBytes<HighValidator<'c, Error>>,
{
rkyv::access::<T, Error>(self.read_slice()).unwrap()
Copy link
Collaborator

@hero78119 hero78119 Nov 28, 2024

Choose a reason for hiding this comment

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

if I remember correctly, from your previous example, we can throw the whole deliberate slice into rkyv and iterating through it by rkyv iterator implementation, which means we dont need to implementing a wrapping iterator by ourself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in principle. But that only works, if we make sure that the end of the mapped data always shows up at a consistent fixed memory address. rkyv finds its data from the end.

Since we decided that our VM can only put the start of our mapped data at a fixed consistent address, we would need to eg store the length of the mapped data at the fixed start of the region.

But once we are mucking around with bytes ourselves, we might as well do the whole outer layer by ourselves, since it's very simple.

There's some additional complexity required for directly storing rkyv-serialised data inside of rkyv in such a way that you can zero-copy deserialise both layers. The extra complexity is mostly around the 16 byte alignment requirement. That's why my original prototype used the unaligned feature.

It's totally doable, but then probably not really simpler than coding up this outer layer ourselves.

However, I'm open to using rkyv for the outer layer as well, if you think that's cleaner. The code in here is just a prototype.

@matthiasgoergens matthiasgoergens mentioned this pull request Dec 6, 2024
@matthiasgoergens matthiasgoergens changed the title Prototypes for hint IO (WIP) Hint IO in the SDK Dec 6, 2024
@icemelon
Copy link
Member

Could you add some example that showcases how CenoStdin can be used?

@matthiasgoergens
Copy link
Collaborator Author

Could you add some example that showcases how CenoStdin can be used?

Yes, I already added them. Have a look at the PR description for the link.

I can add some nob-standalone examples, too.

@icemelon
Copy link
Member

Could you add some example that showcases how CenoStdin can be used?

Yes, I already added them. Have a look at the PR description for the link.

I can add some nob-standalone examples, too.

Yeah, sorry missed that in the description. Why not include this example app in this PR?

use crate::{ByteAddr, EmuContext, VMState, WordAddr};

const WORD_SIZE: usize = 4;
const INFO_OUT_ADDR: WordAddr = ByteAddr(0xC000_0000).waddr();
Copy link
Member

Choose a reason for hiding this comment

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

is this a placeholder for now until we have the public IO?

Copy link
Collaborator Author

@matthiasgoergens matthiasgoergens Dec 13, 2024

Choose a reason for hiding this comment

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

It's a placeholder of sorts, but for debug output, not for public IO.

We have an open issue to improve debug output.

I just moved the existing functionality around in this PR, because I want to make it accessible to other crates, instead of just using it in the internal tests. But I didn't actually change how it works here.

@matthiasgoergens
Copy link
Collaborator Author

Could you add some example that showcases how CenoStdin can be used?

Yes, I already added them. Have a look at the PR description for the link.

I can add some non-standalone examples, too.

Yeah, sorry missed that in the description. Why not include this example app in this PR?

Yes, I will add a non-standalone example in this PR today. (I can't really add the standalone example in this PR, because it's supposed to be something that lives in a separate repository, to show that we can do that.)

Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

A high-level comment or question. Should we restructure and rename the packages like having a ceno_sdk directory that encloses ceno_emul, ceno_host, and ceno_guest?

@matthiasgoergens
Copy link
Collaborator Author

A high-level comment or question. Should we restructure and rename the packages like having a ceno_sdk directory that encloses ceno_emul, ceno_host, and ceno_guest?

For convenience of building and cargo check etc, we have separated the packages along the lines of native vs RiscV. Otherwise, the native, ie ARM64 or x86_64, build doesn't like eg our inline RiscV assembly.

Your suggestion makes logical sense, but I suspect we will want to keep the separation by target architecture around, too. Or use the same workaround as SP1 does, and use conditional compilation to hide the inline assembly and other 'offensive' items from the native build.

@matthiasgoergens
Copy link
Collaborator Author

Yeah, sorry missed that in the description. Why not include this example app in this PR?

Done!

Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

We should still move ceno_rt to the top level directory, in parallel with ceno_host. The hint program example is better to put in the $ROOT/examples if possible.

@matthiasgoergens
Copy link
Collaborator Author

We should still move ceno_rt to the top level directory, in parallel with ceno_host. The hint program example is better to put in the $ROOT/examples if possible.

See #754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Write example program using hints
3 participants