From 8c87b852c4952e1729accb38d8678da4e02556ea Mon Sep 17 00:00:00 2001 From: Vasili Novikov Date: Fri, 22 Sep 2023 18:13:16 +0200 Subject: [PATCH] Code review (async-usercalls) Address code review comments --- .travis.yml | 1 + intel-sgx/async-usercalls/src/callback.rs | 2 +- intel-sgx/async-usercalls/src/lib.rs | 2 +- intel-sgx/enclave-runner/src/usercalls/abi.rs | 2 +- intel-sgx/enclave-runner/src/usercalls/mod.rs | 5 ++--- ipc-queue/src/position.rs | 16 ++++++++++------ 6 files changed, 16 insertions(+), 12 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6a7ea6c8..2049185a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -26,6 +26,7 @@ matrix: - clang-11 - musl-tools rust: + - nightly - nightly-2023-05-07 env: - RUST_BACKTRACE=1 diff --git a/intel-sgx/async-usercalls/src/callback.rs b/intel-sgx/async-usercalls/src/callback.rs index 46e2fded..ea5d969b 100644 --- a/intel-sgx/async-usercalls/src/callback.rs +++ b/intel-sgx/async-usercalls/src/callback.rs @@ -62,4 +62,4 @@ macro_rules! define_callback { }; } -invoke_with_usercalls!(define_callback); \ No newline at end of file +invoke_with_usercalls!(define_callback); diff --git a/intel-sgx/async-usercalls/src/lib.rs b/intel-sgx/async-usercalls/src/lib.rs index d81648a7..1025eed8 100644 --- a/intel-sgx/async-usercalls/src/lib.rs +++ b/intel-sgx/async-usercalls/src/lib.rs @@ -436,4 +436,4 @@ mod tests { } } } -} \ No newline at end of file +} diff --git a/intel-sgx/enclave-runner/src/usercalls/abi.rs b/intel-sgx/enclave-runner/src/usercalls/abi.rs index 0fcc0c88..e36a292e 100644 --- a/intel-sgx/enclave-runner/src/usercalls/abi.rs +++ b/intel-sgx/enclave-runner/src/usercalls/abi.rs @@ -19,7 +19,7 @@ use futures::future::Future; type Register = u64; -pub(super) trait RegisterArgument { +trait RegisterArgument { fn from_register(_: Register) -> Self; fn into_register(self) -> Register; } diff --git a/intel-sgx/enclave-runner/src/usercalls/mod.rs b/intel-sgx/enclave-runner/src/usercalls/mod.rs index 438567ff..b855bfa9 100644 --- a/intel-sgx/enclave-runner/src/usercalls/mod.rs +++ b/intel-sgx/enclave-runner/src/usercalls/mod.rs @@ -38,7 +38,9 @@ use sgxs::loader::Tcs as SgxsTcs; use crate::loader::{EnclavePanic, ErasedTcs}; use crate::tcs::{self, CoResult, ThreadResult}; use self::abi::dispatch; +use self::abi::ReturnValue; use self::abi::UsercallList; +use self::interface::ToSgxResult; use self::interface::{Handler, OutputBuffer}; pub(crate) mod abi; @@ -793,9 +795,6 @@ impl EnclaveState { let mut input = IOHandlerInput { enclave: enclave.clone(), tcs, work_sender: &work_sender }; let handler = Handler(&mut input); let result = { - use self::interface::ToSgxResult; - use self::abi::ReturnValue; - let (p1, p2, p3, p4, p5) = parameters; match notifier_rx { None => dispatch(handler, p1, p2, p3, p4, p5).await.1, diff --git a/ipc-queue/src/position.rs b/ipc-queue/src/position.rs index e70461e3..a1ba59f8 100644 --- a/ipc-queue/src/position.rs +++ b/ipc-queue/src/position.rs @@ -27,21 +27,26 @@ impl PositionMonitor { pub fn read_position(&self) -> ReadPosition { let current = self.fifo.current_offsets(Ordering::Relaxed); let read_epoch = self.read_epoch.load(Ordering::Relaxed); - let read_epoch_shifted = read_epoch + let read_epoch_shifted = (read_epoch as u64) .checked_shl(32) - .expect("Reading from position of over 2^32 (2 to the power of 32). This is unsupported."); + .expect("Read epoch is >= 2^32 (2 to the power of 32). This is unsupported."); ReadPosition(read_epoch_shifted | (current.read_offset() as u64)) } pub fn write_position(&self) -> WritePosition { let current = self.fifo.current_offsets(Ordering::Relaxed); let mut write_epoch = self.read_epoch.load(Ordering::Relaxed); + // Write epoch keeps track of how many times the write offset wrapped around + // the ring buffer. Write epochs are not tracked separately, only read epoch are. + // We know, however, that objects are always written to the buffer first. + // So, the high bit used in the write and read offsets tell us whether writes + // already wrapped around the ring buffer, while reads have not yet. if current.read_high_bit() != current.write_high_bit() { write_epoch += 1; } let write_epoch_shifted = write_epoch .checked_shl(32) - .expect("Writing to position of over 2^32 (2 to the power of 32). This is unsupported."); + .expect("Write epoch is >= 2^32 (2 to the power of 32). This is unsupported."); WritePosition(write_epoch_shifted | (current.write_offset() as u64)) } } @@ -57,8 +62,7 @@ impl Clone for PositionMonitor { impl ReadPosition { /// A `WritePosition` can be compared to a `ReadPosition` **correctly** if - /// at most 2³¹ (2 to the power of 31) writes - /// have occurred since the write position was recorded. + /// the ring buffer wrapped around at most 2³¹ (2 to the power of 31) times. pub fn is_past(&self, write: &WritePosition) -> bool { let (read, write) = (self.0, write.0); let hr = read & (1 << 63); @@ -68,4 +72,4 @@ impl ReadPosition { } true } -} \ No newline at end of file +}