Skip to content

Commit

Permalink
refactor: Eliminate unused {Static,Custom}CpuTemplate variants
Browse files Browse the repository at this point in the history
They were needed for Versionize reasons, but with firecracker-microvm#4230 merged, we can
eliminate them, simplifying the code.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
  • Loading branch information
roypat committed Jan 22, 2024
1 parent 6791d55 commit 3f3ada7
Show file tree
Hide file tree
Showing 10 changed files with 46 additions and 82 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ mod tests {
vcpu_count: Some(8),
mem_size_mib: Some(1024),
smt: Some(false),
cpu_template: Some(StaticCpuTemplate::None),
cpu_template: Some(None),
track_dirty_pages: Some(false),
};
assert_eq!(
Expand Down Expand Up @@ -167,7 +167,7 @@ mod tests {
vcpu_count: Some(8),
mem_size_mib: Some(1024),
smt: Some(false),
cpu_template: Some(StaticCpuTemplate::T2),
cpu_template: Some(Some(StaticCpuTemplate::T2)),
track_dirty_pages: Some(true),
};
assert_eq!(
Expand Down
1 change: 0 additions & 1 deletion src/vmm/src/cpu_config/aarch64/custom_cpu_template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ impl GetCpuTemplate for Option<CpuTemplateType> {
CpuTemplateType::Static(template) => match template {
// TODO: Check if the CPU model is Neoverse-V1.
StaticCpuTemplate::V1N1 => Ok(Cow::Owned(v1n1::v1n1())),
other => Err(GetCpuTemplateError::InvalidStaticCpuTemplate(*other)),
},
},
None => Ok(Cow::Owned(CustomCpuTemplate::default())),
Expand Down
25 changes: 9 additions & 16 deletions src/vmm/src/cpu_config/aarch64/static_cpu_templates/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,31 @@

use serde::{Deserialize, Serialize};

use crate::cpu_config::templates::CpuTemplateType;

/// Module with V1N1 CPU template for aarch64
pub mod v1n1;

/// Templates available for configuring the supported ARM CPU types.
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
pub enum StaticCpuTemplate {
// Needed for compatibility
/// Empty0
Empty0,
// Needed for compatibility
/// Empty1
Empty1,
/// Template to mask Neoverse-V1 as Neoverse-N1
V1N1,
/// No CPU template is used.
#[default]
None,
}

impl StaticCpuTemplate {
/// Check if no template specified
pub fn is_none(&self) -> bool {
self == &StaticCpuTemplate::None
impl Into<Option<StaticCpuTemplate>> for &CpuTemplateType {
fn into(self) -> Option<StaticCpuTemplate> {
match self {
CpuTemplateType::Custom(_) => None,
CpuTemplateType::Static(template) => Some(*template),
}
}
}

impl std::fmt::Display for StaticCpuTemplate {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
StaticCpuTemplate::V1N1 => write!(f, "V1N1"),
StaticCpuTemplate::None => write!(f, "None"),
_ => write!(f, "None"),
}
}
}
Expand Down
22 changes: 0 additions & 22 deletions src/vmm/src/cpu_config/templates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,28 +61,6 @@ pub enum CpuTemplateType {
Static(StaticCpuTemplate),
}

// This conversion is only used for snapshot, but the static CPU template
// information has not been saved into snapshot since v1.1.
impl From<&Option<CpuTemplateType>> for StaticCpuTemplate {
fn from(value: &Option<CpuTemplateType>) -> Self {
match value {
Some(CpuTemplateType::Static(template)) => *template,
Some(CpuTemplateType::Custom(_)) | None => StaticCpuTemplate::None,
}
}
}

// This conversion is used when converting `&VmConfig` to `MachineConfig` to
// respond `GET /machine-config` and `GET /vm`.
impl From<&CpuTemplateType> for StaticCpuTemplate {
fn from(value: &CpuTemplateType) -> Self {
match value {
CpuTemplateType::Static(template) => *template,
CpuTemplateType::Custom(_) => StaticCpuTemplate::None,
}
}
}

impl<'a> TryFrom<&'a [u8]> for CustomCpuTemplate {
type Error = serde_json::Error;

Expand Down
18 changes: 0 additions & 18 deletions src/vmm/src/cpu_config/x86_64/custom_cpu_template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@ impl GetCpuTemplate for Option<CpuTemplateType> {
}
Ok(Cow::Owned(t2a::t2a()))
}
StaticCpuTemplate::None => {
Err(InvalidStaticCpuTemplate(StaticCpuTemplate::None))
}
}
}
},
Expand Down Expand Up @@ -342,21 +339,6 @@ mod tests {
}
}

#[test]
fn test_get_cpu_template_with_none_static_template() {
// Test `get_cpu_template()` when no static CPU template is provided.
// `InvalidStaticCpuTemplate` error should be returned because it is no longer valid and
// was replaced with `None` of `Option<CpuTemplateType>`.
let cpu_template = Some(CpuTemplateType::Static(StaticCpuTemplate::None));
assert_eq!(
cpu_template.get_cpu_template().unwrap_err(),
GetCpuTemplateError::InvalidStaticCpuTemplate(StaticCpuTemplate::None)
);

// Test the Display for StaticCpuTemplate
assert_eq!(format!("{}", StaticCpuTemplate::None), "None");
}

#[test]
fn test_get_cpu_template_with_custom_template() {
// Test `get_cpu_template()` when a custom CPU template is provided. The borrowed
Expand Down
18 changes: 9 additions & 9 deletions src/vmm/src/cpu_config/x86_64/static_cpu_templates/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
use derive_more::Display;
use serde::{Deserialize, Serialize};

use crate::cpu_config::templates::CpuTemplateType;

/// Module with C3 CPU template for x86_64
pub mod c3;
/// Module with T2 CPU template for x86_64
Expand All @@ -17,7 +19,7 @@ pub mod t2s;

/// Template types available for configuring the x86 CPU features that map
/// to EC2 instances.
#[derive(Debug, Default, Display, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Debug, Display, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
pub enum StaticCpuTemplate {
/// C3 Template.
#[display(fmt = "C3")]
Expand All @@ -28,10 +30,6 @@ pub enum StaticCpuTemplate {
/// T2S Template.
#[display(fmt = "T2S")]
T2S,
/// No CPU template is used.
#[default]
#[display(fmt = "None")]
None,
/// T2CL Template.
#[display(fmt = "T2CL")]
T2CL,
Expand All @@ -40,10 +38,12 @@ pub enum StaticCpuTemplate {
T2A,
}

impl StaticCpuTemplate {
/// Check if no template specified
pub fn is_none(&self) -> bool {
self == &StaticCpuTemplate::None
impl From<&CpuTemplateType> for Option<StaticCpuTemplate> {
fn from(val: &CpuTemplateType) -> Self {
match val {
CpuTemplateType::Custom(_) => None,
CpuTemplateType::Static(template) => Some(*template),
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/vmm/src/persist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ pub struct VmInfo {
/// smt information
pub smt: bool,
/// CPU template type
pub cpu_template: StaticCpuTemplate,
pub cpu_template: Option<StaticCpuTemplate>,
/// Boot source information.
pub boot_source: BootSourceConfig,
}
Expand All @@ -61,7 +61,7 @@ impl From<&VmResources> for VmInfo {
Self {
mem_size_mib: value.vm_config.mem_size_mib as u64,
smt: value.vm_config.smt,
cpu_template: StaticCpuTemplate::from(&value.vm_config.cpu_template),
cpu_template: value.vm_config.cpu_template.as_ref().and_then(Into::into),
boot_source: value.boot_source_config().clone(),
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/vmm/src/resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1332,9 +1332,9 @@ mod tests {
mem_size_mib: Some(512),
smt: Some(true),
#[cfg(target_arch = "x86_64")]
cpu_template: Some(StaticCpuTemplate::T2),
cpu_template: Some(Some(StaticCpuTemplate::T2)),
#[cfg(target_arch = "aarch64")]
cpu_template: Some(StaticCpuTemplate::V1N1),
cpu_template: Some(Some(StaticCpuTemplate::V1N1)),
track_dirty_pages: Some(false),
};

Expand Down
4 changes: 2 additions & 2 deletions src/vmm/src/rpc_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,7 @@ mod tests {

use super::*;
use crate::cpu_config::templates::test_utils::build_test_template;
use crate::cpu_config::templates::{CpuTemplateType, StaticCpuTemplate};
use crate::cpu_config::templates::CpuTemplateType;
use crate::devices::virtio::balloon::{BalloonConfig, BalloonError};
use crate::devices::virtio::block_common::CacheType;
use crate::devices::virtio::rng::EntropyError;
Expand Down Expand Up @@ -1071,7 +1071,7 @@ mod tests {
Self {
mem_size_mib: value.vm_config.mem_size_mib as u64,
smt: value.vm_config.smt,
cpu_template: StaticCpuTemplate::from(&value.vm_config.cpu_template),
cpu_template: value.vm_config.cpu_template.as_ref().and_then(Into::into),
boot_source: value.boot_source_config().clone(),
}
}
Expand Down
28 changes: 20 additions & 8 deletions src/vmm/src/vmm_config/machine_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,25 @@ pub struct MachineConfigUpdate {
)]
pub smt: Option<bool>,
/// A CPU template that it is used to filter the CPU features exposed to the guest.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub cpu_template: Option<StaticCpuTemplate>,
#[serde(
default,
skip_serializing_if = "Option::is_none",
deserialize_with = "some_option"
)]
pub cpu_template: Option<Option<StaticCpuTemplate>>,
/// Enables or disables dirty page tracking. Enabling allows incremental snapshots.
#[serde(skip_serializing_if = "Option::is_none")]
pub track_dirty_pages: Option<bool>,
}

fn some_option<'de, T, D>(deserializer: D) -> Result<Option<Option<T>>, D::Error>
where
T: Deserialize<'de>,
D: serde::Deserializer<'de>,
{
Option::<T>::deserialize(deserializer).map(Some)
}

impl MachineConfigUpdate {
/// Checks if the update request contains any data.
/// Returns `true` if all fields are set to `None` which means that there is nothing
Expand All @@ -121,7 +133,7 @@ impl From<MachineConfig> for MachineConfigUpdate {
vcpu_count: Some(cfg.vcpu_count),
mem_size_mib: Some(cfg.mem_size_mib),
smt: Some(cfg.smt),
cpu_template: cfg.cpu_template,
cpu_template: Some(cfg.cpu_template),
track_dirty_pages: Some(cfg.track_dirty_pages),
}
}
Expand Down Expand Up @@ -179,10 +191,7 @@ impl VmConfig {
self.mem_size_mib = mem_size_mib;

if let Some(cpu_template) = update.cpu_template {
self.cpu_template = match cpu_template {
StaticCpuTemplate::None => None,
other => Some(CpuTemplateType::Static(other)),
};
self.cpu_template = cpu_template.map(CpuTemplateType::Static);
}

if let Some(track_dirty_pages) = update.track_dirty_pages {
Expand Down Expand Up @@ -211,7 +220,10 @@ impl From<&VmConfig> for MachineConfig {
vcpu_count: value.vcpu_count,
mem_size_mib: value.mem_size_mib,
smt: value.smt,
cpu_template: value.cpu_template.as_ref().map(|template| template.into()),
cpu_template: value
.cpu_template
.as_ref()
.and_then(|template| template.into()),
track_dirty_pages: value.track_dirty_pages,
}
}
Expand Down

0 comments on commit 3f3ada7

Please sign in to comment.