Skip to content

Commit

Permalink
hvdef: make HvStatus type distinct from HvError (#754)
Browse files Browse the repository at this point in the history 
`HvError` is supposed to represent a non-success status code, but it's
also used for a possibly-successful status code in some places. Fix this
by adding a new `HvStatus` type and making `HvError` a wrapper around
`NonZeroU16`.

This also gives us a nice niche optimization, so that `HvResult<(),
HvError>` is equivalent to `HvStatus`.
  • Loading branch information
@jstarks
jstarks authored Jan 31, 2025
1 parent 88225af commit 22362c3
Show file tree
Hide file tree
Showing 11 changed files with 308 additions and 203 deletions.
10 changes: 5 additions & 5 deletions openhcl/hcl/src/ioctl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ use hvdef::HvMessage;
use hvdef::HvRegisterName;
use hvdef::HvRegisterValue;
use hvdef::HvRegisterVsmPartitionConfig;
use hvdef::HvStatus;
use hvdef::HvX64RegisterName;
use hvdef::HvX64RegisterPage;
use hvdef::HypercallCode;
Expand Down Expand Up @@ -171,10 +172,9 @@ pub enum HypercallError {
impl HypercallError {
pub(crate) fn check(r: Result<i32, nix::Error>) -> Result<(), Self> {
match r {
Ok(0) => Ok(()),
Ok(n) => Err(Self::Hypervisor(HvError(
n.try_into().expect("hypervisor result out of range"),
))),
Ok(n) => HvStatus(n.try_into().expect("hypervisor result out of range"))
.result()
.map_err(Self::Hypervisor),
Err(err) => Err(Self::Ioctl(IoctlError(err))),
}
}
Expand Down Expand Up @@ -1040,7 +1040,7 @@ impl MshvHvcall {
.map_err(HvcallError::HypercallIoctlFailed)?;
}

if call_object.status.call_status() == HvError::Timeout.0 {
if call_object.status.call_status() == Err(HvError::Timeout).into() {
// Any hypercall can timeout, even one that doesn't have reps. Continue processing
// from wherever the hypervisor left off. The rep start index isn't checked for
// validity, since it is only being used as an input to the untrusted hypervisor.
Expand Down
31 changes: 15 additions & 16 deletions openhcl/sidecar/src/arch/x86_64/vp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use core::sync::atomic::Ordering::Release;
use hvdef::hypercall::HvInputVtl;
use hvdef::hypercall::HvRegisterAssoc;
use hvdef::hypercall::TranslateVirtualAddressX64;
use hvdef::HvError;
use hvdef::HvStatus;
use hvdef::HvVtlEntryReason;
use hvdef::HvX64RegisterName;
use hvdef::HypercallCode;
Expand Down Expand Up @@ -400,7 +400,7 @@ fn get_vp_registers(command_page: &mut CommandPage) {
count,
target_vtl,
rsvd: _,
ref mut result,
ref mut status,
rsvd2: _,
regs: [],
} = FromBytes::mut_from(request).unwrap();
Expand All @@ -413,7 +413,7 @@ fn get_vp_registers(command_page: &mut CommandPage) {
return;
};

*result = HvError(0);
*status = HvStatus::SUCCESS;
for &mut HvRegisterAssoc {
name,
pad: _,
Expand All @@ -434,7 +434,7 @@ fn get_vp_registers(command_page: &mut CommandPage) {
match r {
Ok(v) => *value = v,
Err(err) => {
*result = err;
*status = Err(err).into();
break;
}
};
Expand All @@ -450,7 +450,7 @@ fn set_vp_registers(command_page: &mut CommandPage) {
count,
target_vtl,
rsvd: _,
ref mut result,
ref mut status,
rsvd2: _,
regs: [],
} = FromBytes::mut_from(request).unwrap();
Expand All @@ -463,7 +463,7 @@ fn set_vp_registers(command_page: &mut CommandPage) {
return;
};

*result = HvError(0);
*status = HvStatus::SUCCESS;
for &HvRegisterAssoc {
name,
value,
Expand All @@ -482,8 +482,8 @@ fn set_vp_registers(command_page: &mut CommandPage) {
set_hv_vp_register(target_vtl, name, value)
};

if let Err(err) = r {
*result = err;
if r.is_err() {
*status = r.into();
break;
}
}
Expand All @@ -509,17 +509,16 @@ fn translate_gva(command_page: &mut CommandPage) {
}

let result = hypercall(HypercallCode::HvCallTranslateVirtualAddressEx, 0);
let (result, output) = match result {
Ok(()) => {
// SAFETY: the output is not concurrently accessed
let output = unsafe { &*addr_space::hypercall_output() };
(HvError(0), FromBytes::read_from_prefix(output).unwrap())
}
Err(err) => (err, FromZeroes::new_zeroed()),
let output = if result.is_ok() {
// SAFETY: the output is not concurrently accessed
let output = unsafe { &*addr_space::hypercall_output() };
FromBytes::read_from_prefix(output).unwrap()
} else {
FromZeroes::new_zeroed()
};

TranslateGvaResponse {
result,
status: result.into(),
rsvd: [0; 7],
output,
}
Expand Down
23 changes: 9 additions & 14 deletions openhcl/sidecar_client/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use hvdef::hypercall::HvRegisterAssoc;
use hvdef::hypercall::TranslateVirtualAddressExOutputX64;
use hvdef::HvError;
use hvdef::HvMessage;
use hvdef::HvStatus;
use pal_async::driver::PollImpl;
use pal_async::driver::SpawnDriver;
use pal_async::fd::PollFdReady;
Expand Down Expand Up @@ -434,19 +435,17 @@ impl<'a> SidecarVp<'a> {
count: regs.len() as u16,
target_vtl,
rsvd: 0,
result: HvError(0),
status: HvStatus::SUCCESS,
rsvd2: [0; 10],
regs: [],
},
regs.len(),
);
buf.copy_from_slice(regs);
self.run_sync()?;
let (&GetSetVpRegisterRequest { result, .. }, buf) =
let (&GetSetVpRegisterRequest { status, .. }, buf) =
self.command_result::<_, HvRegisterAssoc>(regs.len())?;
if result != HvError(0) {
return Err(SidecarError::Hypervisor(result));
}
status.result().map_err(SidecarError::Hypervisor)?;
regs.copy_from_slice(buf);
}
Ok(())
Expand All @@ -466,18 +465,16 @@ impl<'a> SidecarVp<'a> {
count: regs.len() as u16,
target_vtl,
rsvd: 0,
result: HvError(0),
status: HvStatus::SUCCESS,
rsvd2: [0; 10],
regs: [],
},
regs.len(),
);
buf.copy_from_slice(regs);
self.run_sync()?;
let &GetSetVpRegisterRequest { result, .. } = self.command_result::<_, u8>(0)?.0;
if result != HvError(0) {
return Err(SidecarError::Hypervisor(result));
}
let &GetSetVpRegisterRequest { status, .. } = self.command_result::<_, u8>(0)?.0;
status.result().map_err(SidecarError::Hypervisor)?;
}
Ok(())
}
Expand All @@ -491,16 +488,14 @@ impl<'a> SidecarVp<'a> {
) -> Result<TranslateVirtualAddressExOutputX64, SidecarError> {
tracing::trace!("translate gva");
let &TranslateGvaResponse {
result,
status,
rsvd: _,
output,
} = self.dispatch_sync(
SidecarCommand::TRANSLATE_GVA,
TranslateGvaRequest { gvn, control_flags },
)?;
if result != HvError(0) {
return Err(SidecarError::Hypervisor(result));
}
status.result().map_err(SidecarError::Hypervisor)?;
Ok(output)
}

Expand Down
6 changes: 3 additions & 3 deletions openhcl/sidecar_defs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
use core::sync::atomic::AtomicU32;
use core::sync::atomic::AtomicU8;
use hvdef::hypercall::HvInputVtl;
use hvdef::HvError;
use hvdef::HvMessage;
use hvdef::HvStatus;
use open_enum::open_enum;
use zerocopy::AsBytes;
use zerocopy::FromBytes;
Expand Down Expand Up @@ -214,7 +214,7 @@ pub struct GetSetVpRegisterRequest {
/// Reserved.
pub rsvd: u8,
/// The hypervisor result.
pub result: HvError,
pub status: HvStatus,
/// Reserved.
pub rsvd2: [u8; 10],
/// Alignment field.
Expand Down Expand Up @@ -243,7 +243,7 @@ pub struct TranslateGvaRequest {
#[derive(Debug, Copy, Clone, AsBytes, FromBytes, FromZeroes)]
pub struct TranslateGvaResponse {
/// The hypervisor result.
pub result: HvError,
pub status: HvStatus,
/// Reserved.
pub rsvd: [u16; 7],
/// The output of the translation.
Expand Down
8 changes: 4 additions & 4 deletions vm/hv1/hv1_hypercall/src/support.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<'a, T: HypercallIo> InnerDispatcher<'a, T> {
/// Complete hypercall handling.
fn complete(&mut self, output: Option<HypercallOutput>) {
if let Some(output) = output {
if output.call_status() == HvError::Timeout.0 {
if output.call_status() == Err(HvError::Timeout).into() {
self.handler.retry(
self.control
.with_rep_start(output.elements_processed())
Expand Down Expand Up @@ -282,7 +282,7 @@ impl<'a, T: HypercallIo> InnerDispatcher<'a, T> {
// which is handled as a failure), nothing is written back.
let output_end = if out_elem_size > 0 {
out_elem_size * ret.elements_processed()
} else if ret.call_status() == 0 {
} else if ret.call_status().is_ok() {
output_len
} else {
0
Expand Down Expand Up @@ -338,7 +338,7 @@ impl<'a, T: HypercallIo> InnerDispatcher<'a, T> {
// which is handled as a failure), nothing is written back.
let output_end = if out_elem_size > 0 {
out_elem_size * ret.elements_processed()
} else if ret.call_status() == 0 {
} else if ret.call_status().is_ok() {
output_len
} else {
0
Expand All @@ -354,7 +354,7 @@ impl<'a, T: HypercallIo> InnerDispatcher<'a, T> {
ret
};

if ret.call_status() == 0 {
if ret.call_status().is_ok() {
debug_assert_eq!(ret.elements_processed(), control.rep_count());
}

Expand Down
14 changes: 7 additions & 7 deletions vm/hv1/hv1_hypercall/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,13 +302,13 @@ impl From<TestResult> for HypercallOutput {
match result {
TestResult::Simple(SimpleResult::Success) => HypercallOutput::new(),
TestResult::Simple(SimpleResult::Failure(err)) => {
HypercallOutput::new().with_call_status(err.0)
HypercallOutput::new().with_call_status(Err(err).into())
}
TestResult::Rep(RepResult::Success(rep_count)) => {
HypercallOutput::new().with_elements_processed(rep_count)
}
TestResult::Rep(RepResult::Failure(err, rep_count)) => HypercallOutput::new()
.with_call_status(err.0)
.with_call_status(Err(err).into())
.with_elements_processed(rep_count),
_ => panic!("Should not be invoked for VTL"),
}
Expand Down Expand Up @@ -1371,7 +1371,7 @@ where
let mut io = io_gen(&mut handler);
let result = HypercallOutput::from(io.get_result());
let control = Control::from(io.control());
let call_status = HvError(result.call_status());
let call_status = result.call_status();

// Copy the output back. Note that in the case of errors the hypercall parser may not have
// actually modified the output buffers/registers, and it is the responsibility of the test
Expand All @@ -1382,8 +1382,8 @@ where
// that in cases where this routine does not modify the output, the hypercall parser does
// not do so either.
if is_timeout
|| (call_status != HvError::InvalidHypercallInput
&& call_status != HvError::InvalidAlignment)
|| (call_status != Err(HvError::InvalidHypercallInput).into()
&& call_status != Err(HvError::InvalidAlignment).into())
{
let mut output_buffer;
let (hdr, reps) = if !params.fast {
Expand Down Expand Up @@ -1559,7 +1559,7 @@ fn hypercall_simple(test_params: TestParams) {

check_test_result(&test_params, result, control);

let expected_output_size = if result.call_status() == 0 {
let expected_output_size = if result.call_status().is_ok() {
assert_eq!(
output.as_bytes(),
TestController::generate_test_output::<TestOutput>().as_bytes()
Expand Down Expand Up @@ -1779,7 +1779,7 @@ fn hypercall_variable(test_params: TestParams) {

check_test_result(&test_params, result, control);

let expected_output_size = if result.call_status() == 0 {
let expected_output_size = if result.call_status().is_ok() {
assert_eq!(
output.as_bytes(),
TestController::generate_test_output::<TestOutput>().as_bytes()
Expand Down
Loading

0 comments on commit 22362c3

Please sign in to comment.