From 0cefc2bafdcf27cc08f75ca642a3947796d2ec79 Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Wed, 23 Oct 2024 12:06:26 +0200 Subject: [PATCH 1/3] fix: error handling in AML code generation Right now, we include a few panics and assertions in the code that generates AML bytecode for our ACPI tables. Change the AML methods to explicitly return a Result type and substitute these panics and assertions with error types. Also, change some type methods that now need to return a Result as well. Signed-off-by: Babis Chalios --- src/acpi-tables/src/aml.rs | 528 +++++++++++++++++---------- src/vmm/src/acpi/mod.rs | 8 +- src/vmm/src/acpi/x86_64.rs | 4 +- src/vmm/src/device_manager/acpi.rs | 78 ++-- src/vmm/src/device_manager/legacy.rs | 34 +- src/vmm/src/device_manager/mmio.rs | 26 +- src/vmm/src/devices/acpi/vmgenid.rs | 14 +- 7 files changed, 431 insertions(+), 261 deletions(-) diff --git a/src/acpi-tables/src/aml.rs b/src/acpi-tables/src/aml.rs index 7d6a3d13f55..040bde4496f 100644 --- a/src/acpi-tables/src/aml.rs +++ b/src/acpi-tables/src/aml.rs @@ -8,13 +8,21 @@ use std::marker::PhantomData; +#[derive(Debug, Clone, thiserror::Error, displaydoc::Display)] +pub enum AmlError { + /// Aml Path is empty + NameEmpty, + /// Invalid name part length + InvalidPartLength, +} + pub trait Aml { - fn append_aml_bytes(&self, _v: &mut Vec); + fn append_aml_bytes(&self, _v: &mut Vec) -> Result<(), AmlError>; - fn to_aml_bytes(&self) -> Vec { + fn to_aml_bytes(&self) -> Result, AmlError> { let mut v = Vec::new(); - self.append_aml_bytes(&mut v); - v + self.append_aml_bytes(&mut v)?; + Ok(v) } } @@ -22,8 +30,9 @@ pub const ZERO: Zero = Zero {}; pub struct Zero {} impl Aml for Zero { - fn append_aml_bytes(&self, v: &mut Vec) { - v.push(0u8) + fn append_aml_bytes(&self, v: &mut Vec) -> Result<(), AmlError> { + v.push(0u8); + Ok(()) } } @@ -31,8 +40,9 @@ pub const ONE: One = One {}; pub struct One {} impl Aml for One { - fn append_aml_bytes(&self, v: &mut Vec) { - v.push(1u8) + fn append_aml_bytes(&self, v: &mut Vec) -> Result<(), AmlError> { + v.push(1u8); + Ok(()) } } @@ -40,8 +50,9 @@ pub const ONES: Ones = Ones {}; pub struct Ones {} impl Aml for Ones { - fn append_aml_bytes(&self, v: &mut Vec) { - v.push(0xffu8) + fn append_aml_bytes(&self, v: &mut Vec) -> Result<(), AmlError> { + v.push(0xffu8); + Ok(()) } } @@ -51,13 +62,13 @@ pub struct Path { } impl Aml for Path { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { if self.root { bytes.push(b'\\'); } match self.name_parts.len() { - 0 => panic!("Name cannot be empty"), + 0 => return Err(AmlError::NameEmpty), 1 => {} 2 => { bytes.push(0x2e); // DualNamePrefix @@ -71,27 +82,32 @@ impl Aml for Path { for part in &self.name_parts { bytes.extend_from_slice(part); } + + Ok(()) } } impl Path { - pub fn new(name: &str) -> Self { + pub fn new(name: &str) -> Result { let root = name.starts_with('\\'); let offset = root.into(); let mut name_parts = Vec::new(); for part in name[offset..].split('.') { - assert_eq!(part.len(), 4); + if part.len() != 4 { + return Err(AmlError::InvalidPartLength); + } let mut name_part = [0u8; 4]; name_part.copy_from_slice(part.as_bytes()); name_parts.push(name_part); } - Path { root, name_parts } + Ok(Path { root, name_parts }) } } -impl From<&str> for Path { - fn from(s: &str) -> Self { +impl TryFrom<&str> for Path { + type Error = AmlError; + fn try_from(s: &str) -> Result { Path::new(s) } } @@ -99,36 +115,40 @@ impl From<&str> for Path { pub type Byte = u8; impl Aml for Byte { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x0a); // BytePrefix bytes.push(*self); + Ok(()) } } pub type Word = u16; impl Aml for Word { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x0b); // WordPrefix - bytes.extend_from_slice(&self.to_le_bytes()) + bytes.extend_from_slice(&self.to_le_bytes()); + Ok(()) } } pub type DWord = u32; impl Aml for DWord { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x0c); // DWordPrefix - bytes.extend_from_slice(&self.to_le_bytes()) + bytes.extend_from_slice(&self.to_le_bytes()); + Ok(()) } } pub type QWord = u64; impl Aml for QWord { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x0e); // QWordPrefix - bytes.extend_from_slice(&self.to_le_bytes()) + bytes.extend_from_slice(&self.to_le_bytes()); + Ok(()) } } @@ -137,19 +157,20 @@ pub struct Name { } impl Aml for Name { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { // TODO: Refactor this to make more efficient but there are // lifetime/ownership challenges. - bytes.extend_from_slice(&self.bytes) + bytes.extend_from_slice(&self.bytes); + Ok(()) } } impl Name { - pub fn new(path: Path, inner: &dyn Aml) -> Self { + pub fn new(path: Path, inner: &dyn Aml) -> Result { let mut bytes = vec![0x08]; // NameOp - path.append_aml_bytes(&mut bytes); - inner.append_aml_bytes(&mut bytes); - Name { bytes } + path.append_aml_bytes(&mut bytes)?; + inner.append_aml_bytes(&mut bytes)?; + Ok(Name { bytes }) } } @@ -158,10 +179,10 @@ pub struct Package<'a> { } impl<'a> Aml for Package<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = vec![self.children.len().try_into().unwrap()]; for child in &self.children { - child.append_aml_bytes(&mut tmp); + child.append_aml_bytes(&mut tmp)?; } let pkg_length = create_pkg_length(&tmp, true); @@ -169,6 +190,7 @@ impl<'a> Aml for Package<'a> { bytes.push(0x12); // PackageOp bytes.extend_from_slice(&pkg_length); bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -234,8 +256,10 @@ pub struct EisaName { } impl EisaName { - pub fn new(name: &str) -> Self { - assert_eq!(name.len(), 7); + pub fn new(name: &str) -> Result { + if name.len() != 7 { + return Err(AmlError::InvalidPartLength); + } let data = name.as_bytes(); @@ -248,12 +272,12 @@ impl EisaName { | name.chars().nth(6).unwrap().to_digit(16).unwrap()) .swap_bytes(); - EisaName { value } + Ok(EisaName { value }) } } impl Aml for EisaName { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { self.value.append_aml_bytes(bytes) } } @@ -261,7 +285,7 @@ impl Aml for EisaName { pub type Usize = usize; impl Aml for Usize { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { if *self <= u8::MAX.into() { TryInto::::try_into(*self) .unwrap() @@ -291,16 +315,18 @@ fn append_aml_string(v: &str, bytes: &mut Vec) { pub type AmlStr = &'static str; impl Aml for AmlStr { - fn append_aml_bytes(&self, bytes: &mut Vec) { - append_aml_string(self, bytes) + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { + append_aml_string(self, bytes); + Ok(()) } } pub type AmlString = String; impl Aml for AmlString { - fn append_aml_bytes(&self, bytes: &mut Vec) { - append_aml_string(self, bytes) + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { + append_aml_string(self, bytes); + Ok(()) } } @@ -309,11 +335,11 @@ pub struct ResourceTemplate<'a> { } impl<'a> Aml for ResourceTemplate<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); // Add buffer data for child in &self.children { - child.append_aml_bytes(&mut tmp); + child.append_aml_bytes(&mut tmp)?; } // Mark with end and mark checksum as as always valid @@ -322,7 +348,7 @@ impl<'a> Aml for ResourceTemplate<'a> { // Buffer length is an encoded integer including buffer data // and EndTag and checksum byte - let mut buffer_length = tmp.len().to_aml_bytes(); + let mut buffer_length = tmp.len().to_aml_bytes()?; buffer_length.reverse(); for byte in buffer_length { tmp.insert(0, byte); @@ -334,6 +360,7 @@ impl<'a> Aml for ResourceTemplate<'a> { bytes.push(0x11); // BufferOp bytes.extend_from_slice(&pkg_length); bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -360,13 +387,14 @@ impl Memory32Fixed { } impl Aml for Memory32Fixed { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x86); // Memory32Fixed bytes.extend_from_slice(&9u16.to_le_bytes()); // 9 bytes of payload bytes.push(self.read_write.into()); bytes.extend_from_slice(&self.base.to_le_bytes()); bytes.extend_from_slice(&self.length.to_le_bytes()); + Ok(()) } } @@ -431,7 +459,7 @@ impl AddressSpace { } impl Aml for AddressSpace { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { self.push_header( bytes, 0x88, // Word Address Space Descriptor @@ -444,11 +472,12 @@ impl Aml for AddressSpace { bytes.extend_from_slice(&0u16.to_le_bytes()); // Translation let len = self.max - self.min + 1; bytes.extend_from_slice(&len.to_le_bytes()); // Length + Ok(()) } } impl Aml for AddressSpace { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { self.push_header( bytes, 0x87, // DWord Address Space Descriptor @@ -461,11 +490,12 @@ impl Aml for AddressSpace { bytes.extend_from_slice(&0u32.to_le_bytes()); // Translation let len = self.max - self.min + 1; bytes.extend_from_slice(&len.to_le_bytes()); // Length + Ok(()) } } impl Aml for AddressSpace { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { self.push_header( bytes, 0x8A, // QWord Address Space Descriptor @@ -478,6 +508,7 @@ impl Aml for AddressSpace { bytes.extend_from_slice(&0u64.to_le_bytes()); // Translation let len = self.max - self.min + 1; bytes.extend_from_slice(&len.to_le_bytes()); // Length + Ok(()) } } @@ -500,13 +531,14 @@ impl Io { } impl Aml for Io { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x47); // Io Port Descriptor bytes.push(1); // IODecode16 bytes.extend_from_slice(&self.min.to_le_bytes()); bytes.extend_from_slice(&self.max.to_le_bytes()); bytes.push(self.alignment); bytes.push(self.length); + Ok(()) } } @@ -537,7 +569,7 @@ impl Interrupt { } impl Aml for Interrupt { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x89); // Extended IRQ Descriptor bytes.extend_from_slice(&6u16.to_le_bytes()); let flags = u8::from(self.shared) << 3 @@ -547,6 +579,7 @@ impl Aml for Interrupt { bytes.push(flags); bytes.push(1u8); // count bytes.extend_from_slice(&self.number.to_le_bytes()); + Ok(()) } } @@ -556,12 +589,12 @@ pub struct Device<'a> { } impl<'a> Aml for Device<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.path.append_aml_bytes(&mut tmp); + self.path.append_aml_bytes(&mut tmp)?; for child in &self.children { - child.append_aml_bytes(&mut tmp); + child.append_aml_bytes(&mut tmp)?; } let pkg_length = create_pkg_length(&tmp, true); @@ -570,6 +603,7 @@ impl<'a> Aml for Device<'a> { bytes.push(0x82); // DeviceOp bytes.extend_from_slice(&pkg_length); bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -585,18 +619,19 @@ pub struct Scope<'a> { } impl<'a> Aml for Scope<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.path.append_aml_bytes(&mut tmp); + self.path.append_aml_bytes(&mut tmp)?; for child in &self.children { - child.append_aml_bytes(&mut tmp); + child.append_aml_bytes(&mut tmp)?; } let pkg_length = create_pkg_length(&tmp, true); bytes.push(0x10); // ScopeOp bytes.extend_from_slice(&pkg_length); - bytes.extend_from_slice(&tmp) + bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -625,20 +660,21 @@ impl<'a> Method<'a> { } impl<'a> Aml for Method<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.path.append_aml_bytes(&mut tmp); + self.path.append_aml_bytes(&mut tmp)?; let flags: u8 = (self.args & 0x7) | u8::from(self.serialized) << 3; tmp.push(flags); for child in &self.children { - child.append_aml_bytes(&mut tmp); + child.append_aml_bytes(&mut tmp)?; } let pkg_length = create_pkg_length(&tmp, true); bytes.push(0x14); // MethodOp bytes.extend_from_slice(&pkg_length); - bytes.extend_from_slice(&tmp) + bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -653,9 +689,10 @@ impl<'a> Return<'a> { } impl<'a> Aml for Return<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0xa4); // ReturnOp - self.value.append_aml_bytes(bytes); + self.value.append_aml_bytes(bytes)?; + Ok(()) } } @@ -706,9 +743,9 @@ impl Field { } impl Aml for Field { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.path.append_aml_bytes(&mut tmp); + self.path.append_aml_bytes(&mut tmp)?; let flags: u8 = self.access_type as u8 | (self.update_rule as u8) << 5; tmp.push(flags); @@ -731,7 +768,8 @@ impl Aml for Field { bytes.push(0x5b); // ExtOpPrefix bytes.push(0x81); // FieldOp bytes.extend_from_slice(&pkg_length); - bytes.extend_from_slice(&tmp) + bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -768,13 +806,14 @@ impl OpRegion { } impl Aml for OpRegion { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x5b); // ExtOpPrefix bytes.push(0x80); // OpRegionOp - self.path.append_aml_bytes(bytes); + self.path.append_aml_bytes(bytes)?; bytes.push(self.space as u8); - self.offset.append_aml_bytes(bytes); // RegionOffset - self.length.append_aml_bytes(bytes); // RegionLen + self.offset.append_aml_bytes(bytes)?; // RegionOffset + self.length.append_aml_bytes(bytes)?; // RegionLen + Ok(()) } } @@ -793,11 +832,11 @@ impl<'a> If<'a> { } impl<'a> Aml for If<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.predicate.append_aml_bytes(&mut tmp); + self.predicate.append_aml_bytes(&mut tmp)?; for child in self.if_children.iter() { - child.append_aml_bytes(&mut tmp); + child.append_aml_bytes(&mut tmp)?; } let pkg_length = create_pkg_length(&tmp, true); @@ -805,6 +844,7 @@ impl<'a> Aml for If<'a> { bytes.push(0xa0); // IfOp bytes.extend_from_slice(&pkg_length); bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -820,10 +860,11 @@ impl<'a> Equal<'a> { } impl<'a> Aml for Equal<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x93); // LEqualOp - self.left.append_aml_bytes(bytes); - self.right.append_aml_bytes(bytes); + self.left.append_aml_bytes(bytes)?; + self.right.append_aml_bytes(bytes)?; + Ok(()) } } @@ -839,28 +880,35 @@ impl<'a> LessThan<'a> { } impl<'a> Aml for LessThan<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x95); // LLessOp - self.left.append_aml_bytes(bytes); - self.right.append_aml_bytes(bytes); + self.left.append_aml_bytes(bytes)?; + self.right.append_aml_bytes(bytes)?; + Ok(()) } } pub struct Arg(pub u8); impl Aml for Arg { - fn append_aml_bytes(&self, bytes: &mut Vec) { - assert!(self.0 <= 6); + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { + if self.0 > 6 { + return Err(AmlError::InvalidPartLength); + } bytes.push(0x68 + self.0); // Arg0Op + Ok(()) } } pub struct Local(pub u8); impl Aml for Local { - fn append_aml_bytes(&self, bytes: &mut Vec) { - assert!(self.0 <= 7); + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { + if self.0 > 7 { + return Err(AmlError::InvalidPartLength); + } bytes.push(0x60 + self.0); // Local0Op + Ok(()) } } @@ -876,10 +924,11 @@ impl<'a> Store<'a> { } impl<'a> Aml for Store<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x70); // StoreOp - self.value.append_aml_bytes(bytes); - self.name.append_aml_bytes(bytes); + self.value.append_aml_bytes(bytes)?; + self.name.append_aml_bytes(bytes)?; + Ok(()) } } @@ -895,11 +944,12 @@ impl Mutex { } impl Aml for Mutex { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x5b); // ExtOpPrefix bytes.push(0x01); // MutexOp - self.path.append_aml_bytes(bytes); + self.path.append_aml_bytes(bytes)?; bytes.push(self.sync_level); + Ok(()) } } @@ -915,11 +965,12 @@ impl Acquire { } impl Aml for Acquire { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x5b); // ExtOpPrefix bytes.push(0x23); // AcquireOp - self.mutex.append_aml_bytes(bytes); + self.mutex.append_aml_bytes(bytes)?; bytes.extend_from_slice(&self.timeout.to_le_bytes()); + Ok(()) } } @@ -934,10 +985,11 @@ impl Release { } impl Aml for Release { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x5b); // ExtOpPrefix bytes.push(0x27); // ReleaseOp - self.mutex.append_aml_bytes(bytes); + self.mutex.append_aml_bytes(bytes)?; + Ok(()) } } @@ -953,10 +1005,11 @@ impl<'a> Notify<'a> { } impl<'a> Aml for Notify<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x86); // NotifyOp - self.object.append_aml_bytes(bytes); - self.value.append_aml_bytes(bytes); + self.object.append_aml_bytes(bytes)?; + self.value.append_aml_bytes(bytes)?; + Ok(()) } } @@ -975,11 +1028,11 @@ impl<'a> While<'a> { } impl<'a> Aml for While<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.predicate.append_aml_bytes(&mut tmp); + self.predicate.append_aml_bytes(&mut tmp)?; for child in self.while_children.iter() { - child.append_aml_bytes(&mut tmp) + child.append_aml_bytes(&mut tmp)?; } let pkg_length = create_pkg_length(&tmp, true); @@ -987,6 +1040,7 @@ impl<'a> Aml for While<'a> { bytes.push(0xa2); // WhileOp bytes.extend_from_slice(&pkg_length); bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -1005,11 +1059,11 @@ macro_rules! binary_op { } impl<'a> Aml for $name<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push($opcode); // Op for the binary operator - self.a.append_aml_bytes(bytes); - self.b.append_aml_bytes(bytes); - self.target.append_aml_bytes(bytes); + self.a.append_aml_bytes(bytes)?; + self.b.append_aml_bytes(bytes)?; + self.target.append_aml_bytes(bytes) } } }; @@ -1044,11 +1098,12 @@ impl<'a> MethodCall<'a> { } impl<'a> Aml for MethodCall<'a> { - fn append_aml_bytes(&self, bytes: &mut Vec) { - self.name.append_aml_bytes(bytes); + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { + self.name.append_aml_bytes(bytes)?; for arg in self.args.iter() { - arg.append_aml_bytes(bytes); + arg.append_aml_bytes(bytes)?; } + Ok(()) } } @@ -1063,9 +1118,9 @@ impl Buffer { } impl Aml for Buffer { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { let mut tmp = Vec::new(); - self.data.len().append_aml_bytes(&mut tmp); + self.data.len().append_aml_bytes(&mut tmp)?; tmp.extend_from_slice(&self.data); let pkg_length = create_pkg_length(&tmp, true); @@ -1073,6 +1128,7 @@ impl Aml for Buffer { bytes.push(0x11); // BufferOp bytes.extend_from_slice(&pkg_length); bytes.extend_from_slice(&tmp); + Ok(()) } } @@ -1095,20 +1151,20 @@ impl<'a, T> CreateField<'a, T> { } impl<'a> Aml for CreateField<'a, u64> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x8f); // CreateQWordFieldOp - self.buffer.append_aml_bytes(bytes); - self.offset.append_aml_bytes(bytes); - self.field.append_aml_bytes(bytes); + self.buffer.append_aml_bytes(bytes)?; + self.offset.append_aml_bytes(bytes)?; + self.field.append_aml_bytes(bytes) } } impl<'a> Aml for CreateField<'a, u32> { - fn append_aml_bytes(&self, bytes: &mut Vec) { + fn append_aml_bytes(&self, bytes: &mut Vec) -> Result<(), AmlError> { bytes.push(0x8a); // CreateDWordFieldOp - self.buffer.append_aml_bytes(bytes); - self.offset.append_aml_bytes(bytes); - self.field.append_aml_bytes(bytes); + self.buffer.append_aml_bytes(bytes)?; + self.offset.append_aml_bytes(bytes)?; + self.field.append_aml_bytes(bytes) } } @@ -1143,19 +1199,25 @@ mod tests { ]; assert_eq!( Device::new( - "_SB_.COM1".into(), + "_SB_.COM1".try_into().unwrap(), vec![ - &Name::new("_HID".into(), &EisaName::new("PNP0501")), &Name::new( - "_CRS".into(), + "_HID".try_into().unwrap(), + &EisaName::new("PNP0501").unwrap() + ) + .unwrap(), + &Name::new( + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![ &Interrupt::new(true, true, false, false, 4), &Io::new(0x3f8, 0x3f8, 0, 0x8) ]) ) + .unwrap() ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &com1_device[..] ); } @@ -1181,17 +1243,19 @@ mod tests { assert_eq!( Scope::new( - "_SB_.MBRD".into(), + "_SB_.MBRD".try_into().unwrap(), vec![&Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![&Memory32Fixed::new( true, 0xE800_0000, 0x1000_0000 )]) - )] + ) + .unwrap()] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &mbrd_scope[..] ); } @@ -1212,10 +1276,12 @@ mod tests { assert_eq!( Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![&Memory32Fixed::new(true, 0xE800_0000, 0x1000_0000)]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), crs_memory_32_fixed ); @@ -1273,10 +1339,12 @@ mod tests { assert_eq!( Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![&AddressSpace::new_bus_number(0x0u16, 0xffu16),]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), &crs_word_bus_number ); @@ -1290,13 +1358,15 @@ mod tests { assert_eq!( Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![ &AddressSpace::new_io(0x0u16, 0xcf7u16), &AddressSpace::new_io(0xd00u16, 0xffffu16), ]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), &crs_word_io[..] ); @@ -1311,7 +1381,7 @@ mod tests { assert_eq!( Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![ &AddressSpace::new_memory( AddressSpaceCachable::Cacheable, @@ -1327,7 +1397,9 @@ mod tests { ), ]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), &crs_dword_memory[..] ); @@ -1342,7 +1414,7 @@ mod tests { assert_eq!( Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![&AddressSpace::new_memory( AddressSpaceCachable::Cacheable, true, @@ -1350,7 +1422,9 @@ mod tests { 0xf_ffff_ffffu64 )]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), &crs_qword_memory[..] ); @@ -1375,13 +1449,15 @@ mod tests { assert_eq!( Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![ &Interrupt::new(true, true, false, false, 4), &Io::new(0x3f8, 0x3f8, 0, 0x8) ]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), &interrupt_io_data[..] ); } @@ -1411,45 +1487,62 @@ mod tests { // }) let s5_sleep_data = [0x08, 0x5F, 0x53, 0x35, 0x5F, 0x12, 0x04, 0x01, 0x0A, 0x05]; - let s5 = Name::new("_S5_".into(), &Package::new(vec![&5u8])); + let s5 = Name::new("_S5_".try_into().unwrap(), &Package::new(vec![&5u8])).unwrap(); - assert_eq!(s5_sleep_data.to_vec(), s5.to_aml_bytes()); + assert_eq!(s5_sleep_data.to_vec(), s5.to_aml_bytes().unwrap()); } #[test] fn test_eisa_name() { assert_eq!( - Name::new("_HID".into(), &EisaName::new("PNP0501")).to_aml_bytes(), + Name::new( + "_HID".try_into().unwrap(), + &EisaName::new("PNP0501").unwrap() + ) + .unwrap() + .to_aml_bytes() + .unwrap(), [0x08, 0x5F, 0x48, 0x49, 0x44, 0x0C, 0x41, 0xD0, 0x05, 0x01], ) } #[test] fn test_name_path() { assert_eq!( - (&"_SB_".into() as &Path).to_aml_bytes(), + (&"_SB_".try_into().unwrap() as &Path) + .to_aml_bytes() + .unwrap(), [0x5Fu8, 0x53, 0x42, 0x5F] ); assert_eq!( - (&"\\_SB_".into() as &Path).to_aml_bytes(), + (&"\\_SB_".try_into().unwrap() as &Path) + .to_aml_bytes() + .unwrap(), [0x5C, 0x5F, 0x53, 0x42, 0x5F] ); assert_eq!( - (&"_SB_.COM1".into() as &Path).to_aml_bytes(), + (&"_SB_.COM1".try_into().unwrap() as &Path) + .to_aml_bytes() + .unwrap(), [0x2E, 0x5F, 0x53, 0x42, 0x5F, 0x43, 0x4F, 0x4D, 0x31] ); assert_eq!( - (&"_SB_.PCI0._HID".into() as &Path).to_aml_bytes(), + (&"_SB_.PCI0._HID".try_into().unwrap() as &Path) + .to_aml_bytes() + .unwrap(), [0x2F, 0x03, 0x5F, 0x53, 0x42, 0x5F, 0x50, 0x43, 0x49, 0x30, 0x5F, 0x48, 0x49, 0x44] ); } #[test] fn test_numbers() { - assert_eq!(128u8.to_aml_bytes(), [0x0a, 0x80]); - assert_eq!(1024u16.to_aml_bytes(), [0x0b, 0x0, 0x04]); - assert_eq!((16u32 << 20).to_aml_bytes(), [0x0c, 0x00, 0x00, 0x0, 0x01]); + assert_eq!(128u8.to_aml_bytes().unwrap(), [0x0a, 0x80]); + assert_eq!(1024u16.to_aml_bytes().unwrap(), [0x0b, 0x0, 0x04]); + assert_eq!( + (16u32 << 20).to_aml_bytes().unwrap(), + [0x0c, 0x00, 0x00, 0x0, 0x01] + ); assert_eq!( - 0xdeca_fbad_deca_fbadu64.to_aml_bytes(), + 0xdeca_fbad_deca_fbadu64.to_aml_bytes().unwrap(), [0x0e, 0xad, 0xfb, 0xca, 0xde, 0xad, 0xfb, 0xca, 0xde] ); } @@ -1457,7 +1550,10 @@ mod tests { #[test] fn test_name() { assert_eq!( - Name::new("_SB_.PCI0._UID".into(), &0x1234u16).to_aml_bytes(), + Name::new("_SB_.PCI0._UID".try_into().unwrap(), &0x1234u16) + .unwrap() + .to_aml_bytes() + .unwrap(), [ 0x08, // NameOp 0x2F, // MultiNamePrefix @@ -1474,11 +1570,11 @@ mod tests { #[test] fn test_string() { assert_eq!( - (&"ACPI" as &dyn Aml).to_aml_bytes(), + (&"ACPI" as &dyn Aml).to_aml_bytes().unwrap(), [0x0d, b'A', b'C', b'P', b'I', 0] ); assert_eq!( - "ACPI".to_owned().to_aml_bytes(), + "ACPI".to_owned().to_aml_bytes().unwrap(), [0x0d, b'A', b'C', b'P', b'I', 0] ); } @@ -1486,7 +1582,14 @@ mod tests { #[test] fn test_method() { assert_eq!( - Method::new("_STA".into(), 0, false, vec![&Return::new(&0xfu8)]).to_aml_bytes(), + Method::new( + "_STA".try_into().unwrap(), + 0, + false, + vec![&Return::new(&0xfu8)] + ) + .to_aml_bytes() + .unwrap(), [0x14, 0x09, 0x5F, 0x53, 0x54, 0x41, 0x00, 0xA4, 0x0A, 0x0F] ); } @@ -1513,7 +1616,7 @@ mod tests { assert_eq!( Field::new( - "PRST".into(), + "PRST".try_into().unwrap(), FieldAccessType::Byte, FieldUpdateRule::WriteAsZeroes, vec![ @@ -1526,7 +1629,8 @@ mod tests { FieldEntry::Named(*b"CCMD", 8) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &field_data[..] ); @@ -1544,7 +1648,7 @@ mod tests { assert_eq!( Field::new( - "PRST".into(), + "PRST".try_into().unwrap(), FieldAccessType::DWord, FieldUpdateRule::Preserve, vec![ @@ -1553,7 +1657,8 @@ mod tests { FieldEntry::Named(*b"CDAT", 32) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &field_data[..] ); } @@ -1566,7 +1671,14 @@ mod tests { ]; assert_eq!( - OpRegion::new("PRST".into(), OpRegionSpace::SystemIo, 0xcd8, 0xc).to_aml_bytes(), + OpRegion::new( + "PRST".try_into().unwrap(), + OpRegionSpace::SystemIo, + 0xcd8, + 0xc + ) + .to_aml_bytes() + .unwrap(), &op_region_data[..] ); } @@ -1586,7 +1698,7 @@ mod tests { assert_eq!( Method::new( - "TEST".into(), + "TEST".try_into().unwrap(), 1, false, vec![ @@ -1594,7 +1706,8 @@ mod tests { &Return::new(&ZERO) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &arg_if_data ); } @@ -1614,7 +1727,7 @@ mod tests { ]; assert_eq!( Method::new( - "TEST".into(), + "TEST".try_into().unwrap(), 0, false, vec![ @@ -1623,7 +1736,8 @@ mod tests { &Return::new(&ZERO) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &local_if_data ); } @@ -1649,26 +1763,31 @@ mod tests { 0xFF, 0xFF, 0x70, 0x01, 0x60, 0x5B, 0x27, 0x4D, 0x4C, 0x43, 0x4B, ]; - let mutex = Mutex::new("MLCK".into(), 0); + let mutex = Mutex::new("MLCK".try_into().unwrap(), 0); assert_eq!( Device::new( - "_SB_.MHPC".into(), + "_SB_.MHPC".try_into().unwrap(), vec![ - &Name::new("_HID".into(), &EisaName::new("PNP0A06")), + &Name::new( + "_HID".try_into().unwrap(), + &EisaName::new("PNP0A06").unwrap() + ) + .unwrap(), &mutex, &Method::new( - "TEST".into(), + "TEST".try_into().unwrap(), 0, false, vec![ - &Acquire::new("MLCK".into(), 0xffff), + &Acquire::new("MLCK".try_into().unwrap(), 0xffff), &Store::new(&Local(0), &ONE), - &Release::new("MLCK".into()) + &Release::new("MLCK".try_into().unwrap()) ] ) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &mutex_data[..] ); } @@ -1691,18 +1810,23 @@ mod tests { assert_eq!( Device::new( - "_SB_.MHPC".into(), + "_SB_.MHPC".try_into().unwrap(), vec![ - &Name::new("_HID".into(), &EisaName::new("PNP0A06")), + &Name::new( + "_HID".try_into().unwrap(), + &EisaName::new("PNP0A06").unwrap() + ) + .unwrap(), &Method::new( - "TEST".into(), + "TEST".try_into().unwrap(), 0, false, - vec![&Notify::new(&Path::new("MHPC"), &ONE),] + vec![&Notify::new(&Path::new("MHPC").unwrap(), &ONE),] ) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), ¬ify_data[..] ); } @@ -1730,11 +1854,15 @@ mod tests { assert_eq!( Device::new( - "_SB_.MHPC".into(), + "_SB_.MHPC".try_into().unwrap(), vec![ - &Name::new("_HID".into(), &EisaName::new("PNP0A06")), + &Name::new( + "_HID".try_into().unwrap(), + &EisaName::new("PNP0A06").unwrap() + ) + .unwrap(), &Method::new( - "TEST".into(), + "TEST".try_into().unwrap(), 0, false, vec![ @@ -1747,7 +1875,8 @@ mod tests { ) ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &while_data[..] ) } @@ -1771,21 +1900,26 @@ mod tests { let mut methods = Vec::new(); methods.extend_from_slice( &Method::new( - "TST1".into(), + "TST1".try_into().unwrap(), 1, false, - vec![&MethodCall::new("TST2".into(), vec![&ONE, &ONE])], + vec![&MethodCall::new( + "TST2".try_into().unwrap(), + vec![&ONE, &ONE], + )], ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), ); methods.extend_from_slice( &Method::new( - "TST2".into(), + "TST2".try_into().unwrap(), 2, false, - vec![&MethodCall::new("TST1".into(), vec![&ONE])], + vec![&MethodCall::new("TST1".try_into().unwrap(), vec![&ONE])], ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), ); assert_eq!(&methods[..], &test_data[..]) } @@ -1803,10 +1937,12 @@ mod tests { assert_eq!( Name::new( - "_MAT".into(), + "_MAT".try_into().unwrap(), &Buffer::new(vec![0x00, 0x08, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00]) ) - .to_aml_bytes(), + .unwrap() + .to_aml_bytes() + .unwrap(), &buffer_data[..] ) } @@ -1841,25 +1977,39 @@ mod tests { assert_eq!( Method::new( - "MCRS".into(), + "MCRS".try_into().unwrap(), 0, true, vec![ &Name::new( - "MR64".into(), + "MR64".try_into().unwrap(), &ResourceTemplate::new(vec![&AddressSpace::new_memory( AddressSpaceCachable::Cacheable, true, 0x0000_0000_0000_0000u64, 0xFFFF_FFFF_FFFF_FFFEu64 )]) + ) + .unwrap(), + &CreateField::::new( + &Path::new("MR64").unwrap(), + &14usize, + "MIN_".try_into().unwrap() + ), + &CreateField::::new( + &Path::new("MR64").unwrap(), + &22usize, + "MAX_".try_into().unwrap() + ), + &CreateField::::new( + &Path::new("MR64").unwrap(), + &38usize, + "LEN_".try_into().unwrap() ), - &CreateField::::new(&Path::new("MR64"), &14usize, "MIN_".into()), - &CreateField::::new(&Path::new("MR64"), &22usize, "MAX_".into()), - &CreateField::::new(&Path::new("MR64"), &38usize, "LEN_".into()), ] ) - .to_aml_bytes(), + .to_aml_bytes() + .unwrap(), &data[..] ); } diff --git a/src/vmm/src/acpi/mod.rs b/src/vmm/src/acpi/mod.rs index 75de9edfebc..247c832e665 100644 --- a/src/vmm/src/acpi/mod.rs +++ b/src/vmm/src/acpi/mod.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use acpi_tables::fadt::{FADT_F_HW_REDUCED_ACPI, FADT_F_PWR_BUTTON, FADT_F_SLP_BUTTON}; -use acpi_tables::{Aml, Dsdt, Fadt, Madt, Rsdp, Sdt, Xsdt}; +use acpi_tables::{aml, Aml, Dsdt, Fadt, Madt, Rsdp, Sdt, Xsdt}; use log::{debug, error}; use vm_allocator::AllocPolicy; @@ -37,6 +37,8 @@ pub enum AcpiError { VmAllocator(#[from] vm_allocator::Error), /// ACPI tables error: {0} AcpiTables(#[from] acpi_tables::AcpiError), + /// Error creating AML bytecode: {0} + AmlError(#[from] aml::AmlError), } /// Helper type that holds the guest memory in which we write the tables in and a resource @@ -86,10 +88,10 @@ impl<'a> AcpiTableWriter<'a> { dsdt_data.extend_from_slice(&mmio_device_manager.dsdt_data); // Add GED and VMGenID AML data. - acpi_device_manager.append_aml_bytes(&mut dsdt_data); + acpi_device_manager.append_aml_bytes(&mut dsdt_data)?; // Architecture specific DSDT data - setup_arch_dsdt(&mut dsdt_data); + setup_arch_dsdt(&mut dsdt_data)?; let mut dsdt = Dsdt::new(OEM_ID, *b"FCVMDSDT", OEM_REVISION, dsdt_data); self.write_acpi_table(&mut dsdt) diff --git a/src/vmm/src/acpi/x86_64.rs b/src/vmm/src/acpi/x86_64.rs index 2f06c35264f..7d2e9f0364a 100644 --- a/src/vmm/src/acpi/x86_64.rs +++ b/src/vmm/src/acpi/x86_64.rs @@ -8,7 +8,7 @@ use acpi_tables::fadt::{ IAPC_BOOT_ARG_FLAGS_VGA_NOT_PRESENT, }; use acpi_tables::madt::{IoAPIC, LocalAPIC}; -use acpi_tables::Fadt; +use acpi_tables::{aml, Fadt}; use vm_memory::GuestAddress; use zerocopy::IntoBytes; @@ -41,7 +41,7 @@ pub(crate) fn setup_arch_fadt(fadt: &mut Fadt) { } #[inline(always)] -pub(crate) fn setup_arch_dsdt(dsdt_data: &mut Vec) { +pub(crate) fn setup_arch_dsdt(dsdt_data: &mut Vec) -> Result<(), aml::AmlError> { PortIODeviceManager::append_aml_bytes(dsdt_data) } diff --git a/src/vmm/src/device_manager/acpi.rs b/src/vmm/src/device_manager/acpi.rs index 216039a7644..03315cbcc3c 100644 --- a/src/vmm/src/device_manager/acpi.rs +++ b/src/vmm/src/device_manager/acpi.rs @@ -41,45 +41,49 @@ impl ACPIDeviceManager { } impl Aml for ACPIDeviceManager { - fn append_aml_bytes(&self, v: &mut Vec) { + fn append_aml_bytes(&self, v: &mut Vec) -> Result<(), aml::AmlError> { // If we have a VMGenID device, create the AML for the device and GED interrupt handler - self.vmgenid.as_ref().inspect(|vmgenid| { - // AML for GED - aml::Device::new( - "_SB_.GED_".into(), - vec![ - &aml::Name::new("_HID".into(), &"ACPI0013"), - &aml::Name::new( - "_CRS".into(), - &aml::ResourceTemplate::new(vec![&aml::Interrupt::new( + match self.vmgenid.as_ref() { + Some(vmgenid) => { + // AML for GED + aml::Device::new( + "_SB_.GED_".try_into()?, + vec![ + &aml::Name::new("_HID".try_into()?, &"ACPI0013")?, + &aml::Name::new( + "_CRS".try_into()?, + &aml::ResourceTemplate::new(vec![&aml::Interrupt::new( + true, + true, + false, + false, + vmgenid.gsi, + )]), + )?, + &aml::Method::new( + "_EVT".try_into()?, + 1, true, - true, - false, - false, - vmgenid.gsi, - )]), - ), - &aml::Method::new( - "_EVT".into(), - 1, - true, - vec![&aml::If::new( - // We know that the maximum IRQ number fits in a u8. We have up to 32 - // IRQs in x86 and up to 128 in ARM (look into - // `vmm::crate::arch::layout::IRQ_MAX`) - #[allow(clippy::cast_possible_truncation)] - &aml::Equal::new(&aml::Arg(0), &(vmgenid.gsi as u8)), - vec![&aml::Notify::new( - &aml::Path::new("\\_SB_.VGEN"), - &0x80usize, + vec![&aml::If::new( + // We know that the maximum IRQ number fits in a u8. We have up to + // 32 IRQs in x86 and up to 128 in + // ARM (look into + // `vmm::crate::arch::layout::IRQ_MAX`) + #[allow(clippy::cast_possible_truncation)] + &aml::Equal::new(&aml::Arg(0), &(vmgenid.gsi as u8)), + vec![&aml::Notify::new( + &aml::Path::new("\\_SB_.VGEN")?, + &0x80usize, + )], )], - )], - ), - ], - ) - .append_aml_bytes(v); - // AML for VMGenID itself. - vmgenid.append_aml_bytes(v); - }); + ), + ], + ) + .append_aml_bytes(v)?; + // AML for VMGenID itself. + vmgenid.append_aml_bytes(v) + } + None => Ok(()), + } } } diff --git a/src/vmm/src/device_manager/legacy.rs b/src/vmm/src/device_manager/legacy.rs index dc46a5172d3..45842d933b2 100644 --- a/src/vmm/src/device_manager/legacy.rs +++ b/src/vmm/src/device_manager/legacy.rs @@ -9,6 +9,7 @@ use std::fmt::Debug; use std::sync::{Arc, Mutex}; +use acpi_tables::aml::AmlError; use acpi_tables::{aml, Aml}; use kvm_ioctls::VmFd; use libc::EFD_NONBLOCK; @@ -169,7 +170,7 @@ impl PortIODeviceManager { Ok(()) } - pub(crate) fn append_aml_bytes(bytes: &mut Vec) { + pub(crate) fn append_aml_bytes(bytes: &mut Vec) -> Result<(), AmlError> { // Set up COM devices let gsi = [ Self::COM_EVT_1_3_GSI, @@ -180,13 +181,13 @@ impl PortIODeviceManager { for com in 0u8..4 { // COM1 aml::Device::new( - format!("_SB_.COM{}", com + 1).as_str().into(), + format!("_SB_.COM{}", com + 1).as_str().try_into()?, vec![ - &aml::Name::new("_HID".into(), &aml::EisaName::new("PNP0501")), - &aml::Name::new("_UID".into(), &com), - &aml::Name::new("_DDN".into(), &format!("COM{}", com + 1)), + &aml::Name::new("_HID".try_into()?, &aml::EisaName::new("PNP0501")?)?, + &aml::Name::new("_UID".try_into()?, &com)?, + &aml::Name::new("_DDN".try_into()?, &format!("COM{}", com + 1))?, &aml::Name::new( - "_CRS".into(), + "_CRS".try_into().unwrap(), &aml::ResourceTemplate::new(vec![ &aml::Interrupt::new(true, true, false, false, gsi[com as usize]), &aml::Io::new( @@ -200,19 +201,24 @@ impl PortIODeviceManager { PortIODeviceManager::SERIAL_PORT_SIZE.try_into().unwrap(), ), ]), - ), + )?, ], ) - .append_aml_bytes(bytes); + .append_aml_bytes(bytes)?; } // Setup i8042 aml::Device::new( - "_SB_.PS2_".into(), + "_SB_.PS2_".try_into()?, vec![ - &aml::Name::new("_HID".into(), &aml::EisaName::new("PNP0303")), - &aml::Method::new("_STA".into(), 0, false, vec![&aml::Return::new(&0x0fu8)]), + &aml::Name::new("_HID".try_into()?, &aml::EisaName::new("PNP0303")?)?, + &aml::Method::new( + "_STA".try_into()?, + 0, + false, + vec![&aml::Return::new(&0x0fu8)], + ), &aml::Name::new( - "_CRS".into(), + "_CRS".try_into()?, &aml::ResourceTemplate::new(vec![ &aml::Io::new( PortIODeviceManager::I8042_KDB_DATA_REGISTER_ADDRESS @@ -228,10 +234,10 @@ impl PortIODeviceManager { &aml::Io::new(0x0064, 0x0064, 1u8, 1u8), &aml::Interrupt::new(true, true, false, false, Self::KBD_EVT_GSI), ]), - ), + )?, ], ) - .append_aml_bytes(bytes); + .append_aml_bytes(bytes) } } diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index cba9047d564..00c155abcfd 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -60,6 +60,9 @@ pub enum MmioError { RegisterIoEvent(kvm_ioctls::Error), /// Failed to register irqfd: {0} RegisterIrqFd(kvm_ioctls::Error), + #[cfg(target_arch = "x86_64")] + /// Failed to create AML code for device + AmlError(#[from] aml::AmlError), } /// This represents the size of the mmio device specified to the kernel through ACPI and as a @@ -81,20 +84,25 @@ pub struct MMIODeviceInfo { } #[cfg(target_arch = "x86_64")] -fn add_virtio_aml(dsdt_data: &mut Vec, addr: u64, len: u64, irq: u32) { +fn add_virtio_aml( + dsdt_data: &mut Vec, + addr: u64, + len: u64, + irq: u32, +) -> Result<(), aml::AmlError> { let dev_id = irq - crate::arch::IRQ_BASE; debug!( "acpi: Building AML for VirtIO device _SB_.V{:03}. memory range: {:#010x}:{} irq: {}", dev_id, addr, len, irq ); aml::Device::new( - format!("V{:03}", dev_id).as_str().into(), + format!("V{:03}", dev_id).as_str().try_into()?, vec![ - &aml::Name::new("_HID".into(), &"LNRO0005"), - &aml::Name::new("_UID".into(), &dev_id), - &aml::Name::new("_CCA".into(), &aml::ONE), + &aml::Name::new("_HID".try_into()?, &"LNRO0005")?, + &aml::Name::new("_UID".try_into()?, &dev_id)?, + &aml::Name::new("_CCA".try_into()?, &aml::ONE)?, &aml::Name::new( - "_CRS".into(), + "_CRS".try_into()?, &aml::ResourceTemplate::new(vec![ &aml::Memory32Fixed::new( true, @@ -103,10 +111,10 @@ fn add_virtio_aml(dsdt_data: &mut Vec, addr: u64, len: u64, irq: u32) { ), &aml::Interrupt::new(true, true, false, false, irq), ]), - ), + )?, ], ) - .append_aml_bytes(dsdt_data); + .append_aml_bytes(dsdt_data) } /// Manages the complexities of registering a MMIO device. @@ -250,7 +258,7 @@ impl MMIODeviceManager { // We are sure that `irqs` has at least one element; allocate_mmio_resources makes // sure of it. device_info.irqs[0], - ); + )?; } Ok(device_info) } diff --git a/src/vmm/src/devices/acpi/vmgenid.rs b/src/vmm/src/devices/acpi/vmgenid.rs index 24220577ccf..13dd19902df 100644 --- a/src/vmm/src/devices/acpi/vmgenid.rs +++ b/src/vmm/src/devices/acpi/vmgenid.rs @@ -162,20 +162,20 @@ impl<'a> Persist<'a> for VmGenId { } impl Aml for VmGenId { - fn append_aml_bytes(&self, v: &mut Vec) { + fn append_aml_bytes(&self, v: &mut Vec) -> Result<(), aml::AmlError> { #[allow(clippy::cast_possible_truncation)] let addr_low = self.guest_address.0 as u32; let addr_high = (self.guest_address.0 >> 32) as u32; aml::Device::new( - "_SB_.VGEN".into(), + "_SB_.VGEN".try_into()?, vec![ - &aml::Name::new("_HID".into(), &"FCVMGID"), - &aml::Name::new("_CID".into(), &"VM_Gen_Counter"), - &aml::Name::new("_DDN".into(), &"VM_Gen_Counter"), + &aml::Name::new("_HID".try_into()?, &"FCVMGID")?, + &aml::Name::new("_CID".try_into()?, &"VM_Gen_Counter")?, + &aml::Name::new("_DDN".try_into()?, &"VM_Gen_Counter")?, &aml::Name::new( - "ADDR".into(), + "ADDR".try_into()?, &aml::Package::new(vec![&addr_low, &addr_high]), - ), + )?, ], ) .append_aml_bytes(v) From 8ea46907129bce9bc125654477a172c07917bba4 Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Wed, 23 Oct 2024 12:32:17 +0200 Subject: [PATCH 2/3] fix: naming of types in acpi-tables We were calling stuff "Cachable" instead of "Cacheable". Signed-off-by: Babis Chalios --- src/acpi-tables/src/aml.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/acpi-tables/src/aml.rs b/src/acpi-tables/src/aml.rs index 040bde4496f..c2b4e85451d 100644 --- a/src/acpi-tables/src/aml.rs +++ b/src/acpi-tables/src/aml.rs @@ -406,7 +406,7 @@ enum AddressSpaceType { } #[derive(Copy, Clone)] -pub enum AddressSpaceCachable { +pub enum AddressSpaceCacheable { NotCacheable, Cacheable, WriteCombining, @@ -421,7 +421,7 @@ pub struct AddressSpace { } impl AddressSpace { - pub fn new_memory(cacheable: AddressSpaceCachable, read_write: bool, min: T, max: T) -> Self { + pub fn new_memory(cacheable: AddressSpaceCacheable, read_write: bool, min: T, max: T) -> Self { AddressSpace { r#type: AddressSpaceType::Memory, min, @@ -1384,13 +1384,13 @@ mod tests { "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![ &AddressSpace::new_memory( - AddressSpaceCachable::Cacheable, + AddressSpaceCacheable::Cacheable, true, 0xa_0000u32, 0xb_ffffu32 ), &AddressSpace::new_memory( - AddressSpaceCachable::NotCacheable, + AddressSpaceCacheable::NotCacheable, true, 0xc000_0000u32, 0xfebf_ffffu32 @@ -1416,7 +1416,7 @@ mod tests { Name::new( "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![&AddressSpace::new_memory( - AddressSpaceCachable::Cacheable, + AddressSpaceCacheable::Cacheable, true, 0x8_0000_0000u64, 0xf_ffff_ffffu64 @@ -1984,7 +1984,7 @@ mod tests { &Name::new( "MR64".try_into().unwrap(), &ResourceTemplate::new(vec![&AddressSpace::new_memory( - AddressSpaceCachable::Cacheable, + AddressSpaceCacheable::Cacheable, true, 0x0000_0000_0000_0000u64, 0xFFFF_FFFF_FFFF_FFFEu64 From cda14424e8f8e9dc421712c92283e55f41616917 Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Wed, 23 Oct 2024 12:56:59 +0200 Subject: [PATCH 3/3] fix: potential underflow in AML bytecode creation acpi_tables::aml::AddressSpace type is describing an address space with a minimum address `min: T` and a maximum address `max: T`. Currently, we do not perform any checks on these values when creating the AddressSpace objects. This means that the starting address `min` can be bigger than the last address `max`, which can cause underflows when calculating the length of this address space, i.e. `max - min + 1`. Add a check in the constructor methods of AddressSpace objects and return an error when `min > max`. Signed-off-by: Babis Chalios --- src/acpi-tables/src/aml.rs | 59 +++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/src/acpi-tables/src/aml.rs b/src/acpi-tables/src/aml.rs index c2b4e85451d..8f839c258e0 100644 --- a/src/acpi-tables/src/aml.rs +++ b/src/acpi-tables/src/aml.rs @@ -14,6 +14,8 @@ pub enum AmlError { NameEmpty, /// Invalid name part length InvalidPartLength, + /// Invalid address range + AddressRange, } pub trait Aml { @@ -420,32 +422,49 @@ pub struct AddressSpace { type_flags: u8, } -impl AddressSpace { - pub fn new_memory(cacheable: AddressSpaceCacheable, read_write: bool, min: T, max: T) -> Self { - AddressSpace { +impl AddressSpace +where + T: PartialOrd, +{ + pub fn new_memory( + cacheable: AddressSpaceCacheable, + read_write: bool, + min: T, + max: T, + ) -> Result { + if min > max { + return Err(AmlError::AddressRange); + } + Ok(AddressSpace { r#type: AddressSpaceType::Memory, min, max, type_flags: (cacheable as u8) << 1 | u8::from(read_write), - } + }) } - pub fn new_io(min: T, max: T) -> Self { - AddressSpace { + pub fn new_io(min: T, max: T) -> Result { + if min > max { + return Err(AmlError::AddressRange); + } + Ok(AddressSpace { r#type: AddressSpaceType::Io, min, max, type_flags: 3, // EntireRange - } + }) } - pub fn new_bus_number(min: T, max: T) -> Self { - AddressSpace { + pub fn new_bus_number(min: T, max: T) -> Result { + if min > max { + return Err(AmlError::AddressRange); + } + Ok(AddressSpace { r#type: AddressSpaceType::BusNumber, min, max, type_flags: 0, - } + }) } fn push_header(&self, bytes: &mut Vec, descriptor: u8, length: usize) { @@ -1340,7 +1359,9 @@ mod tests { assert_eq!( Name::new( "_CRS".try_into().unwrap(), - &ResourceTemplate::new(vec![&AddressSpace::new_bus_number(0x0u16, 0xffu16),]) + &ResourceTemplate::new(vec![ + &AddressSpace::new_bus_number(0x0u16, 0xffu16).unwrap(), + ]) ) .unwrap() .to_aml_bytes() @@ -1360,8 +1381,8 @@ mod tests { Name::new( "_CRS".try_into().unwrap(), &ResourceTemplate::new(vec![ - &AddressSpace::new_io(0x0u16, 0xcf7u16), - &AddressSpace::new_io(0xd00u16, 0xffffu16), + &AddressSpace::new_io(0x0u16, 0xcf7u16).unwrap(), + &AddressSpace::new_io(0xd00u16, 0xffffu16).unwrap(), ]) ) .unwrap() @@ -1388,13 +1409,15 @@ mod tests { true, 0xa_0000u32, 0xb_ffffu32 - ), + ) + .unwrap(), &AddressSpace::new_memory( AddressSpaceCacheable::NotCacheable, true, 0xc000_0000u32, 0xfebf_ffffu32 - ), + ) + .unwrap(), ]) ) .unwrap() @@ -1420,7 +1443,8 @@ mod tests { true, 0x8_0000_0000u64, 0xf_ffff_ffffu64 - )]) + ) + .unwrap()]) ) .unwrap() .to_aml_bytes() @@ -1988,7 +2012,8 @@ mod tests { true, 0x0000_0000_0000_0000u64, 0xFFFF_FFFF_FFFF_FFFEu64 - )]) + ) + .unwrap()]) ) .unwrap(), &CreateField::::new(