-
Notifications
You must be signed in to change notification settings - Fork 290
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
Overview: Add tock v2 support #580
Conversation
Thanks @L0g4n for the work you put in this PR! @kaczmarczyck is currently on vacation and I'm a currently a bit busy, but I'll take a look next week and provide some comments. Some early comment though that might simplify your workflow: You can use the |
Thanks, will try do that. I originally had multiple patches but then just merged it one big file since some of the stuff commits in there revert some earlier changes since this has been a bit of an exploratory journey. |
Quick question regarding the |
I'm not sure I understand the question, but I'll try another explanation which might help. The logic of the script is to provide you a way to sync between the patches and the chain of commits of the submodule. The Here are a few examples.
Note that you can combine any such operations. You just want the chain of commits to be alphabetically ordered and represent the set of patches you want. You can use any git operations you want to modify the chain of commits (including inserting, deleting, modifying, splitting, merging, reordering, renaming commits). |
Alright, however, I did not start with the patches as the source of truth, but rather with the tock submodule and then added my commits into the submodule. Can I simply edit the files, add the commits, rebase, and then call P.S: I changed this PR into a draft to avoid getting a load of emails because of failed CI. |
No you don't need to start with the patches as source-of-truth. You can just
Why do you need to rebase? I'm assuming you start from commit 55c73f1c9f3c0966d975458a9564ae430a5919af in tock, edit files until it works, commit changes as you go, and call
Sounds good. |
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 didn't have time to review everything, but went over most of it (at least quickly). I think overall it looks to go in the right direction. I'm wondering though if we need the code to be generic over those libtock_platform
traits or if we can define the actual types somewhere (gated by appropriate cfg
) and then use those types directly. Because it doesn't look like we need to instantiate differently in the same binary. This would remove a bit of boilerplate.
@@ -14,3 +14,6 @@ target/ | |||
/reproducible/elf2tab.txt | |||
/reproducible/reproduced.tar | |||
__pycache__ | |||
|
|||
# ctags | |||
**/tags* |
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.
Where do those come from? You're not using rust-analyzer? Or do we have some C files?
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.
Ah, these come from my neovim tooling, more specifically gutentags
. I do use rust-analyzer, this artefacts are automatically generated when I open sth. nvim. Can probably be removed if annoying.
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.
There should be a way to not generate them. I'm not a partisan of having non-common tooling configuration in the repo, but if others believe that's fine, then fine by me.
Regardless I would just use tags*
, the **/
prefix is redundant. Could you provide example filenames to get an idea of the sort of suffixes that gutentags create?
|
||
[profile.release] | ||
panic = "abort" | ||
lto = true # Link Time Optimization usually reduces size of binaries and static libraries | ||
lto = true # Link Time Optimization usually reduces size of binaries and static libraries |
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.
Out of curiosity, what editor do you use? We currently don't enforce any formatting on TOML files, but maybe we should if there are easy solutions. (I just checked and looks like taplo
would be an LSP server.)
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.
For rust I typically use vscode with rust analyzer. Sometimes though for quick editing I also use neovim.
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.
Do you know which one (VSCode or neovim) formats TOML files by trying to "align" line-suffix comments?
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 think this one is caused by the VSCode Plugin "better-toml": https://github.com/bungcip/better-toml
Although I see that is archived now ^^
patches/tock/0004-vendor-hid.patch
Outdated
@@ -155,163 +153,3 @@ index f7899d8c5..6956523c6 100644 | |||
} else { | |||
hil::usb::CtrlSetupResult::ErrGeneric | |||
} | |||
diff --git a/capsules/src/usb/usbc_ctap_hid.rs b/capsules/src/usb/usbc_ctap_hid.rs |
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 guess this is removed because work in progress.
Thanks, I added some comments to your remarks, in particular about the platform impl stuff. Edit: BTW, ignore the "ready for review" thing for now, I just changed it from draft to normal PR again as it still informed about failing CI ;) |
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.
Thanks @ia0 for the first review.
Do you have a general opinion on USB? Does Tock v2 have better support and we should follow more closely, rather than maintaining our own stack? Did you already get something to work?
Next steps for you are probably to finish the missing drivers?
I haven't looked into the submodules. But first of all thanks for this big contribution!
|
||
/// Dummy buffer that causes the linker to reserve enough space for the stack. | ||
#[no_mangle] | ||
#[link_section = ".stack_buffer"] | ||
pub static mut STACK_MEMORY: [u8; 0x1000] = [0; 0x1000]; | ||
pub static mut STACK_MEMORY: [u8; 0x2000] = [0; 0x2000]; |
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.
FYI: We had to do this sometimes in v1 already. Out of curiosity, did you observe memory problems often before you made this change? I noticed you made comments about stack sizes elsewhere, so I was wondering if you can share some of your gained wisdom :)
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.
Nope, this is simply here because it was done in v1. I did no fine tuning with the memory limits, priority was first to get it to work :)
} | ||
|
||
# The following value must match the one used in the file | ||
# `src/entry_point.rs` | ||
APP_HEAP_SIZE = 90000 | ||
APP_HEAP_SIZE = 90_000 |
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.
FYI: Not sure if this value is still accurately reflecting how much we should use.
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 see. I wondered how you came up with this specific heap limit, do you have some measurements with the certified versions that give a rough estimate how much heap is needed for allocation? Because this seems to be a rather large value.
@@ -30,6 +30,12 @@ pub struct MainHid { | |||
wink_permission: TimedPermission, | |||
} | |||
|
|||
impl Default for MainHid { |
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.
Where is this used?
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.
In here, nowhere. This was added (as countless other times) to satisfy the clippy lints.
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.
We can move the implementation of new
to default
then. And call ::default()
instead of ::new()
.
} | ||
|
||
util::flash_all_leds(); | ||
#[cfg(not(feature = "panic_console"))] |
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.
Can we always flash the LEDs?
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.
Problem is, when we have the panic_console
active, both the TockSyscalls::exit_terminate
and the flash_all_leds
both never return, as they have a return type of !
. This will spit out warnings by the rust compiler.
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.
Ah right, if the above never returns, no point in having code after.
Eh, I don't really have an opinion on this. TBH, I did not ready anything newsworthy in the Tock v2 release notes that mention something in regard to USB. Do you mean with better support the upstream USB_CTAP capsule? The code is a lot shorter, and seems a bit more elegant (as they seem not to have handle as much conditionals), fewer limitations (more than one client for example, althought this is not really relevant), but I don't know for certain how it compares in detail.
I planned to work in first all the relevant review comments, and check for review after that. As you are talking about drivers in plural, what else is still missing besides |
Yours seems to work, so we don't have to bother with USB for now it seems. The question is orthogonal then, but I was curious. I played around with the firmware a bit, and it seems to work. For RTT (as expected), I had to flip the bool in More comments:
Which means I am happy. Also I didn't see any substantial or blocking comment. We can go ahead and think about how to submit. |
9ffba32
to
70df9bb
Compare
This includes new patches for the TockOS 2.1-dev base and the libtock-rs 2.0 (WIP) submodules, with the addition of the new pinned commits for both submodules.
This has still some very rough edges, hence, this is considered a WIP commit. Next to the missing `firmware_protect` driver for TockOs (for disabling JTAG support and preventing dumping of the firmware), this includes non-working examples. Additionally, it is important to note that OpenSK can not be built anymore on x86 host machines, since `libtock_runtime` (part of libtock-rs 2.0 WIP) is only running on ARM and riscv hosts. Hence a normal invocation of `cargo build` or `cargo check` on your average dev machine will fail, but you can still cross-compile for ARM or RISCV and/or run `cargo check --features std` to see if the code compiles fine, but you **need** to apply the *alloc_init* patch for the libtock-rs submodule before running the build commands. Change submodules back to upstream
In the last commits I did some work and rebased the tock patches on top of the stable tock 2.1.1 release (despite released later than previous state on master, we're not actually now on an earlier state of the tock repo); I also updated the nightly in this session to the aforementioned one. I did not have the chance to test it on real hardware though yet, so if you got time you can do that to see if there were hopefully no regressions in Tock 😀. As you might have noticed (or not), I am currently not so active working on this PR, as my thesis deadline in Mid-April approaches, I'm currently kinda focusing on getting the OpenTitan port to work (mostly NVMC driver in the flash_ctrl for persistent storage, and a slight variation of the button driver for using the user DIP switches as buttons for the user presence). |
Tested on the development kit, and still works. Thanks for the update on your available time. |
We merged #602 to move all CTAP logic into its own library. You mostly shouldn't need to touch this part of the code anymore. #603 fixes most of the lints in So when you rebase, all other changes should be in |
Hi, I hope your thesis is going well? Please let me know if you want to continue to land this PR afterwards. On CRP: I realized that there are different variants of the nrf52840 with different versions of CRP. We consider removing CRP support until we have a working implementation for the latest chip. I wanted to share since that was the last piece you were missing, and we can agree to ignore it. |
Hey there, thanks for asking: Yes, I submitted my thesis this week. I did not reach all goals I originally set for myself considering the time available (mostly having to do with OpenTitan blocking things), but overall I'm still positive about it 😀
About the question: Yes, I still want to land this PR, since CRP is not anymore required, what is still missing in this PR besides the rebase work with the CTAP library? |
Introduced in google#580 The conversion to libtock's ErrorCode has to happen outside of the library.
I doubled checked, and this should be it. I expect that your rebase does not touch Concerning the TockEnv function: Thanks for your help! |
Introduced in #580 The conversion to libtock's ErrorCode has to happen outside of the library.
You know, I think it might be better if you finish this PR off. That's quite a lot of steps for rebasing (n = 44 and quite some merge conflicts), and I'm not really familiar with all the code changes in the background that you did (removed rng256, crypto changes, ...) and don't want to mess things up, plus it has been a while since I actively worked on this code. And to be honest, fixing all the merge conflicts is not something I like to do for fun :) |
fn get_info(nr: usize, arg: usize) -> StorageResult<usize> { | ||
let code = syscalls::command(DRIVER_NUMBER, command_nr::GET_INFO, nr, arg); | ||
code.map_err(|_| StorageError::CustomError) | ||
fn get_info<S: Syscalls>(nr: u32, arg: u32) -> StorageResult<u32> { |
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.
Should we put these syscall
functions in a struct similar to those in third_party/libtock-drivers/src/
?
pub trait Config: platform::allow_ro::Config + platform::subscribe::Config
{
}
impl<T: platform::allow_ro::Config + platform::subscribe::Config> Config for T
{
}
pub struct Storage<S: Syscalls + RawSyscalls, C: Config = DefaultConfig>(S, C);
impl<S: Syscalls + RawSyscalls, C: Config> Storage<S, C> {
pub fn get_info(nr: u32, arg: u32) -> StorageResult<u32> {
let info = S::command(DRIVER_NUMBER, command_nr::GET_INFO, nr, arg)
.to_result::<u32, ErrorCode>()
.map_err(|_| StorageError::CustomError)?;
Ok(info)
}
pub fn memop(nr: u32, arg: u32) -> StorageResult<u32> {
let registers = unsafe { S::syscall2::<{ syscall_class::MEMOP }>([nr.into(), arg.into()]) };
let r0 = registers[0].as_u32();
let r1 = registers[1].as_u32();
// make sure r0 is the `success with u32` (129) return variant and then return the value in r1 as u32
// see: https://github.com/tock/tock/blob/master/doc/reference/trd104-syscalls.md#32-return-values
match (r0, r1) {
(129, r1) => Ok(r1),
(_, _) => Err(StorageError::CustomError),
}
}
pub fn write_slice(
ptr: usize,
value: &[u8],
) -> StorageResult<()> {
share::scope(|allow_ro| {
S::allow_ro::<C, DRIVER_NUMBER, { ro_allow_nr::WRITE_SLICE }>(allow_ro, value)
.map_err(|_| StorageError::CustomError)?;
block_command::<S, C>(
DRIVER_NUMBER,
command_nr::WRITE_SLICE,
ptr as u32,
value.len() as u32,
)
})
}
pub fn erase_page(
ptr: usize,
page_length: usize,
) -> StorageResult<()> {
block_command::<S, C>(
DRIVER_NUMBER,
command_nr::ERASE_PAGE,
ptr as u32,
page_length as u32,
)
}
fn block_command(
driver: u32,
cmd: u32,
arg1: u32,
arg2: u32,
) -> StorageResult<()> {
let called: Cell<Option<(u32,)>> = Cell::new(None);
share::scope(|subscribe| {
S::subscribe::<_, _, C, DRIVER_NUMBER, { subscribe_nr::DONE }>(subscribe, &called)
.map_err(|_| StorageError::CustomError)?;
S::command(driver, cmd, arg1, arg2)
.to_result::<(), ErrorCode>()
.map_err(|_| StorageError::CustomError)?;
libtock_drivers::util::Util::<S>::yieldk_for(|| called.get().is_some());
if called.get().unwrap().0 == 0 {
Ok(())
} else {
Err(StorageError::CustomError)
}
})
}
}
unsafe fn read_slice(address: usize, length: usize) -> &'static [u8] {
core::slice::from_raw_parts(address as *const u8, length)
}
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.
What's the benefit? The calls to these functions are not getting shorter, and the functions are private, so it's not really an API design advantage?
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 was picturing we would end up putting this in third_party/libtock-drivers/src
. Is there a reason it is kept here?
Understood. Thanks for all your work so far! I'm almost done with the rebased PR, just needs more testing now. It sits here until then, for the curious: |
* Changes from #580 * fixes USB cancel panic * style fixes * Update src/env/tock/storage.rs Co-authored-by: Zach Halvorsen <[email protected]> --------- Co-authored-by: Zach Halvorsen <[email protected]>
Merged in #620 |
Fixes #579
This is the starting point for the maintainers to get an overview of all changes (and can be split up, after further discussion) and hence this PR has not been opened with the intention to merge in this state.
Important things to note
These circumstances probably mean that with the current CI config the pipeline will for sure fail and needs further modifications. Let me know what you think, where problematic areas are, and how we can continue from here.