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

Interest in Tock 2.x Port? #579

Closed
L0g4n opened this issue Jan 8, 2023 · 5 comments
Closed

Interest in Tock 2.x Port? #579

L0g4n opened this issue Jan 8, 2023 · 5 comments
Assignees

Comments

@L0g4n
Copy link
Contributor

L0g4n commented Jan 8, 2023

Hello there,

as I am working toward getting OpenSK running on the OpenTitan FPGA platform, I successfully ported the existing develop branch from the Tock 1.x base to Tock 2.1.x. This includes all the necessary breaking fixes such as the new SyscallDriver trait with the new ro_allow and rw_allow as well as the new userspace part.

Of course this has still rough edges, e.g. the bootloader and examples are still untouched and hence do not work, and a fork of the libtock-rs to get static heap support (the v2 version of libtock-rs has still a lot of rough edges, unfortunately). However, most rough edges in my fork are due to the fact of the top priority to get it to work on the existing nrf5280 boards in the first place, which it does, I tested in on both the development board and dongle in practice.

Those things can be smoothed out, however, this only really makes sense if you, the project owners, are interested in this kind of stuff in the first place. If yes, I could submit a draft pull request in the first place and then we could see where this leads. Otherwise I would leave it in my fork untouched for others to potentially use that version, but of course, contributing back to upstream would be better since it has much better visibility 😄.

Let me know what you think.

@kaczmarczyck kaczmarczyck self-assigned this Jan 9, 2023
@kaczmarczyck
Copy link
Collaborator

Hi!

Thanks for putting in the effort. I checked your branch, and there is a lot going on :) We are interested in reviewing your changes and merging them into develop. We probably have to discuss logistics, like:

  • Will develop be in an unusable state for some time (which might be acceptable), do you plan to send one big PR?
  • Your PR doesn't touch any patches, will we still need any of them? What will be the difference with upstream afterwards?
  • To make sure we don't break you after we merge your code, we need a new board to test?
  • Any questions / observations you had around OpenSK?

Question out of curiosity, if I may ask: Your profile mentions that you write a thesis, may I ask what it's about? In case you want to shoot us an email for this or all the logistics above, send it to me or [email protected] .

PS: A few random observations:

  • Why is the bootloader affected? I doesn't use Tock, and is otherwise unrelated?
  • The logic (src/ctap/) is almost untouched. Which means Env is doing an okay job so far.
  • You seem to be touching Easier porting to other architectures #573 , at least you have in a comment in your changes that seems to fix the mentioned problem.

    We can not use an AtomicBool on platforms that do not support atomics,

@L0g4n
Copy link
Contributor Author

L0g4n commented Jan 9, 2023

Hey,

Will develop be in an unusable state for some time (which might be acceptable), do you plan to send one big PR?

I haven't given this much thought really, my initial idea was one big draft PR in the beginning and after the review has been completed (and the tests don't break), but I would also be open to smaller patches if that is easier to merge for the maintainers.

Your PR doesn't touch any patches, will we still need any of them? What will be the difference with upstream afterwards?

That's because I am working with a fork of tock, I integrated the patches directly (in the repo) as it was/is easier for me during the dev workflow, as I did not need to mess with the whole patch thing you are doing. Of course that changes against tock would have to be "patched out" before. The patches are comparable against what you currently have (uicr patch and mpu I did not need, although I forgot to mention the whole firmware_protect driver is still missing in my version, as it's not really necessary for getting things to work in the first place).

To make sure we don't break you after we merge your code, we need a new board to test?

Yeah, that seems to make sense. Currently I don't yet know what additional things need adjustment for opentitan (except the whole atomics things) as I am not there yet. I think the whole flash driver could pose some problem, IIRC, as there does not seem be one in tock upstream yet. Of course the whole thing could still work without persistent storage, as there is not tapeout for opentitan at the moment and the whole storage is not persistent on the FPGA anyways.

Why is the bootloader affected? I doesn't use Tock, and is otherwise unrelated?

Eh, tbh I didn't look at the whole bootloader source code yet as I don't need to use it 😄 But if you say it does not use tock forget my whole statement :)

The logic (src/ctap/) is almost untouched. Which means Env is doing an okay job so far.

Yep, I only really needed to adjust for the new libtock-rs api in the userspace application, most work had to be done in the kernel.

You seem to be touching #573 , at least you have in a comment in your changes that seems to fix the mentioned problem.

That's right, I am using a mutable static as a "solution" right now for platforms without atomics. As long as we are running on one thread I think that should be safe (again see my thoughts about that in the safety comment).

Question out of curiosity, if I may ask: Your profile mentions that you write a thesis, may I ask what it's about? In case you want to shoot us an email for this or all the logistics above, send it to me or [email protected] .

Basically I want to implement a fully open source FIDO2 security key in my thesis, using OpenSK as the software implementation on top of OpenTitan. My professor is already in contact with the OpenTitan guys, but I guess as you are separate teams in google you don't know about it yet. However, if you want to know I can write an email with more information if you like?

@kaczmarczyck
Copy link
Collaborator

I haven't given this much thought really, my initial idea was one big draft PR in the beginning and after the review has been completed (and the tests don't break), but I would also be open to smaller patches if that is easier to merge for the maintainers.

Not a blocker either way, just something to think about. Smaller PRs speed up the process. But you can just start with one big PR so we have an overview, and then we can take it from there.

That's because I am working with a fork of tock, I integrated the patches directly (in the repo) as it was/is easier for me during the dev workflow, as I did not need to mess with the whole patch thing you are doing. Of course that changes against tock would have to be "patched out" before. The patches are comparable against what you currently have (uicr patch and mpu I did not need, although I forgot to mention the whole firmware_protect driver is still missing in my version, as it's not really necessary for getting things to work in the first place).

Using submodules is definitely the right thing to do. Makes review nicer than in patch files, too. One reason we don't commit to submodules is because we would have to maintain a fork. I'll think about if we want to to that. Probably, we just convert all Tock changes to patches after review.

Regarding the missing drivers in patches, I don't think there are any blockers, it's just some extra work we have to do?

Yeah, that seems to make sense. Currently I don't yet know what additional things need adjustment for opentitan (except the whole atomics things) as I am not there yet. I think the whole flash driver could pose some problem, IIRC, as there does not seem be one in tock upstream yet. Of course the whole thing could still work without persistent storage, as there is not tapeout for opentitan at the moment and the whole storage is not persistent on the FPGA anyways.

Do you plan to still look into persistent storage? Also, do you have one of our Nordic boards for testing?

Basically I want to implement a fully open source FIDO2 security key in my thesis, using OpenSK as the software implementation on top of OpenTitan. My professor is already in contact with the OpenTitan guys, but I guess as you are separate teams in google you don't know about it yet. However, if you want to know I can write an email with more information if you like?

Your guess is correct :) A quick email can't hurt, given that we seem to be doing research in the same field!

@L0g4n
Copy link
Contributor Author

L0g4n commented Jan 9, 2023

But you can just start with one big PR so we have an overview, and then we can take it from there.

I like that idea. Basically first the overview and then breaking it apart.

Using submodules is definitely the right thing to do. Makes review nicer than in patch files, too. One reason we don't commit to submodules is because we would have to maintain a fork. I'll think about if we want to to that. Probably, we just convert all Tock changes to patches after review.

Yes, definitely better, as I said, I just did that to get to that "MVP" state first.

Do you plan to still look into persistent storage? Also, do you have one of our Nordic boards for testing?

I had quite some debugging to do with the persistent storage driver ;) It is included in my fork and works in practice on the nrf52840 variants (tested on the dongle and the development board). Just mentioned the potential problems that occur on OpenTitan with the flash driver (no way for me to verify the persistence on an OpenTitan FPGA with volatile storage).

A quick email can't hurt, given that we seem to be doing research in the same field!

Sure, will do that ;)

@kaczmarczyck
Copy link
Collaborator

Fixed in #580 and #620

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

No branches or pull requests

2 participants