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

Add userspace support for the Key/Value System #2928

Merged
merged 13 commits into from
Mar 8, 2022

Conversation

alistair23
Copy link
Contributor

@alistair23 alistair23 commented Dec 21, 2021

Pull Request Overview

This exposes Tock's Key/value system to userspace.

Testing Strategy

Running the libtock-c example app on OpenTitan.

TODO or Help Wanted

  • Enforce permissions

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@github-actions github-actions bot added HIL This affects a Tock HIL interface. tock-libraries This affects libraries supported by the Tock project WG-OpenTitan In the purview of the OpenTitan working group. labels Dec 21, 2021
capsules/src/kv_store.rs Outdated Show resolved Hide resolved
capsules/src/kv_store.rs Outdated Show resolved Hide resolved
capsules/src/kv_driver.rs Show resolved Hide resolved
capsules/src/kv_driver.rs Outdated Show resolved Hide resolved
})
}

fn check_queue(&self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems out of place here, but, I feel like you've come this far, maybe it's worth doing the userspace virtualization too?

libraries/tock-tbf/src/types.rs Outdated Show resolved Hide resolved
@alistair23 alistair23 force-pushed the alistair/storage-ids-capsule branch 2 times, most recently from 00bb0dc to c421903 Compare January 5, 2022 01:54
@alistair23
Copy link
Contributor Author

Just pushed an update!

boards/components/src/kv_system.rs Outdated Show resolved Hide resolved
boards/components/src/kv_system.rs Outdated Show resolved Hide resolved
boards/components/src/kv_system.rs Outdated Show resolved Hide resolved
boards/opentitan/src/main.rs Show resolved Hide resolved
boards/opentitan/src/main.rs Outdated Show resolved Hide resolved
capsules/src/kv_store.rs Outdated Show resolved Hide resolved
capsules/src/kv_store.rs Outdated Show resolved Hide resolved
capsules/src/kv_store.rs Outdated Show resolved Hide resolved
capsules/src/kv_driver.rs Outdated Show resolved Hide resolved
Comment on lines 438 to 441
/// Get the process `write_id`.
/// Returns `None` if a `write_id` is not included.
fn get_write_id(&self) -> Option<u32>;

/// Get the `read_ids`.
/// Returns `None` if a `read_ids` is not included.
fn get_read_ids(&self) -> Option<[u32; 8]>;

/// Get the number of `read_ids`.
/// Returns `None` if a `read_ids` is not included.
fn num_read_ids(&self) -> Option<usize>;

/// Get the `access_ids`.
/// Returns `None` if a `access_ids` is not included.
fn get_access_ids(&self) -> Option<[u32; 8]>;

/// Get the number of `access_ids`.
/// Returns `None` if a `access_ids` is not included.
fn num_access_ids(&self) -> Option<usize>;

Copy link
Contributor

Choose a reason for hiding this comment

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

All boards pay for the code size of this support, even if they do not use the KV system. Looks like it is about 250 bytes per board, which is of course not a ton. I think long-term it is important for Tock to find ways to expose TBF header extensions to capsules in a configurable manner, but won't ask to block this PR on that support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully long term we can eliminate virtual functions with rust-lang/rust#68262

@alistair23 alistair23 force-pushed the alistair/storage-ids-capsule branch 2 times, most recently from 1582745 to 61caaac Compare January 31, 2022 05:20
hudson-ayers
hudson-ayers previously approved these changes Feb 1, 2022
twilfredo
twilfredo previously approved these changes Feb 4, 2022
@hudson-ayers hudson-ayers added the last-call Final review period for a pull request. label Feb 7, 2022
@hudson-ayers
Copy link
Contributor

I marked this last-call, but pinging in case anyone else wants to do a final review

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
If the total length is too long for the buffer, let's at least return
some of the data and an error. This is really useful when trying to
check is a caller has permission to delete a key, as now we don't need a
really large buffer to store the value.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
This is the initial commit of KV Store. KV store is like a virtuliser
and exposes the hil::kv_system in a user friendly way that can be used
by app drivers or the kernel.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@alistair23
Copy link
Contributor Author

Just pushed an update to try and address issues raised by @bradjc in the last OT call

kernel/src/process.rs Outdated Show resolved Hide resolved
@bradjc
Copy link
Contributor

bradjc commented Mar 8, 2022

Take a look at the commits I added. The major change is adding storage_permissions.rs and the StoragePermissions object which encapsulates the storage permissions. I then used this to simplify the kv_store implementation.


fn get_write_permissions(&self) -> WritePermissions {
let write_id = self.header.get_write_id().unwrap_or(0);
let (write_count, write_storage_ids) = self
Copy link
Contributor Author

Choose a reason for hiding this comment

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

access_count, access_storage_ids

/// Returns `None` if `access_ids` are not included.
fn get_write_permissions(&self) -> WritePermissions;
/// Get the storage permissions for the process.
fn get_storage_permissions(&self) -> Option<storage_permissions::StoragePermissions>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returns None if the process has no storage permissions.

@alistair23
Copy link
Contributor Author

Looks good

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

Let me know if you are happy with this or want to do more testing.

@alistair23
Copy link
Contributor Author

Looks good to me

@bradjc
Copy link
Contributor

bradjc commented Mar 8, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 8, 2022

@bors bors bot merged commit 719c21e into tock:master Mar 8, 2022
bors bot added a commit to tock/libtock-c that referenced this pull request Mar 11, 2022
273: Add userspace support for the Key/Value System r=hudson-ayers a=alistair23

This uses tock/tock#2928 and access and tests Tock's KV System

Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
tyler-potyondy pushed a commit to tyler-potyondy/libtock-c that referenced this pull request Mar 13, 2024
273: Add userspace support for the Key/Value System r=hudson-ayers a=alistair23

This uses tock/tock#2928 and access and tests Tock's KV System

Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HIL This affects a Tock HIL interface. kernel last-call Final review period for a pull request. tock-libraries This affects libraries supported by the Tock project WG-OpenTitan In the purview of the OpenTitan working group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants