On Wed, Jun 12, 2024 at 4:42 PM Manos Pitsidianakis
<manos.pitsidiana...@linaro.org> wrote:
> There was consensus in the community call that we won't be writing Rust
> APIs for internal C QEMU interfaces; or at least, that's not the goal
I disagree with that. We need _some_ kind of bindings, otherwise we
have too much unsafe code, and the benefit of Rust becomes so much
lower that I doubt the utility.
If something is used by only one device then fine, but when some kind
of unsafe code repeats across most if not all devices, that is a
problem. It can be macros, it can be smart pointers, that remains to
be seen---but repetition should be a warning signal that _something_
is necessary.
> >For definining new classes I think it's okay if Rust does not support
> >writing superclasses yet, only leaves.
> >
> >I would make a QOM class written in Rust a struct that only contains
> >the new fields. The struct must implement Default and possibly Drop
> >(for finalize).
>
> The object is allocated and freed from C, hence it is not Dropped. We're
> only ever accessing it from a reference retrieved from a QEMU provided
> raw pointer. If the struct gains heap object fields like Box or Vec or
> String, they'd have to be dropped manually on _unrealize.
That's my point, if you have
struct MyDevice_Inner {
data: Vec<u8>,
}
struct MyDevice {
parent_obj: qemu::bindings::SysBusDevice,
private: ManuallyDrop<PL011_Inner>,
}
then the instance_finalize method can simply do
pub instance_finalize(self: *c_void)
{
let dev = self as *mut MyDevice;
unsafe { ManuallyDrop::drop(dev.private) }
}
Don't do it on _unrealize, create macros that do it for you.
> >and then a macro defines a wrapper struct that includes just two
> >fields, one for the superclass and one for the Rust struct.
> >instance_init can initialize the latter with Default::default().
> >
> > struct PL011 {
> > parent_obj: qemu::bindings::SysBusDevice,
> > private: PL011_Inner,
> > }
>
> a nested struct is not necessary for using the Default trait
Agreed, but a nested struct is nice anyway in my opinion as a boundary
between the C-ish and Rust idiomatic code.
> >"private" probably should be RefCell<PL011_Inner>, avoiding the unsafe
> >
> > state.as_mut().read(addr, size)
>
>
> RefCell etc are not FFI safe.
Why does it matter? Everything after the SysBusDevice is private.
> Also, nested fields must be visible so that the offset_of! macro works, for
> QOM properties.
Note that QOM properties do not use offset_of; qdev properties do.
Using qdev properties is much easier because they hide visitors, but
again - not necessary, sometimes going lower-level can be easier if
the API you wrap is less C-ish.
Also, you can define constants (including properties) in contexts
where non-public fields are visible:
use std::mem;
pub struct Foo {
_x: i32,
y: i32,
}
impl Foo {
pub const OFFSET_Y: usize = mem::offset_of!(Foo, y);
}
fn main() {
println!("{}", Foo::OFFSET_Y);
}
Any offset needed to go past the SysBusDevice and any other fields
before MyDevice_Inner can be added via macros. Also note that it
doesn't _have_ to be RefCell; RefCell isn't particularly magic. We can
implement our own interior mutability thingy that is more friendly to
qdev properties, or that includes the ManuallyDrop<> thing from above,
or both.
For example you could have
type PL011 = QOMImpl<qemu::bindings::SysBusDevice, PL011_Inner>;
and all the magic (for example Borrow<PL011_Inner>, the TypeInfo, the
instance_init and instance_finalize function) would be in QOMImpl.
My point is: let's not focus on having a C-like API. It's the easiest
thing to do but not the target.
> Finally,
>
> state.as_mut().read(addr, size)
>
> Is safe since we receive a valid pointer from QEMU. This fact cannot be
> derived by the compiler, which is why it has an `unsafe` keyword. That
> does not mean that the use here is unsafe.
Yes, it is safe otherwise it would be undefined behavior, but there
are no checks of the kind that you have in Rust whenever you have
&mut.
state.as_mut() implies that no other references to state are in use;
but there are (you pass it as the opaque value to both the
MemoryRegionOps and the chardev frontend callbacks). This is why I
think something like RefCell is needed to go from a shared reference
to an exclusive one (interior mutability).
> >There should also be macros to define the wrappers for MMIO MemoryRegions.
>
> Do you mean the MemoryRegionOps?
Yes.
> I wanted to focus on the build system integration for the first RFC
> which is why there are some macros but not in every place it makes
> sense.
Yes, absolutely. We need to start somewhere.
Paolo
