Skip to content
Open
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
43 changes: 29 additions & 14 deletions src/aml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,33 +95,47 @@ macro_rules! extract_args {
};
}

/// `Interpreter` implements a virtual machine for the dynamic AML bytecode. It can be used by a
/// `BaseInterpreter` implements a virtual machine for the dynamic AML bytecode. It can be used by a
/// host operating system to load tables containing AML bytecode (generally the DSDT and SSDTs) and
/// will then manage the AML namespace and all objects created during the life of the system.
pub struct Interpreter<H>
///
/// [`Interpreter`] and [`NoSendInterpreter`] provide sensible partial specialisations of
/// `BaseInterpreter` for the multi-threaded and single-threaded use cases.
pub struct BaseInterpreter<H, R>
where
H: Handler,
R: RegionHandler + ?Sized,
{
handler: H,
pub namespace: Spinlock<Namespace>,
pub object_token: Spinlock<ObjectToken>,
integer_size: IntegerSize,
region_handlers: Spinlock<BTreeMap<RegionSpace, Box<dyn RegionHandler>>>,
region_handlers: Spinlock<BTreeMap<RegionSpace, Box<R>>>,

global_lock_mutex: Handle,
registers: Arc<FixedRegisters<H>>,
facs: Option<PhysicalMapping<H, Facs>>,
}

/// An Interpreter that does not enforce a requirement to be Send. This might be useful for
/// uni-processor kernels, or operating systems where this crate runs in a single-threaded user-mode
/// process.
pub type NoSendInterpreter<H> = BaseInterpreter<H, dyn RegionHandler>;

/// An Interpreter that is known to be Send + Sync. The version you should prefer by default.
pub type Interpreter<H> = BaseInterpreter<H, dyn RegionHandler + Send + Sync>;

// TODO: Make sure to remove these two lines after Interpreter really is Send + Sync.
unsafe impl<H> Send for Interpreter<H> where H: Handler + Send {}
unsafe impl<H> Sync for Interpreter<H> where H: Handler + Send {}

/// The value returned by the `Revision` opcode.
const INTERPRETER_REVISION: u64 = 1;

impl<H> Interpreter<H>
impl<H, R> BaseInterpreter<H, R>
where
H: Handler,
R: RegionHandler + ?Sized,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm I don't think I've ever used this pattern myself where you constrain a generic to be a dyn Trait in an aliasing type like this - does this constrain R to being a single type (i.e. all region handlers in the map have to be of the same type), or does it mean R represents a trait object like before?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=f484a5f3198489d5b17de891509b2874

Unfortunately looks like this is the former. The Box does need to contain some kind of trait object to push it into being a fat pointer with a vtable etc.

Overall, I don't think it's an unreasonable requirement for RegionHandlers to be Send+Sync. I wonder if we should just constrain all of them to be?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's odd... The attached shows it working for me at least. Mind copying this to tests and seeing if it's just something weird in my config? region_handlers.rs.txt (drop the .txt of course)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh - on line 18 you mean Baz instead of Bar, plus you need a ?Sized constraint on line 5.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Overall, I don't think it's an unreasonable requirement for RegionHandlers to be Send+Sync. I wonder if we should just constrain all of them to be?

I did wonder about this quite a bit... I think there's a counter-argument of two parts

  1. A micro-kernel system would (presumably) run acpi in a separate process. In which case it might well be single-threaded.
  2. If it is single-threaded, then it may be useful to use non-send types like Rc in a RegionHandler, and since there's a low-cost way for us to enable that, it's worth us enabling it.

But as always I'm open to your thoughts!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh whoops, indeed I do. Updated gist to prove this does work:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=f2f4189ac1278ea8187b34f275874feb

A micro-kernel system would (presumably) run acpi in a separate process. In which case it might well be single-threaded.

Hm, yeah, genuinely unsure re my thoughts here. I'll have a think but I don't think having an option of a single-threaded interpreter is a bad one, as long as maintenance burden is not significantly worsened (which I don't think this PR does).

{
/// Construct a new [`Interpreter`]. This does not load any tables - if you have an
/// [`crate::AcpiTables`] already, construct an [`AcpiPlatform`] first and then use
Expand All @@ -131,12 +145,12 @@ where
dsdt_revision: u8,
registers: Arc<FixedRegisters<H>>,
facs: Option<PhysicalMapping<H, Facs>>,
) -> Interpreter<H> {
) -> Self {
info!("Initializing AML interpreter v{}", env!("CARGO_PKG_VERSION"));

let global_lock_mutex = handler.create_mutex();

Interpreter {
BaseInterpreter {
handler,
namespace: Spinlock::new(Namespace::new(global_lock_mutex)),
object_token: Spinlock::new(unsafe { ObjectToken::create_interpreter_token() }),
Expand All @@ -149,8 +163,12 @@ where
}

/// Construct a new [`Interpreter`] with the given [`AcpiPlatform`].
pub fn new_from_platform(platform: &AcpiPlatform<H>) -> Result<Interpreter<H>, AcpiError> {
fn load_table(interpreter: &Interpreter<impl Handler>, table: AmlTable) -> Result<(), AcpiError> {
pub fn new_from_platform(platform: &AcpiPlatform<H>) -> Result<BaseInterpreter<H, R>, AcpiError> {
fn load_table<H, R>(interpreter: &BaseInterpreter<H, R>, table: AmlTable) -> Result<(), AcpiError>
where
H: Handler,
R: RegionHandler + ?Sized,
{
let mapping = unsafe {
interpreter.handler.map_physical_region::<SdtHeader>(table.phys_address, table.length as usize)
};
Expand All @@ -174,7 +192,7 @@ where
};

let dsdt = platform.tables.dsdt()?;
let interpreter = Interpreter::new(platform.handler.clone(), dsdt.revision, registers, facs);
let interpreter = BaseInterpreter::new(platform.handler.clone(), dsdt.revision, registers, facs);

if let Err(err) = load_table(&interpreter, dsdt) {
error!("Error while loading DSDT: {:?}. Continuing; this may cause downstream errors.", err);
Expand Down Expand Up @@ -233,13 +251,10 @@ where
}
}

pub fn install_region_handler<RH>(&self, space: RegionSpace, handler: RH)
where
RH: RegionHandler + 'static,
{
pub fn install_region_handler(&self, space: RegionSpace, handler: Box<R>) {
let mut handlers = self.region_handlers.lock();
assert!(handlers.get(&space).is_none(), "Tried to install handler for same space twice!");
handlers.insert(space, Box::new(handler));
handlers.insert(space, handler);
}

/// Initialize the namespace - this should be called after all tables have been loaded and
Expand Down
Loading