Skip to content

Fix mutex references and the global lock - #322

Open
martin-hughes wants to merge 1 commit into
rust-osdev:mainfrom
martin-hughes:i305-global-lock
Open

Fix mutex references and the global lock#322
martin-hughes wants to merge 1 commit into
rust-osdev:mainfrom
martin-hughes:i305-global-lock

Conversation

@martin-hughes

Copy link
Copy Markdown
Contributor

This fixes #305 by correcting the mutex acquisition and release. It also allows mutexes to be acquired and released by reference, since otherwise it is not possible to acquire (or release) _GL.

@IsaacWoods the main question I had was about where to keep track of the reentrant acquisition count of global_lock_mutex. Clearly the Handler should not release the actual mutex until the count reaches zero, but we don't know what the count is.

There were two options that I could think of:

  1. Keep track of the count in Interpreter (which I chose), or
  2. Require Handler to return a value indicating whether the mutex was fully released or not.

Option 2 would require an API change, and also relies on the implementer to correctly deal with reentrancy. Whereas option 1 allows the crate to deal with an inadequately implemented Handler but if Handler is wrong then the actual mutex may come out of sync with the firmware lock.

I would be happy to switch to option 2 if you prefer, Isaac.

Allow mutexes to be acquired and released by reference. This then allows
the global reference `_GL` to be acquired and released.

Correct the semantics of the global lock so that it handles the firmware
lock correctly
@SnowCheetos

Copy link
Copy Markdown

Perhaps a syntax sugar closure like handler.with_lock(...) would a nice little addition to make life easier, smth like:

fn with_lock<R>(
    &self,
    mutex: Handle,
    timeout: u16,
    f: impl FnOnce() -> R,
) -> Result<R, AmlError> {
    self.acquire(mutex, timeout)?;

    struct Guard<'a, H: Handler> {
        handler: &'a H,
        mutex: Handle,
    }

    impl<H: Handler> Drop for Guard<'_, H> {
        fn drop(&mut self) {
            self.handler.release(self.mutex);
        }
    }

    let _guard = Guard {
        handler: self,
        mutex,
    };

    Ok(f())
}

Little helpers like this saved plenty of boot wedges for me

@martin-hughes

Copy link
Copy Markdown
Contributor Author

Do you mean to add to this PR? If so would you show me an example of it being used?

I can think why it might be generally useful but I'm not sure about needing it here

@IsaacWoods

Copy link
Copy Markdown
Member

Thanks for working on this Martin! I also have some _GL work locally that is partially orthogonal to this (I am wondering if we need to manually synchronise the hardware access between potential OS threads with a spinlock-type primitive too, plus moving some bits re the hardware locking into Facs) that I'll try to get in at some point. This seems like a strict improvement in the meantime, so thank you.

@IsaacWoods the main question I had was about where to keep track of the reentrant acquisition count of global_lock_mutex. Clearly the Handler should not release the actual mutex until the count reaches zero, but we don't know what the count is.

Yeah, we have to trust the Handler to be correct in some ways; I think either approach would be reasonable but I'm happy with keeping track of the count within Interpreter like this.

Comment thread src/aml/mod.rs
*/
self.handler.release(self.global_lock_mutex);
continue;
let last = self.global_lock_acquisition_count.fetch_add(1, Ordering::Relaxed);

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.

On second reading - are these orderings strong enough? This is an accursed area of knowledge for me, but I wonder if these should be Acquire and Release respectively? Other than that, very happy to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AML global lock acquisition of global_lock_mutex looks wrong

3 participants