Skip to content

Update to zerocopy 0.8 #2054

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

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

Update to zerocopy 0.8 #2054

wants to merge 14 commits into from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented May 1, 2025

This branch updates our dependency on zerocopy from v0.6.x to v0.8.x.
The primary motivation for this change is that I had wanted to use
zerocopy v0.8's support for data-bearing enums in the
gateway-ereport-messages crate I added in
oxidecomputer/management-gateway-service#370, and...I hadn't realized
how painful taking the zerocopy update in Hubris would be. :) But,
it's also just good to stay on top of new dependency versions
regardless.

This is a very large change, since pretty much every place where we
derive or use zerocopy's traits needed to be changed slightly to use
the new APIs, but for the most part, it's not actually that
interesting, so reviewing it should be pertty straightforward. The
main API changes that are worth noting are:

  • AsBytes is now called IntoBytes, which was an easy update.
  • All the _from_prefix/_from_suffix APIs now return the rest of the
    slice, which is a nice improvement --- previously we would manually
    split off the rest of the slice when using those functions.
  • Conversions from bytes now return Results instead of Options,
    which very trivial changes in a few places.
  • LayoutVerified is replaced by a new Ref type, but other API
    changes mean that you now basically never need to use it, which rocks!
  • Some methods were renamed, which was also a pretty trivial
    find-and-replace.
  • zerocopy adds new Immutable and KnownLayout marker traits.
    Immutable is required for IntoBytes::as_bytes(); types which do
    not derive it are now assumed to have interior mutability and only
    provide IntoBytes::as_bytes_mut(). So, basically everything now
    needs to derive Immutable. KnownLayout isn't required for APIs we
    use as commonly but I added it on everything anyway. This was most of
    what made this update annoying.
  • FromBytes::new_zeroed is now provided by a new FromZeros trait,
    but the FromBytes derive implements that trait as well.

There may be some places where we can now make better use of new
zerocopy features. In particular, we can now use zerocopy on
data-bearing enums, which might allow us to replace hubpack with
zerocopy in several places. I didn't do that in this PR, but it's
worth looking into in follow-up changes --- I just wanted to get
everything building with the new API and felt that improving our usage
of it would be better off done in smaller commits.

One other important thing to note is that updating the
gateway-messages dependency increased stack depth in
control-plane-agent from 6000B to 6136B for non-sidecar targets, so I
bumped them up to 6256B. This, in turn, increases RAM to 65525B for
control-plane-agent, which exceeds the max-sizes config, per
@cbiffle's previous advice, I just deleted all the max-sizes for
control-plane-agent tasks.

Furthermore, this branch requires the following changes to other crates
to pick up the latest zerocopy:

Those ought to merge first so we can point our Git dependencies on those
repos back at the main branch.

@hawkw hawkw force-pushed the eliza/zerocopy-0.8 branch from ed42c86 to fd944f7 Compare May 2, 2025 17:21
@hawkw hawkw force-pushed the eliza/zerocopy-0.8 branch from fd944f7 to 22c7f5a Compare May 2, 2025 17:23
hawkw added 3 commits May 2, 2025 12:10
seems like picking up a much more recent `gateway-sp-comms` has
increased `control-plane-agent`'s stack depth to 6136, so i've given it
a stack margin a bit above that. this caused the RAM size to grow to
65535B, so --- as per @cbiffle's general suggestion --- i've
un-max-size'd all the control-plane-agent RAM allocations.
@hawkw hawkw changed the title [WIP] update to zerocopy 0.8 Update to zerocopy 0.8 May 2, 2025
@hawkw hawkw marked this pull request as ready for review May 2, 2025 19:52
@hawkw hawkw requested review from cbiffle, flihp and labbott as code owners May 2, 2025 19:52
Copy link
Collaborator

@labbott labbott left a comment

Choose a reason for hiding this comment

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

Overall LGTM

Comment on lines +148 to +151
humpty = { git = "https://github.com/oxidecomputer/humpty", branch = "eliza/zerocopy-0.8", default-features = false, version = "0.1.3" }
hubtools = { git = "https://github.com/oxidecomputer/hubtools", default-features = false, version = "0.4.6" }
idol = { git = "https://github.com/oxidecomputer/idolatry.git", default-features = false }
idol-runtime = { git = "https://github.com/oxidecomputer/idolatry.git", default-features = false }
idol = { git = "https://github.com/oxidecomputer/idolatry.git", branch = "eliza/zerocopy-0.8", default-features = false }
idol-runtime = { git = "https://github.com/oxidecomputer/idolatry.git", branch = "eliza/zerocopy-0.8", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to merge with that branch or are you going to rebase when those get approved?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i'm waiting for these branches to be approved before merging; i'll update to point the git deps back at the main branches once they're through.

@labbott
Copy link
Collaborator

labbott commented May 6, 2025

I did give this a quick test on my grapefruit and it seemed to work (tasks running, could run faux-mgs state)

@hawkw
Copy link
Member Author

hawkw commented May 6, 2025

humility test passes on gimletlet:

test output
eliza@theseus ~/Code/oxide/hubris $ HUMILITY_TARGET=gimletlet cargo xtask test test/tests-gimletlet/app.toml
    Finished `dev` profile [optimized + debuginfo] target(s) in 0.16s
     Running `target/debug/xtask test test/tests-gimletlet/app.toml`
building crate test-runner
    Finished `release` profile [optimized + debuginfo] target(s) in 0.13s
target/thumbv7em-none-eabihf/release/test-runner -> target/tests-gimletlet/dist/runner.elf
building crate test-suite
    Finished `release` profile [optimized + debuginfo] target(s) in 0.14s
target/thumbv7em-none-eabihf/release/test-suite -> target/tests-gimletlet/dist/suite.elf
building crate test-assist
    Finished `release` profile [optimized + debuginfo] target(s) in 0.13s
target/thumbv7em-none-eabihf/release/test-assist -> target/tests-gimletlet/dist/assist.elf
building crate test-idol-server
    Finished `release` profile [optimized + debuginfo] target(s) in 0.13s
target/thumbv7em-none-eabihf/release/test-idol-server -> target/tests-gimletlet/dist/idol.elf
building crate task-hiffy
    Finished `release` profile [optimized + debuginfo] target(s) in 0.14s
target/thumbv7em-none-eabihf/release/task-hiffy -> target/tests-gimletlet/dist/hiffy.elf
building crate task-idle
    Finished `release` profile [optimized + debuginfo] target(s) in 0.13s
target/thumbv7em-none-eabihf/release/task-idle -> target/tests-gimletlet/dist/idle.elf
linking task 'runner'
linking task 'suite'
linking task 'assist'
linking task 'idol'
linking task 'hiffy'
linking task 'idle'
building crate gimletlet
    Finished `release` profile [optimized + debuginfo] target(s) in 0.13s
target/thumbv7em-none-eabihf/release/gimletlet -> target/tests-gimletlet/dist/kernel
flash   = 0x08000000..0x08100000
ram     = 0x20000000..0x20020000
sram1_mac = 0x30000000..0x30010000
sram1   = 0x30010000..0x30020000
sram2   = 0x30020000..0x30040000
sram3   = 0x30040000..0x30048000
sram4   = 0x38000000..0x38010000
bank2   = 0x08100000..0x08200000
Used:
  flash:   0x18000 (9%)
  ram:     0x8500 (25%)
  sram1_mac: 0x0 (0%)
  sram1:   0x0 (0%)
  sram2:   0x0 (0%)
  sram3:   0x0 (0%)
  sram4:   0x0 (0%)
  bank2:   0x0 (0%)
humility: WARNING: archive on command-line overriding archive in environment file
humility: attaching with chip set to "STM32H753ZITx"
humility: attached to 0483:3754:000B00154D46501520383832 via ST-Link V3
humility: flash/archive mismatch; reflashing
humility: flashing done
humility: WARNING: archive on command-line overriding archive in environment file
humility: attached to 0483:3754:000B00154D46501520383832 via ST-Link V3
Total test cases: 52
humility: running test_send ...ok
humility: running test_recv_reply ...ok
humility: running test_recv_reply_fault ...ok
humility: running test_floating_point_lowregs ...ok
humility: running test_floating_point_highregs ...ok
humility: running test_floating_point_fault ...ok
humility: running test_fault_badmem ...ok
humility: running test_fault_stackoverflow ...ok
humility: running test_fault_execdata ...ok
humility: running test_fault_illop ...ok
humility: running test_fault_nullexec ...ok
humility: running test_fault_textoob ...ok
humility: running test_fault_stackoob ...ok
humility: running test_fault_buserror ...ok
humility: running test_fault_illinst ...ok
humility: running test_fault_divzero ...ok
humility: running test_fault_maxstatus ...ok
humility: running test_fault_badstatus ...ok
humility: running test_fault_maxrestart ...ok
humility: running test_fault_badrestart ...ok
humility: running test_fault_maxinjection ...ok
humility: running test_fault_badinjection ...ok
humility: running test_fault_superinjection ...ok
humility: running test_fault_selfinjection ...ok
humility: running test_panic ...ok
humility: running test_restart ...ok
humility: running test_restart_taskgen ...ok
humility: running test_borrow_info ...ok
humility: running test_borrow_read ...ok
humility: running test_borrow_write ...ok
humility: running test_borrow_without_peer_waiting ...ok
humility: running test_supervisor_fault_notification ...ok
humility: running test_timer_advance ...ok
humility: running test_timer_notify ...ok
humility: running test_timer_notify_past ...ok
humility: running test_task_config ...ok
humility: running test_task_status ...ok
humility: running test_task_fault_injection ...ok
humility: running test_refresh_task_id_basic ...ok
humility: running test_refresh_task_id_off_by_one ...ok
humility: running test_refresh_task_id_off_by_many ...ok
humility: running test_post ...ok
humility: running test_idol_basic ...ok
humility: running test_idol_bool_arg ...ok
humility: running test_idol_bool_ret ...ok
humility: running test_idol_bool_xor ...ok
humility: running test_idol_err_ret ...ok
humility: running test_idol_ssmarshal ...ok
humility: running test_idol_ssmarshal_multiarg ...ok
humility: running test_idol_ssmarshal_multiarg_enum ...ok
humility: running test_irq_notif ...ok
humility: running test_irq_status ...ok
Ran a total of 52 cases
Wrote test output to hubris.testout.0

@hawkw
Copy link
Member Author

hawkw commented May 6, 2025

Additional manual verification: I flashed sled 14 on London to the gimlet-c image from this CI run and it seems to be happy. I've asked the SP for inventory, read sensor values, and power-cycled the host and everything appears to work.

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.

2 participants