Conversation
|
This seems fine. Please make sure the sprockets code works and omicron comes up with trust quorum being established. |
verifier/src/ipcc.rs
Outdated
| /// Creates a new `Ipcc` instance. | ||
| pub fn new() -> Result<Self, IpccError> { | ||
| let handle = IpccHandle::new()?; | ||
| let handle = tokio::sync::Mutex::new(IpccHandle::new()?); |
There was a problem hiding this comment.
I have a visceral, horrified reaction to seeing tokio::sync::Mutex introduced here given cancellation issues we've had in the past. Is there a reason that this is fine to use here? Is it that block_in_place can't actually be cancelled?
There was a problem hiding this comment.
that's kind of why I spelled out the full name, really. this deserves at least a comment on AttestIpcc for why I think it's okay, which is that the only thing in this mutex is IpccHandle and, almost the same as you say, that block_in_place can't be cancelled. I'd thought of it slightly differently, which is that the only thing in the block_in_place is rot_request() which is synchronous.
I'd overlooked just taking the lock on a normal mutex inside the block_in_place though, and that's much better than any of this, so I'll switch this over and save everyone the heartburn.
There was a problem hiding this comment.
One other option: skip storing the IpccHandle on AttestIpcc altogether and just open a new one in do_rot_request every time? The ipcc kernel driver will already block on outstanding requests.
There was a problem hiding this comment.
oh that'd be nice! I was kind of wondering if there was anything more complex to IpccHandle.
There was a problem hiding this comment.
so https://github.com/oxidecomputer/dice-util/pull/360/changes#diff-417812bd59700ca7e903b9f21039baa4311658a4b5c7413958494e32493ca129R21-R47 swaps the tokio::sync::Mutex for @luqmana's suggestion to just make handles on-demand. this seems less weird on the whole but I still don't really want to land wider system changes until after 19.
verifier/src/ipcc.rs
Outdated
| /// Creates a new `Ipcc` instance. | ||
| pub fn new() -> Result<Self, IpccError> { | ||
| let handle = IpccHandle::new()?; | ||
| let handle = tokio::sync::Mutex::new(IpccHandle::new()?); |
There was a problem hiding this comment.
One other option: skip storing the IpccHandle on AttestIpcc altogether and just open a new one in do_rot_request every time? The ipcc kernel driver will already block on outstanding requests.
(In fact this could already be reworked today since |
| /// IPCC interface / <https://github.com/oxidecomputer/ipcc-rs>. | ||
| /// | ||
| /// The actual handle to the IPCC interface is created and released on-demand. | ||
| pub struct AttestIpcc {} |
There was a problem hiding this comment.
nit: perhaps this ought to have a _p: () or something so that it can only be constructed through new()?
4 participants
this is the first part of excising
rtfromAttestSledAgent,, and after looking at uses ofAttestelsewhere I think we don't really have much reason to keep the sync interfaces around. this pairs with a change invm-attestand both end up inpropolis.I haven't actually tried to pull this into sprockets but this makes
AttestIpccnowSendso this when this gets crank-turned over insprocketsthis whole bit can be reworked (or really, just madeasync. seems fine as-is?)I also haven't actually run
verifier-cli, and I don't intend to merge this without at least seeingpropolis-serveron the other end happy with these changes, but there's no reason to hold up reviewing this change on me checking out a custom build on berlin real quick!