Interpreter<H> isn't safely Send or Sync because of `dyn RegionHandler`

[martin-hughes]

The lines

unsafe impl<H> Send for Interpreter<H> where H: Handler + Send {}
unsafe impl<H> Sync for Interpreter<H> where H: Handler + Send {}

aren't true because dyn RegionHandler might not be Send/Sync.

I can see that facs is safe to declare Send or Sync because only the global_lock field is actually used.

I'm sure there's some generic-types magic to resolve this. Alternatively it might be desirable to constrain the RegionHandlers to be Send + Sync, but that'd technically be a breaking API change.

[IsaacWoods]

This is true - region handlers at the moment are basically at the stage of me sketching them out in the initial rewrite to say that they should exist. The signature will need to change: read and write will need to take an address in the region the IO is happening at, and likely will need to take an &Interpreter to be able to e.g. find a PCI address (unless we want to say region handlers should be storing an Arc<Interpreter> or something behind the scenes (but that pushes users in a direction they may not want to take)).

Overall, RegionHandler should require types to also be Send and I think most likely Sync

[sophymarine]

Constraining RegionHandler to Send + Sync is the cleanest path here, even if it forces a minor or major version bump. If you try to hand-roll unsafe impl blocks with dyn RegionHandler inside, any multi-threaded ACPI event dispatch will eventually trigger undefined behaviour when a non-Send handler gets sent across thread boundaries. In similar kernel-adjacent Rust crates, we usually define a wrapper trait like pub trait SendSyncRegionHandler: RegionHandler + Send + Sync {} and implement it blanket-wise. This keeps the public interface readable while explicitly enforcing the safety guarantees that Interpreter<H> already claims to provide.

[IsaacWoods]

Yep that would likely be best.

Could you elaborate on your reasoning for having a separate SendSyncRegionHandler with a blanket impl vs constraining the trait directly (i.e. trait RegionHandler: Send + Sync)?

[martin-hughes]

My naive first reaction was that most users would need a Send + Sync interpreter so it can be used in multiple threads - perhaps both an IRQ slow-path and a separate device initialisation or management task.

But having thought about it more, perhaps there's a use case for a non Send + Sync interpreter in, say, a microkernel where all ACPI ops are in their own single-threaded process?

In which case both RegionHandler and SendSyncRegionHandler would have their use-cases in Interpreter and SendSyncInterpreter respectively. Although being a monolithic kernel heathen I'd prefer the naming scheme NonSendRegionHandler and RegionHandler 😉