The control plane should use libipcc #4536

papertigers · 2023-11-21T04:02:40Z

With oxidecomputer/illumos-gate@779fb5b now integrated we should use libipcc in omicron rather than make direct calls via ioctl. The library will take care of hiding the implementation details from upstream consumers -- this becomes important in the future when communication with the service processor from the host OS physically changes with newer board design.

Currently the only consumer in this repo is installinator but the control plane is about to start communicating with the RoT via IPCC as well.

This PR introduces new bindings around libipcc and removes the old ioctl interfaces.

ipcc/src/handle.rs

ipcc/src/lib.rs

papertigers · 2023-11-27T21:20:22Z

To test this I ran some tests on a bench gimlet.

First off let's make sure installinator can lookup the InstallinatorImageId:

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Nov 27 20:59:15.708 INFO Received prefixes from ddmd, prefixes: {"fe80::2cf4:23ff:fe60:ff36": [PathVector { destination: Ipv6Prefix { addr: fd00:99::, len: 64 }, path: ["oxz_switch"] }]}, DdmAdminClient: [::1]:8000, artifact: ArtifactHashId { kind: ArtifactKind("host_phase_2"), hash: ArtifactHash("0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") }, file: clients/ddm-admin-client/src/lib.rs:119
Nov 27 20:59:15.708 INFO discovered 0 peers: [], artifact: ArtifactHashId { kind: ArtifactKind("host_phase_2"), hash: ArtifactHash("0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") }, file: installinator/src/peers.rs:167
Nov 27 20:59:15.708 WARN unable to fetch artifact from peers, retrying discovery, artifact: ArtifactHashId { kind: ArtifactKind("host_phase_2"), hash: ArtifactHash("0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") }, file: installinator/src/peers.rs:178

Using a butler to adjust the size of the InstallinatorImageId value (too small / too big):

$
butler $ pfexec humility writeword 0x2402b040 122 $((16#707569A3)) $((16#65746164)) $((16#5064695f)) $((16#44434241)) $((16#48474645)) $((16#4c4b4a49)) $((16#504f4e4d)) $((16#736F686C)) $((16#68705F74)) $((16#5F657361)) $((16#01205832)) $((16#05040302)) $((16#09080706)) $((16#0D0C0B0A)) $((16#11100F0E)) $((16#15141312)) $((16#19181716)) $((16#1D1C1B1A)) $((16#6D201F1E)) $((16#746E6F63)) $((16#5F6C6F72)) $((16#6E616C70)) $((16#21205865)) $((16#25242322)) $((16#29282726)) $((16#2D2C2B2A)) $((16#31302F2E)) $((16#35343332)) $((16#39383736)) $((16#3D3C3B3A)) $((16#00403F3E))

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Error: error retrieving installinator image ID

Caused by:
    deserializing installinator image ID failed: i/o error reading from slice: failed to fill whole buffer

butler $ pfexec humility writeword 0x2402b040 124 $((16#707569A3)) $((16#65746164)) $((16#5064695f)) $((16#44434241)) $((16#48474645)) $((16#4c4b4a49)) $((16#504f4e4d)) $((16#736F686C)) $((16#68705F74)) $((16#5F657361)) $((16#01205832)) $((16#05040302)) $((16#09080706)) $((16#0D0C0B0A)) $((16#11100F0E)) $((16#15141312)) $((16#19181716)) $((16#1D1C1B1A)) $((16#6D201F1E)) $((16#746E6F63)) $((16#5F6C6F72)) $((16#6E616C70)) $((16#21205865)) $((16#25242322)) $((16#29282726)) $((16#2D2C2B2A)) $((16#31302F2E)) $((16#35343332)) $((16#39383736)) $((16#3D3C3B3A)) $((16#00403F3E))

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Error: error retrieving installinator image ID

Caused by:
    IPCC key lookup failed for key InstallinatorImageId: buffer too small for value

Change host_phase2 to host_qhase2:

butler $ pfexec humility writeword 0x2402b040 123 $((16#707569A3)) $((16#65746164)) $((16#5064695f)) $((16#44434241)) $((16#48474645)) $((16#4c4b4a49)) $((16#504f4e4d)) $((16#736F686C)) $((16#68715F74)) $((16#5F657361)) $((16#01205832)) $((16#05040302)) $((16#09080706)) $((16#0D0C0B0A)) $((16#11100F0E)) $((16#15141312)) $((16#19181716)) $((16#1D1C1B1A)) $((16#6D201F1E)) $((16#746E6F63)) $((16#5F6C6F72)) $((16#6E616C70)) $((16#21205865)) $((16#25242322)) $((16#29282726)) $((16#2D2C2B2A)) $((16#31302F2E)) $((16#35343332)) $((16#39383736)) $((16#3D3C3B3A)) $((16#00403F3E))

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Error: error retrieving installinator image ID

Caused by:
    deserializing installinator image ID failed: semantic error: missing field `host_phase_2` (offset unknown)

Crashing the SP during key lookup:

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Error: error retrieving installinator image ID

Caused by:
    IPCC key lookup ioctl failed for key InstallinatorImageId: Connection timed out (os error 145)

Remove the key completely:

butler $ pfexec humility writeword 0x2402b040 0

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Error: error retrieving installinator image ID

Caused by:
    IPCC key lookup failed for key InstallinatorImageId: no value for key

Restoring a valid configuration again:

gimlet # ./installinator install --from-ipcc /var/tmp/foo
Nov 27 21:18:50.283 INFO Received prefixes from ddmd, prefixes: {"fe80::2cf4:23ff:fe60:ff36": [PathVector { destination: Ipv6Prefix { addr: fd00:99::, len: 64 }, path: ["oxz_switch"] }]}, DdmAdminClient: [::1]:8000, artifact: ArtifactHashId { kind: ArtifactKind("host_phase_2"), hash: ArtifactHash("0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") }, file: clients/ddm-admin-client/src/lib.rs:119
Nov 27 21:18:50.283 INFO discovered 0 peers: [], artifact: ArtifactHashId { kind: ArtifactKind("host_phase_2"), hash: ArtifactHash("0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") }, file: installinator/src/peers.rs:167
Nov 27 21:18:50.284 WARN unable to fetch artifact from peers, retrying discovery, artifact: ArtifactHashId { kind: ArtifactKind("host_phase_2"), hash: ArtifactHash("0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") }, file: installinator/src/peers.rs:178

papertigers · 2023-11-27T21:22:40Z

.cargo/config

@@ -13,7 +13,7 @@ rustdocflags = "--document-private-items"
 # things critical to correctness probably don't belong here.
 [target.x86_64-unknown-illumos]
 rustflags = [
-    "-C", "link-arg=-Wl,-znocompstrtab"
+    "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64"


This is a workaround I was using to get around a missing run path. The solution is likely somewhere in the rpath crate - I am diverting my attention to fixing this now, but if someone has pointers on how this should be done, that would be greatly appreciated.

Spoke to dap about this and I plan to push a commit to clean up this mess.

Note: opting to go with passing the oxide platform directory here after some discussion in chat.

jordanhendricks

Excited to see this land! Some comments and questions

jordanhendricks · 2023-11-28T20:44:17Z

ipcc/build.rs

+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+#[cfg(target_os = "illumos")]
+static OXIDE_PLATFORM: &str = "/usr/platform/oxide/lib/amd64/";


worth a comment explaining what this path is?

Fixed in 77bd065.

jordanhendricks · 2023-11-28T20:44:46Z

ipcc/src/ffi.rs

+    _marker: core::marker::PhantomData<(*mut u8, core::marker::PhantomPinned)>,
+}
+
+pub(crate) const LIBIPCC_ERR_OK: libipcc_err_t = 0;


missing doc comment here?

Fixed in 77bd065.

jordanhendricks · 2023-11-28T20:45:14Z

ipcc/src/ffi.rs

+    _marker: core::marker::PhantomData<(*mut u8, core::marker::PhantomPinned)>,
+}
+
+pub(crate) const LIBIPCC_ERR_OK: libipcc_err_t = 0;


nit: I would prefer if there are newlines between the constants, since it helps with readability.

Fixed in 77bd065.

jordanhendricks · 2023-11-28T20:46:58Z

ipcc/src/handle.rs

+        LIBIPCC_ERR_NO_MEM => IpccError::NoMem(inner),
+        LIBIPCC_ERR_INVALID_PARAM => IpccError::InvalidParam(inner),
+        LIBIPCC_ERR_INTERNAL => IpccError::Internal(inner),
+        LIBIPCC_ERR_KEY_UNKNOWN => IpccError::Internal(inner),


should this be IpccError::KeyUnknown?

Nice catch -- dang it copy and paste!
Fixed in 77bd065.

jordanhendricks · 2023-11-28T20:48:37Z

ipcc/src/lib.rs

+//! An interface to libipcc (inter-processor communications channel) which
+//! currently supports looking up values stored in the SP by key. These
+//! values are passed from the control plane to the SP (through MGS) or
+//! are maniuplated through the `ipcc` command found on Helios.


nit: "manipulated"

Unnecessary after accepting Andy's change.

jordanhendricks · 2023-11-28T20:50:19Z

ipcc/src/handle_stub.rs

+// Copyright 2023 Oxide Computer Company
+
+use crate::IpccError;
+


maybe add a comment in here that these stubs are for not-illumos platforms?

Fixed in 77bd065.

jordanhendricks · 2023-11-28T20:52:47Z

ipcc/src/lib.rs

+}
+
+/// Interface to the inter-processor communications channel.
+/// For more information see rfd 316.


are there docs for the illumos library as well?

There is no documentation for the library other than the header file. I thought about linking the ipcc driver man page but the point of libipcc is that users don't call the kernel driver interface directly anymore.

There will be man pages for libipcc in the next week or so.

jordanhendricks · 2023-11-28T20:55:45Z

ipcc/src/handle.rs

+
+impl Drop for IpccHandle {
+    fn drop(&mut self) {
+        unsafe {


For each unsafe block, it would be to have a comment explaining why the code is safe across FFI boundaries. An example of what I mean:

omicron/sled-agent/src/swap_device.rs

Lines 434 to 448 in 30d4191

// Safety: CStr::from_ptr is documented as safe if:

// 1. The pointer contains a valid null terminator at the end of

// the string

// 2. The pointer is valid for reads of bytes up to and including

// the null terminator

// 3. The memory referenced by the return CStr is not mutated for

// the duration of lifetime 'a

//

// (1) is true because we initialize the buffers for ste_path as all

// 0s, and their length is long enough to include the null

// terminator for all paths on the system.

// (2) should be guaranteed by the syscall itself, and we can know

// how many entries are valid via its return value.

// (3) We aren't currently mutating the memory referenced by the

// CStr, though there's nothing here enforcing that.

I added safety comments to unsafe blocks that were not calls into libipcc itself since calling any C function has to be marked unsafe. So all the usage of CString and CStr list why they are safe. Let me know if you would like more safety comments elsewhere.

ipcc/src/lib.rs

Co-authored-by: Andy Fiddaman <[email protected]>

jordanhendricks

Thanks! Excited to land this (modulo buildomat worker OS versions getting updated)

citrus-it

Thanks for the work on this, I'm pleased to see the last direct ipcc ioctl user quashed.
Modulo the tests passing, approved.

davepacheco · 2024-01-11T18:02:45Z

Just putting this here in case others search for this error message: this PR appears to move forward the minimum Helios that the build machine must be running to 2.0.22221 from October 12th, 2023 (thanks @citrus-it). If you're on a machine that's too old, you'll see a message like this:

error: could not compile `installinator` (bin "installinator" test) due to previous error
warning: build failed, waiting for other jobs to finish...
error: linking with `gcc` failed: exit status: 1
  |
...
  = note: ld: fatal: library -lipcc: not found
          ld: fatal: file processing errors. No output written to /home/dap/omicron-iter/target/debug/deps/installinator-40f5c7be5862bb49
          collect2: error: ld returned 1 exit status
          

error: could not compile `installinator` (lib test) due to previous error
error: command `/home/dap/.rustup/toolchains/1.74.1-x86_64-unknown-illumos/bin/cargo test --no-run --message-format json-render-diagnostics` exited with code 101

You should generally be able to update Helios machines with pkg update followed by a reboot.

papertigers added 5 commits November 20, 2023 21:19

Use libipcc lib instead of raw ioctl calls

8dbc358

Support non illumos systems & minor cleanup

c45227e

Temporary workaround for ipcc runtime path

724ed22

Add MPL

b32d21b

Fix up libipcc handle stub

cbcee08

citrus-it reviewed Nov 21, 2023

View reviewed changes

ipcc/src/handle.rs Outdated Show resolved Hide resolved

ipcc/src/handle.rs Outdated Show resolved Hide resolved

ipcc/src/lib.rs Outdated Show resolved Hide resolved

papertigers added 3 commits November 21, 2023 15:32

Fix gateway.json openapi

279673c

Address andy's feedback

2e25d56

Add some documentation

a7aa5c6

papertigers changed the title ~~Mike/ipcc~~ The control plane should use libipcc Nov 27, 2023

papertigers marked this pull request as ready for review November 27, 2023 21:20

papertigers commented Nov 27, 2023

View reviewed changes

jordanhendricks self-requested a review November 28, 2023 18:26

jordanhendricks reviewed Nov 28, 2023

View reviewed changes

citrus-it reviewed Nov 29, 2023

View reviewed changes

ipcc/src/lib.rs Outdated Show resolved Hide resolved

papertigers and others added 2 commits December 11, 2023 14:51

Apply Andy's suggestion

e50589d

Co-authored-by: Andy Fiddaman <[email protected]>

Addres Jordan's feedback

77bd065

papertigers requested a review from jordanhendricks December 11, 2023 20:55

papertigers added 5 commits December 11, 2023 23:05

Update documentation found in .cargo/config

be7931d

Merge remote-tracking branch 'origin/main' into mike/ipcc

c7de1cd

grammar

a6a4e14

Enforce the actual errmsg length

ef49d98

Address rain's feedback

78a9777

papertigers requested a review from citrus-it December 12, 2023 21:30

jordanhendricks approved these changes Dec 13, 2023

View reviewed changes

citrus-it approved these changes Dec 14, 2023

View reviewed changes

papertigers added 3 commits December 27, 2023 17:29

Test new buildomat image

9b673a7

Make sure the oxide library runpath is passed through to tests

25f9b6e

Actually specify link args for buildomat test

a99a8c2

Move omicron back to helios-2.0 buildomat targets

861aa76

papertigers enabled auto-merge (squash) January 10, 2024 22:54

papertigers merged commit 0528456 into main Jan 11, 2024
22 of 23 checks passed

papertigers deleted the mike/ipcc branch January 11, 2024 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The control plane should use libipcc #4536

The control plane should use libipcc #4536

papertigers commented Nov 21, 2023 •

edited

Loading

papertigers commented Nov 27, 2023

papertigers Nov 27, 2023

papertigers Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks left a comment

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

citrus-it Dec 12, 2023

jordanhendricks Nov 28, 2023

papertigers Dec 11, 2023

jordanhendricks left a comment

citrus-it left a comment

davepacheco commented Jan 11, 2024

		// Copyright 2023 Oxide Computer Company

		use crate::IpccError;

	// Safety: CStr::from_ptr is documented as safe if:
	// 1. The pointer contains a valid null terminator at the end of
	// the string
	// 2. The pointer is valid for reads of bytes up to and including
	// the null terminator
	// 3. The memory referenced by the return CStr is not mutated for
	// the duration of lifetime 'a
	//
	// (1) is true because we initialize the buffers for ste_path as all
	// 0s, and their length is long enough to include the null
	// terminator for all paths on the system.
	// (2) should be guaranteed by the syscall itself, and we can know
	// how many entries are valid via its return value.
	// (3) We aren't currently mutating the memory referenced by the
	// CStr, though there's nothing here enforcing that.

The control plane should use libipcc #4536

The control plane should use libipcc #4536

Conversation

papertigers commented Nov 21, 2023 • edited Loading

papertigers commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanhendricks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanhendricks left a comment

Choose a reason for hiding this comment

citrus-it left a comment

Choose a reason for hiding this comment

davepacheco commented Jan 11, 2024

papertigers commented Nov 21, 2023 •

edited

Loading