Skip to content

CPUID (1/3): Error type expansion #3104

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

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/api_server/Cargo.toml
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@ serde = { version = ">=1.0.27", features = ["derive"] }
serde_derive = ">=1.0.27"
serde_json = ">=1.0.9"
derive_more = { version = "0.99.17", default-features = false, features = ["from"] }
thiserror = "1.0.32"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to make this >= ?

Copy link
Contributor Author

@JonathanWoollett-Light JonathanWoollett-Light Sep 1, 2022

Choose a reason for hiding this comment

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

With >= we cannot guarantee an author won't make a change that breaks our build. = "1.0.32" is the only way to fully guarantee this, we could loosen it with = "1" instead if we trust they stick to SemVer.

I'm not strongly opinionated here, I'm not sure if we have a policy on this (we should probably make one if not). What do you think is best?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do people tend to break semver? I would assume that if 1 does not become 2 we should be fine.

Copy link
Contributor Author

@JonathanWoollett-Light JonathanWoollett-Light Sep 2, 2022

Choose a reason for hiding this comment

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

Yes, quite often. Even if they intend to abide SemVer (which they often don't) since their is no common automated checking mechanism (rust-vmm broke it recently). I have no strong opinion here, do you think we should go with thiserror="1"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't have a strong opinion. Maybe we could play safe and leave it as it is here.


logger = { path = "../logger" }
micro_http = { git = "https://github.com/firecracker-microvm/micro-http", rev = "0a58eb1" }
12 changes: 3 additions & 9 deletions src/api_server/src/lib.rs
Original file line number Diff line number Diff line change
@@ -34,22 +34,16 @@ pub type ApiRequest = Box<VmmAction>;
pub type ApiResponse = Box<std::result::Result<VmmData, VmmActionError>>;

/// Errors thrown when binding the API server to the socket path.
#[derive(thiserror::Error)]
pub enum Error {
/// IO related error.
#[error("IO error: {0}")]
Io(io::Error),
/// EventFD related error.
#[error("EventFd error: {0}")]
Eventfd(io::Error),
}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self {
Error::Io(ref err) => write!(f, "IO error: {}", err),
Error::Eventfd(ref err) => write!(f, "EventFd error: {}", err),
}
}
}

impl fmt::Debug for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self {
1 change: 1 addition & 0 deletions src/arch/Cargo.toml
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@ versionize = ">=0.1.6"
versionize_derive = ">=0.1.3"
vm-fdt = "0.1.0"
derive_more = { version = "0.99.17", default-features = false, features = ["from"] }
thiserror = "1.0.32"
bitflags = ">=1.0.4"

arch_gen = { path = "../arch_gen" }
8 changes: 7 additions & 1 deletion src/arch/src/aarch64/gic/mod.rs
Original file line number Diff line number Diff line change
@@ -16,15 +16,21 @@ pub use regs::GicState;
use super::layout;

/// Errors thrown while setting up the GIC.
#[derive(Debug)]
#[derive(Debug, thiserror::Error)]
pub enum Error {
/// Error while calling KVM ioctl for setting up the global interrupt controller.
#[error("Error while calling KVM ioctl for setting up the global interrupt controller: {0}")]
CreateGIC(kvm_ioctls::Error),
/// Error while setting or getting device attributes for the GIC.
#[error("Error while setting or getting device attributes for the GIC: {0}, {1}, {2}")]
DeviceAttribute(kvm_ioctls::Error, bool, u32),
/// The number of vCPUs in the GicState doesn't match the number of vCPUs on the system
#[error(
"The number of vCPUs in the GicState doesn't match the number of vCPUs on the system."
)]
InconsistentVcpuCount,
/// The VgicSysRegsState is invalid
#[error("The VgicSysRegsState is invalid.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to add a parameter for what the state is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would need to change the underlying function for this. Where practical I have tried not to change too much functionality.

InvalidVgicSysRegState,
}
type Result<T> = result::Result<T, Error>;
8 changes: 5 additions & 3 deletions src/arch/src/x86_64/interrupts.rs
Original file line number Diff line number Diff line change
@@ -9,11 +9,13 @@ use kvm_bindings::kvm_lapic_state;
use kvm_ioctls::VcpuFd;
use utils::byte_order;
/// Errors thrown while configuring the LAPIC.
#[derive(Debug)]
#[derive(Debug, thiserror::Error, PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the PartialEq here?

Copy link
Contributor

Choose a reason for hiding this comment

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

... and in other Error types

Copy link
Contributor Author

@JonathanWoollett-Light JonathanWoollett-Light Sep 2, 2022

Choose a reason for hiding this comment

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

kvm_ioctls::Error doesn't implement std::cmp::Eq unfortunately, so we cannot derive it on types which contain it (deriving PartialEq is still useful so we can do ErrorX == ErrorY).

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need it in the first place? Does thiserror require it?

pub enum Error {
/// Failure in retrieving the LAPIC configuration.
/// Failure in getting the LAPIC configuration.
#[error("Failure in getting the LAPIC configuration: {0}")]
GetLapic(kvm_ioctls::Error),
/// Failure in modifying the LAPIC configuration.
/// Failure in setting the LAPIC configuration.
#[error("Failure in setting the LAPIC configuration: {0}")]
SetLapic(kvm_ioctls::Error),
}
type Result<T> = std::result::Result<T, Error>;
36 changes: 30 additions & 6 deletions src/arch/src/x86_64/msr.rs
Original file line number Diff line number Diff line change
@@ -282,20 +282,44 @@ pub fn create_boot_msr_entries() -> Vec<kvm_msr_entry> {
]
}

/// Error type for [`set_msrs`].
#[derive(Debug, thiserror::Error)]
pub enum SetMSRsError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I suggest reframing the error naming that this type encapsulates? My recommendation would be to rename the enum to MSRConfigurationError? (or MSRAccessError, then it could contain errors for reading and writing, unless you anticipate a large variety of error types for each category of cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My current pattern at the moment is to prefix with the function name. I tend to prefer this as it best illustrates they are bound (and this error is not used elsewhere).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this mean that there would potentially be a GetMSRsError eventually as well?

The naming here of an error class (enum) associated with each single function seems like overkill?

Copy link
Contributor Author

@JonathanWoollett-Light JonathanWoollett-Light Sep 1, 2022

Choose a reason for hiding this comment

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

Would this mean that there would potentially be a GetMSRsError eventually as well?

Most likely.

The naming here of an error class (enum) associated with each single function seems like overkill?

This is a somewhat opinionated area. In the Rust standard library we see instances of both function specific error types and shared error types. I lean towards specific error types as this provides better documentation and traceability. There is a balance here, if functions return mostly similar errors it may make sense to use a shared error type, otherwise function specific error types may make sense. I find error types act like documentation here.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO If SetMSR and GetMSR have identical/similar errors, they should use the same Error type as you suggest also, so maybe something MSRConfigError?

Copy link
Contributor Author

@JonathanWoollett-Light JonathanWoollett-Light Sep 2, 2022

Choose a reason for hiding this comment

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

Worth noting here, there wouldn't be a GetMSRsError in Firecracker since there is no get_msrs in Firecracker (this function is within rust-vmm).

/// Failed to create [`vmm_sys_util::fam::FamStructWrapper`] for MSRs.
#[error("Could not create `vmm_sys_util::fam::FamStructWrapper` for MSRs")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the context for this error may give a hint as to the reason why this may happen, but I don't think this would be a useful error to see in a log. Some context as to why the error happened would be ideal, otherwise dropping this error in favour of the more generic error would seem more appropriate to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with giving further context here is that since fam::Error doesn't implement std::error::Error we can't display it nicely (I recently got a PR merged fixing this rust-vmm/vmm-sys-util#168 but we would need to update the version for this).

In this context I'd argue that we leave it in the most descriptive state, considering this will form an error message like:

Failed to boot microVM: Failed to initialize vCPU: Failed to set MSRs: Could not create vmm_sys_util::fam::FamStructWrapper for MSRs

When we update our version we can update here to add further context.

It would be useful to remind someone in the future to add this context. Do you know if there is there any recommended approach here?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this context I'd argue that we leave it in the most descriptive state

My concern is that for a Firecracker user, this error isn't descriptive. Parameterising a more specific message from where the error occurs so that the user can troubleshoot would be a better way to go here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is as specific as we can go, all we know from Msrs::from_entries(&msr_entries) is that an error occurred. fam::Error contains this description, but for now, we cannot display it (we need there to be a new version of the crate containing fam::Error so as to expose the std::error::Error implementation, so we can then display it).

While it would be ideal that every error would be descriptive enough to highlight solutions I think for the moment here it cannot be well done and this is still an improvement over the existing even less specific error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the context of the error here might clarify the usefulness of it. A less specific but general error like "Internal error" for example would at least indicate a system error versus a user error. The current description is for a developer's use, not for the user of Firecracker.

Copy link
Contributor Author

@JonathanWoollett-Light JonathanWoollett-Light Aug 31, 2022

Choose a reason for hiding this comment

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

A less specific but general error like "Internal error" for example would at least indicate a system error versus a user error. The current description is for a developer's use, not for the user of Firecracker.

I think this distinction is difficult to draw and somewhat unnecessary. If we have a descriptive error we can then on the front-end present the user with a simple "Internal error". I do not see the argument for having a less descriptive error (in the same sense I would not see an argument for having less descriptive documentation).

Apologies if I am misunderstanding.

Create(utils::fam::Error),
/// Settings MSRs resulted in an error.
#[error("Setting MSRs resulted in an error: {0}")]
Set(#[from] kvm_ioctls::Error),
/// Not all given MSRs were set.
#[error("Not all given MSRs were set.")]
Incomplete,
}

/// Configure Model Specific Registers (MSRs) required to boot Linux for a given x86_64 vCPU.
///
/// # Arguments
///
/// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
pub fn set_msrs(vcpu: &VcpuFd, msr_entries: &[kvm_msr_entry]) -> Result<()> {
let msrs = Msrs::from_entries(&msr_entries).map_err(Error::FamError)?;
///
/// # Errors
///
/// When:
/// - Failed to create [`vmm_sys_util::fam::FamStructWrapper`] for MSRs.
/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_msrs`] errors.
/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_msrs`] fails to write all given MSRs entries.
pub fn set_msrs(
vcpu: &VcpuFd,
msr_entries: &[kvm_msr_entry],
) -> std::result::Result<(), SetMSRsError> {
let msrs = Msrs::from_entries(&msr_entries).map_err(SetMSRsError::Create)?;
vcpu.set_msrs(&msrs)
.map_err(Error::SetModelSpecificRegisters)
.map_err(SetMSRsError::Set)
.and_then(|msrs_written| {
if msrs_written as u32 != msrs.as_fam_struct_ref().nmsrs {
Err(Error::SetModelSpecificRegistersCount)
} else {
if msrs_written as u32 == msrs.as_fam_struct_ref().nmsrs {
Ok(())
} else {
Err(SetMSRsError::Incomplete)
}
})
}
98 changes: 84 additions & 14 deletions src/arch/src/x86_64/regs.rs
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the THIRD-PARTY file.

use std::mem;
use std::{fmt, mem};

use kvm_bindings::{kvm_fpu, kvm_regs, kvm_sregs};
use kvm_ioctls::VcpuFd;
@@ -19,42 +19,76 @@ const PDPTE_START: u64 = 0xa000;
const PDE_START: u64 = 0xb000;

/// Errors thrown while setting up x86_64 registers.
#[derive(Debug)]
#[derive(Debug, thiserror::Error, PartialEq)]
pub enum Error {
/// Failed to get SREGs for this CPU.
#[error("Failed to get SREGs for this CPU: {0}")]
GetStatusRegisters(kvm_ioctls::Error),
/// Failed to set base registers for this CPU.
#[error("Failed to set base registers for this CPU: {0}")]
SetBaseRegisters(kvm_ioctls::Error),
/// Failed to configure the FPU.
#[error("Failed to configure the FPU: {0}")]
SetFPURegisters(kvm_ioctls::Error),
/// Failed to set SREGs for this CPU.
#[error("Failed to set SREGs for this CPU: {0}")]
SetStatusRegisters(kvm_ioctls::Error),
/// Writing the GDT to RAM failed.
#[error("Writing the GDT to RAM failed.")]
WriteGDT,
/// Writing the IDT to RAM failed.
#[error("Writing the IDT to RAM failed")]
WriteIDT,
/// Writing PDPTE to RAM failed.
#[error("WritePDPTEAddress")]
WritePDPTEAddress,
/// Writing PDE to RAM failed.
#[error("WritePDEAddress")]
WritePDEAddress,
/// Writing PML4 to RAM failed.
#[error("WritePML4Address")]
WritePML4Address,
}
type Result<T> = std::result::Result<T, Error>;

/// Error type for [`setup_fpu`].
#[derive(Debug, derive_more::From, PartialEq)]
pub struct SetupFpuError(utils::errno::Error);
impl std::error::Error for SetupFpuError {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not thiserror here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thiserror doesn't seem to be able to derive on new types (new types being struct X(Y)).

Copy link
Contributor

Choose a reason for hiding this comment

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

link?

impl fmt::Display for SetupFpuError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "Failed to setup FPU: {}", self.0)
}
}

/// Configure Floating-Point Unit (FPU) registers for a given CPU.
///
/// # Arguments
///
/// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
pub fn setup_fpu(vcpu: &VcpuFd) -> Result<()> {
///
/// # Errors
///
/// When [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_fpu`] errors.
pub fn setup_fpu(vcpu: &VcpuFd) -> std::result::Result<(), SetupFpuError> {
let fpu: kvm_fpu = kvm_fpu {
fcw: 0x37f,
mxcsr: 0x1f80,
..Default::default()
};

vcpu.set_fpu(&fpu).map_err(Error::SetFPURegisters)
let res = vcpu.set_fpu(&fpu)?;
Ok(res)
}

/// Error type of [`setup_regs`].
#[derive(Debug, derive_more::From, PartialEq)]
pub struct SetupRegistersError(utils::errno::Error);
impl std::error::Error for SetupRegistersError {}
impl fmt::Display for SetupRegistersError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "Failed to setup registers:{}", self.0)
}
}

/// Configure base registers for a given CPU.
@@ -63,7 +97,11 @@ pub fn setup_fpu(vcpu: &VcpuFd) -> Result<()> {
///
/// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
/// * `boot_ip` - Starting instruction pointer.
pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> Result<()> {
///
/// # Errors
///
/// When [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_regs`] errors.
pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> std::result::Result<(), SetupRegistersError> {
let regs: kvm_regs = kvm_regs {
rflags: 0x0000_0000_0000_0002u64,
rip: boot_ip,
@@ -79,22 +117,54 @@ pub fn setup_regs(vcpu: &VcpuFd, boot_ip: u64) -> Result<()> {
..Default::default()
};

vcpu.set_regs(&regs).map_err(Error::SetBaseRegisters)
vcpu.set_regs(&regs).map_err(SetupRegistersError)
}

/// Error type for [`setup_sregs`].
#[derive(Debug, thiserror::Error, PartialEq)]
pub enum SetupSpecialRegistersError {
/// Failed to get special registers
#[error("Failed to get special registers: {0}")]
GetSpecialRegisters(utils::errno::Error),
/// Failed to configure segments and special registers
#[error("Failed to configure segments and special registers: {0}")]
ConfigureSegmentsAndSpecialRegisters(Error),
/// Failed to setup page tables
#[error("Failed to setup page tables: {0}")]
SetupPageTables(Error),
/// Failed to set special registers
#[error("Failed to set special registers: {0}")]
SetSpecialRegisters(utils::errno::Error),
}

/// Configures the segment registers and system page tables for a given CPU.
/// Configures the special registers and system page tables for a given CPU.
///
/// # Arguments
///
/// * `mem` - The memory that will be passed to the guest.
/// * `vcpu` - Structure for the VCPU that holds the VCPU's fd.
pub fn setup_sregs(mem: &GuestMemoryMmap, vcpu: &VcpuFd) -> Result<()> {
let mut sregs: kvm_sregs = vcpu.get_sregs().map_err(Error::GetStatusRegisters)?;

configure_segments_and_sregs(mem, &mut sregs)?;
setup_page_tables(mem, &mut sregs)?; // TODO(dgreid) - Can this be done once per system instead?

vcpu.set_sregs(&sregs).map_err(Error::SetStatusRegisters)
///
/// # Errors
///
/// When:
/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::get_sregs`] errors.
/// - [`configure_segments_and_sregs`] errors.
/// - [`setup_page_tables`] errors
/// - [`kvm_ioctls::ioctls::vcpu::VcpuFd::set_sregs`] errors.
pub fn setup_sregs(
mem: &GuestMemoryMmap,
vcpu: &VcpuFd,
) -> std::result::Result<(), SetupSpecialRegistersError> {
let mut sregs: kvm_sregs = vcpu
.get_sregs()
.map_err(SetupSpecialRegistersError::GetSpecialRegisters)?;

configure_segments_and_sregs(mem, &mut sregs)
.map_err(SetupSpecialRegistersError::ConfigureSegmentsAndSpecialRegisters)?;
setup_page_tables(mem, &mut sregs).map_err(SetupSpecialRegistersError::SetupPageTables)?; // TODO(dgreid) - Can this be done once per system instead?

vcpu.set_sregs(&sregs)
.map_err(SetupSpecialRegistersError::SetSpecialRegisters)
}

const BOOT_GDT_OFFSET: u64 = 0x500;
1 change: 1 addition & 0 deletions src/cpuid/Cargo.toml
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@ license = "Apache-2.0"
kvm-bindings = { version = ">=0.5.0", features = ["fam-wrappers"] }
kvm-ioctls = ">=0.9.0"
derive_more = { version = "0.99.17", default-features = false, features = ["from"] }
thiserror = "1.0.32"

utils = { path = "../utils"}
arch = { path = "../arch" }
4 changes: 3 additions & 1 deletion src/cpuid/src/common.rs
Original file line number Diff line number Diff line change
@@ -18,11 +18,13 @@ pub const VENDOR_ID_INTEL: &[u8; 12] = b"GenuineIntel";
pub const VENDOR_ID_AMD: &[u8; 12] = b"AuthenticAMD";

/// cpuid related error.
#[derive(Clone, Debug)]
#[derive(Clone, Debug, thiserror::Error, PartialEq, Eq)]
pub enum Error {
/// The function was called with invalid parameters.
#[error("The function was called with invalid parameters.")]
InvalidParameters(String),
/// Function not supported on the current architecture.
#[error("Function not supported on the current architecture.")]
NotSupported,
}

Loading