qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH] rust: add bindings for interrupt sources + interior muta


From: Paolo Bonzini
Subject: Re: [RFC PATCH] rust: add bindings for interrupt sources + interior mutability discussion
Date: Mon, 11 Nov 2024 11:09:37 +0100

On Fri, Nov 8, 2024 at 10:16 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
> I like the idea of modeling device struct fields as interior-mutable.
> That reflects how such fields are referenced and can help forbid invalid
> accesses early.
>
> To showcase the benefit of modeling interior-mutable fields, a more
> complex device example (probably involving coroutines or other async
> mechanisms) may be needed.

I don't think there's really a benefit, it's more like avoiding a lie.
Even though right now (or in general) it's extremely unlikely that
this causes miscompilation, Rust's expectation is that a &mut is not
aliased except if it is formed from a shared reference via
UnsafeCell<>. Right now that's violated in both realize and the
Chardev callbacks.

> To me a container that check locking rules sound more like Lock, not
> Cell. Name it as a Cell can be misleading to those being used to Rust
> cells.

Right, it still performs the function and has the API of a Cell but
I've settled on BqlCell and BqlRefCell for clarity.

Especially if you have multiple imports of std::cell::Cell and QEMU's
own BQL-based (lowercase) cell, you don't want them to have the same
name.

> That said, one of its primary differences from the standard locking
> types in Rust is that the lock involved is global instead of being bound
> to a specific object. There are two alternative APIs in my mind:
>
> 1. get(&self) -> T / set(&self, T) which internally checks if BQL is
> held by the current thread, and panics if it is not. This is more
> straightforward.
>
> 2. get(&self, _: &BqlLockGuard) -> T / set(&self, _: T,
> _: &BqlLockGuard) which takes an extra evidence of BQL being
> held. BqlLockGuard can only be got as an argument to BQL-protected
> callbacks (macro-generated glue code here) or Bql::lock() which calls
> bql_lock().
>
> The second approach looks more idiomatic to me and may allow the
> compiler to error (via analyzing lifetimes) on derefs of
> borrow()/borrow_mut()-returned refs after BQL is unlocked (I need a
> double check on this). However, temporarily unlocking BQL is extremely
> rare in devices. I'm not sure if that's worthwhile at the cost of making
> the APIs more tedious to use.

Yes, this would be better in terms of compile-time safety, but on the
other hand the guard would propagate to all APIs that require the BQL.
The nice thing about the cell approach is that, as far as most devices
are concerned, QEMU has a single program flow (though not a single
thread) with very well defined preemption points but with an abundance
of reentrancy. If your callbacks get a BQL guard from the outside,
it's true that checks move from run-time to compile-time, on the other
hand you still need a RefCell-like mechanism to protect from
unexpected reentrancy. I'd rather avoid types like Bql<RefCell<T>>,
they are verbose in the definitions and the usage because they require
both a guard and explicit borrowing.

However, there are two points that are related to your observation:

1) when BqlRefCell is added, the C code needs to abort if the BQL is
released while a BqlRefCell borrow (either shared or mutable) is
alive. That's because you can never be sure that C code won't come in
and mutate the data that is protected by BqlRefCell.

2) We probably want a custom guard object sooner or later, maybe for
the block layer to assert that you're in the right AioContext, but for
now the risk of lock-related runtime failures of BqlRefCell seems
small to me (especially compared to already-borrowed panics due to
Rust->C->Rust reentrancy).


There is another issue that is solved by custom cell types compared to
RefCell, by the way, and it's that you cannot do offset_of! of
something within a RefCell. However we can make the BqlRefCell start
with the inner type:

#[repr(C)]
pub struct BqlRefCell<T> {
    inner: T,
    ...
}

and then use offset_of!(DeviceStruct, cell) + offset_of!(DeviceRegs,
reg_name). This however is very much left for later, as there are more
pressing issues and I think no one has put much thought into VMState
yet.

Thanks for sharing. I'll post a version of the BqlCell sometime within
the next week or two.

Paolo




reply via email to

[Prev in Thread] Current Thread [Next in Thread]