Resolve names used as package elements lazily

src/aml/mod.rs

@@ -119,6 +119,11 @@ unsafe impl<H> Sync for Interpreter<H> where H: Handler + Send {}
 /// The value returned by the `Revision` opcode.
 const INTERPRETER_REVISION: u64 = 1;
 
+/// The maximum number of times a name used as a package element may resolve to another such name
+/// before we give up. Real firmware doesn't chain these at all, but broken tables must not be able
+/// to make us loop forever.
+const MAX_NAME_PATH_INDIRECTIONS: usize = 8;
+
 impl<H> Interpreter<H>
 where
     H: Handler,
@@ -839,18 +844,7 @@ where
                     }
                     Opcode::DerefOf => {
                         extract_args!(op => [Argument::Object(object)]);
-                        let object = object.clone().unwrap_reference();
-                        let result = match &*object {
-                            Object::BufferField { .. } => object.read_buffer_field(self.integer_size)?.wrap(),
-                            Object::FieldUnit(field) => self.do_field_read(field)?,
-                            Object::String(path) => {
-                                let path = AmlName::from_str(path)?;
-                                let (_, object) = self.namespace.lock().search(&path, &context.current_scope)?;
-                                object.clone()
-                            }
-                            Object::Reference { .. } => unreachable!(),
-                            _ => object,
-                        };
+                        let result = self.do_deref_of(object.clone(), &context.current_scope)?;
                         context.contribute_arg(Argument::Object(result));
                         context.retire_op(op);
                     }
@@ -967,38 +961,8 @@ where
                     }
                     Opcode::ObjectType => {
                         extract_args!(op => [Argument::Object(object)]);
-
-                        // TODO: this should technically support scopes as well - this is less easy
-                        // (they should return `0`)
-                        fn object_type(object: &Object) -> u64 {
-                            if let Object::Reference { kind: _, inner } = object {
-                                object_type(&inner)
-                            } else {
-                                match object.typ() {
-                                    ObjectType::Uninitialized => 0,
-                                    ObjectType::Integer => 1,
-                                    ObjectType::String => 2,
-                                    ObjectType::Buffer => 3,
-                                    ObjectType::Package => 4,
-                                    ObjectType::FieldUnit => 5,
-                                    ObjectType::Device => 6,
-                                    ObjectType::Event => 7,
-                                    ObjectType::Method => 8,
-                                    ObjectType::Mutex => 9,
-                                    ObjectType::OpRegion => 10,
-                                    ObjectType::PowerResource => 11,
-                                    ObjectType::Processor => 12,
-                                    ObjectType::ThermalZone => 13,
-                                    ObjectType::BufferField => 14,
-                                    // XXX: 15 is reserved
-                                    ObjectType::Debug => 16,
-                                    ObjectType::RawDataBuffer => 17,
-                                    ObjectType::Reference => unreachable!(),
-                                }
-                            }
-                        }
-
-                        context.contribute_arg(Argument::Object(Object::Integer(object_type(&object)).wrap()));
+                        let object_type = self.object_type(object.clone())?;
+                        context.contribute_arg(Argument::Object(Object::Integer(object_type).wrap()));
                         context.retire_op(op);
                     }
                     Opcode::SizeOf => self.do_size_of(&mut context, op)?,
@@ -1630,7 +1594,16 @@ where
                             }
                         }
                         ResolveBehaviour::AsPackageElements => {
-                            context.contribute_arg(Argument::Object(Object::String(name.to_string()).wrap()));
+                            /*
+                             * NameStrings in packages can refer to objects that are defined later,
+                             * including ones in another table. Keep the name and the scope it
+                             * appeared in, so resolution doesn't depend on table load order, or on
+                             * the scope the package is eventually used from.
+                             */
+                            let name_path = Object::NamePath { name, scope: context.current_scope.clone() }.wrap();
+                            context.contribute_arg(Argument::Object(
+                                Object::Reference { kind: ReferenceKind::RefOf, inner: name_path }.wrap(),
+                            ));
                         }
                         ResolveBehaviour::Placeholder => {
                             panic!("Invalid resolve behaviour for name to be resolved!")
@@ -2157,6 +2130,8 @@ where
                 Object::Method { .. } | Object::NativeMethod { .. } => "[Control Method]".to_string(),
                 Object::Mutex { .. } => "[Mutex]".to_string(),
                 Object::Reference { inner, .. } => resolve_as_string(&(inner.clone().unwrap_reference())),
+                // We can't resolve the name here, as we don't have access to the namespace
+                Object::NamePath { name, .. } => name.to_string(),
                 Object::OpRegion(_) => "[Operation Region]".to_string(),
                 Object::Package(_) => "[Package]".to_string(),
                 Object::PowerResource { .. } => "[Power Resource]".to_string(),
@@ -2236,7 +2211,7 @@ where
 
     fn do_size_of(&self, context: &mut MethodContext, op: OpInFlight) -> Result<(), AmlError> {
         extract_args!(op => [Argument::Object(object)]);
-        let object = object.clone().unwrap_reference();
+        let object = self.resolve_name_path(object.clone())?;
 
         let result = match *object {
             Object::Buffer(ref buffer) => buffer.len(),
@@ -2300,6 +2275,71 @@ where
         Ok(())
     }
 
+    /// Resolve an object to the value it refers to, looking up `NamePath`s (i.e. names that were
+    /// used as package elements) in the namespace. Objects that aren't references are returned
+    /// unchanged.
+    fn resolve_name_path(&self, object: WrappedObject) -> Result<WrappedObject, AmlError> {
+        let mut object = object.unwrap_reference();
+
+        /*
+         * A name can resolve to another unresolved name, and firmware is free to make that
+         * circular. Bound the number of indirections we follow rather than recursing.
+         */
+        for _ in 0..MAX_NAME_PATH_INDIRECTIONS {
+            let (name, scope) = match *object {
+                Object::NamePath { ref name, ref scope } => (name.clone(), scope.clone()),
+                _ => return Ok(object),
+            };
+            let (_, resolved) = self.namespace.lock().search(&name, &scope)?;
+            object = resolved.unwrap_reference();
+        }
+
+        Err(AmlError::NameResolutionLoop)
+    }
+
+    fn object_type(&self, object: WrappedObject) -> Result<u64, AmlError> {
+        let object = self.resolve_name_path(object)?;
+
+        // TODO: this should technically support scopes as well - this is less easy
+        // (they should return `0`)
+        Ok(match object.typ() {
+            ObjectType::Uninitialized => 0,
+            ObjectType::Integer => 1,
+            ObjectType::String => 2,
+            ObjectType::Buffer => 3,
+            ObjectType::Package => 4,
+            ObjectType::FieldUnit => 5,
+            ObjectType::Device => 6,
+            ObjectType::Event => 7,
+            ObjectType::Method => 8,
+            ObjectType::Mutex => 9,
+            ObjectType::OpRegion => 10,
+            ObjectType::PowerResource => 11,
+            ObjectType::Processor => 12,
+            ObjectType::ThermalZone => 13,
+            ObjectType::BufferField => 14,
+            // XXX: 15 is reserved
+            ObjectType::Debug => 16,
+            ObjectType::RawDataBuffer => 17,
+            ObjectType::Reference => unreachable!(),
+        })
+    }
+
+    fn do_deref_of(&self, object: WrappedObject, current_scope: &AmlName) -> Result<WrappedObject, AmlError> {
+        let object = self.resolve_name_path(object)?;
+        match &*object {
+            Object::BufferField { .. } => Ok(object.read_buffer_field(self.integer_size)?.wrap()),
+            Object::FieldUnit(field) => self.do_field_read(field),
+            Object::String(path) => {
+                let path = AmlName::from_str(path)?;
+                let (_, object) = self.namespace.lock().search(&path, current_scope)?;
+                Ok(object.clone())
+            }
+            Object::Reference { .. } => unreachable!(),
+            _ => Ok(object),
+        }
+    }
+
     /// Perform a store of `object` into `target`, matching the expected behaviour of `DefStore`,
     /// which depends on the target:
     ///    - Locals are overwritten, unless they contain a reference, in which case a store is
@@ -2836,9 +2876,11 @@ enum ResolveBehaviour {
     /// `SuperName`, but can also be `NullName`
     Target,
     /// Surrogate argument, used by `DefPackage` and `DefVarPackage`. Only one of these is emitted,
-    /// but represents parsing of potentially many package elements. Names in packages should be
-    /// resolved into `String` objects - this is not well defined by the specification, but matches
-    /// expected behaviour of other interpreters.
+    /// but represents parsing of potentially many package elements. Names in packages refer to the
+    /// named object (see `Package` in the ASL reference, §19.6.101 of ACPI 6.5). We keep the name
+    /// and the scope it appeared in, and only resolve it when the reference is used, as the object
+    /// may not exist yet. ACPICA resolves these at package creation, but that makes resolution
+    /// depend on the order tables are loaded in.
     AsPackageElements,
     /// Used with [`OpInFlight::new_with`] to represent arguments that have already been resolved
     /// when an operation enters flight.
@@ -3410,6 +3452,11 @@ pub enum AmlError {
 
     InvalidNameSeg([u8; 4]),
     InvalidNormalizedName(AmlName),
+    /// An absolute name was required, but a relative one was supplied.
+    NameNotAbsolute(AmlName),
+    /// A name in a package resolved to another unresolved name too many times, probably because
+    /// the names form a cycle.
+    NameResolutionLoop,
     RootHasNoParent,
     EmptyNamesAreInvalid,
     LevelDoesNotExist(AmlName),

src/aml/namespace.rs

@@ -137,8 +137,7 @@ impl Namespace {
     }
 
     pub fn add_level(&mut self, path: AmlName, kind: NamespaceLevelKind) -> Result<(), AmlError> {
-        assert!(path.is_absolute());
-        let path = path.normalize()?;
+        let path = path.normalize_absolute()?;
 
         // Don't try to recreate the root scope
         if path != AmlName::root() {
@@ -155,8 +154,7 @@ impl Namespace {
     }
 
     pub fn remove_level(&mut self, path: AmlName) -> Result<(), AmlError> {
-        assert!(path.is_absolute());
-        let path = path.normalize()?;
+        let path = path.normalize_absolute()?;
 
         // Don't try to remove the root scope
         // TODO: we probably shouldn't be able to remove the pre-defined scopes either?
@@ -169,8 +167,7 @@ impl Namespace {
     }
 
     pub fn insert(&mut self, path: AmlName, object: WrappedObject) -> Result<(), AmlError> {
-        assert!(path.is_absolute());
-        let path = path.normalize()?;
+        let path = path.normalize_absolute()?;
 
         let (level, last_seg) = self.get_level_for_path_mut(&path)?;
         match level.values.insert(last_seg, (ObjectFlags::new(false), object)) {
@@ -187,8 +184,7 @@ impl Namespace {
     }
 
     pub fn create_alias(&mut self, path: AmlName, object: WrappedObject) -> Result<(), AmlError> {
-        assert!(path.is_absolute());
-        let path = path.normalize()?;
+        let path = path.normalize_absolute()?;
 
         let (level, last_seg) = self.get_level_for_path_mut(&path)?;
         match level.values.insert(last_seg, (ObjectFlags::new(true), object)) {
@@ -198,8 +194,7 @@ impl Namespace {
     }
 
     pub fn get(&mut self, path: AmlName) -> Result<WrappedObject, AmlError> {
-        assert!(path.is_absolute());
-        let path = path.normalize()?;
+        let path = path.normalize_absolute()?;
 
         let (level, last_seg) = self.get_level_for_path_mut(&path)?;
         match level.values.get(&last_seg) {
@@ -210,16 +205,17 @@ impl Namespace {
 
     /// Search for an object at the given path of the namespace, applying the search rules described in §5.3 of the
     /// ACPI specification, if they are applicable. Returns the resolved name, and the handle of the first valid
-    /// object, if found.
+    /// object, if found. Errors if `starting_scope` is not absolute.
     pub fn search(&self, path: &AmlName, starting_scope: &AmlName) -> Result<(AmlName, WrappedObject), AmlError> {
+        starting_scope.require_absolute()?;
+
         if path.search_rules_apply() {
             /*
              * If search rules apply, we need to recursively look through the namespace. If the
              * given name does not occur in the current scope, we look at the parent scope, until
              * we either find the name, or reach the root of the namespace.
              */
             let mut scope = starting_scope.clone();
-            assert!(scope.is_absolute());
             loop {
                 // Search for the name at this namespace level. If we find it, we're done.
                 let name = path.resolve(&scope)?;
@@ -254,9 +250,10 @@ impl Namespace {
     }
 
     pub fn search_for_level(&self, level_name: &AmlName, starting_scope: &AmlName) -> Result<AmlName, AmlError> {
+        starting_scope.require_absolute()?;
+
         if level_name.search_rules_apply() {
             let mut scope = starting_scope.clone().normalize()?;
-            assert!(scope.is_absolute());
 
             loop {
                 let name = level_name.resolve(&scope)?;
@@ -554,10 +551,24 @@ impl AmlName {
         }
     }
 
+    /// Returns `AmlError::NameNotAbsolute` if this path is not absolute. Relative paths can reach
+    /// entry points that require absolute ones from firmware input, so this is a normal error
+    /// rather than a panic.
+    pub fn require_absolute(&self) -> Result<(), AmlError> {
+        if self.is_absolute() { Ok(()) } else { Err(AmlError::NameNotAbsolute(self.clone())) }
+    }
+
+    /// Normalize an AML path that is required to be absolute. Prefer this over `normalize` where
+    /// an absolute path is required.
+    pub fn normalize_absolute(self) -> Result<AmlName, AmlError> {
+        self.require_absolute()?;
+        self.normalize()
+    }
+
     /// Resolve this path against a given scope, making it absolute. If the path is absolute, it is
-    /// returned directly. The path is also normalized.
+    /// returned directly. The path is also normalized. Errors if `scope` is not absolute.
     pub fn resolve(&self, scope: &AmlName) -> Result<AmlName, AmlError> {
-        assert!(scope.is_absolute());
+        scope.require_absolute()?;
 
         if self.is_absolute() {
             return Ok(self.clone());

src/aml/object.rs

@@ -1,4 +1,4 @@
-use crate::aml::{AmlError, Handle, IntegerSize, Operation, op_region::OpRegion};
+use crate::aml::{AmlError, Handle, IntegerSize, Operation, namespace::AmlName, op_region::OpRegion};
 use alloc::{
     borrow::Cow,
     string::{String, ToString},
@@ -23,6 +23,7 @@ pub enum Object {
     NativeMethod { f: Arc<NativeMethod>, flags: MethodFlags },
     Mutex { mutex: Handle, sync_level: u8 },
     Reference { kind: ReferenceKind, inner: WrappedObject },
+    NamePath { name: AmlName, scope: AmlName },
     OpRegion(OpRegion),
     Package(Vec<WrappedObject>),
     PowerResource { system_level: u8, resource_order: u16 },
@@ -62,6 +63,7 @@ impl fmt::Display for Object {
             Object::NativeMethod { .. } => write!(f, "NativeMethod"),
             Object::Mutex { .. } => write!(f, "Mutex"),
             Object::Reference { kind, inner } => write!(f, "Reference({:?} -> {})", kind, **inner),
+            Object::NamePath { name, scope } => write!(f, "NamePath({name} from {scope})"),
             Object::OpRegion(region) => write!(f, "{region:?}"),
             Object::Package(elements) => {
                 write!(f, "Package {{ ")?;
@@ -354,9 +356,9 @@ impl Object {
             Object::NativeMethod { .. } => ObjectType::Method,
             Object::Mutex { .. } => ObjectType::Mutex,
             // Object::Reference { inner, .. } => inner.typ(),
-            Object::Reference { .. } => ObjectType::Reference, // TODO: maybe this should
-            // differentiate internal/real
-            // references?
+            // TODO: maybe this should differentiate internal/real references?
+            // An unresolved package name also behaves as a reference until dereferenced.
+            Object::Reference { .. } | Object::NamePath { .. } => ObjectType::Reference,
             Object::OpRegion(_) => ObjectType::OpRegion,
             Object::Package(_) => ObjectType::Package,
             Object::PowerResource { .. } => ObjectType::PowerResource,