Re: [PATCH 00/14] rust: example of bindings code for Rust in QEMU

[Pierrick Bouvier]

On 7/5/24 01:06, Paolo Bonzini wrote:
> On Thu, Jul 4, 2024 at 9:26 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> > > Patches 9-10 deal with how to define new subclasses in Rust.  They are
> > > a lot less polished and less ready.  There is probably a lot of polish
> > > that could be applied to make the code look nicer, but I guess there is
> > > always time to make the code look nicer until the details are sorted out.
> > >
> > > The things that I considered here are:
> > >
> > > - splitting configuration from runtime state
> > > - automatic generation of instance_init and instance_finalize
> > > - generation of C vtables from safe code that is written in Rust
> > > - generation of qdev property tables
> >
> > Shouldn't we focus more on how we want to write a QOM device in Rust
> > instead, by making abstraction of existing C implementation first?
> > Once we have satisfying idea, we could evaluate how it maps to existing
> > implementation, and which compromise should be made for efficiency.
> > It seems that the current approach is to stick to existing model, and
> > derive Rust bindings from this.
>
> I think I tried to do a little bit of that. For example, the split of
> configuration from runtime state is done to isolate the interior
> mutability, and the automatic generation of instance_init and
> instance_finalize relies on Rust traits like Default and Drop; the
> realize implementation returns a qemu::Result<()>; and so on.

Patches 9/10 have a concrete example of what a TestDevice look like, 
mixed with some glue definition. But definitely, this example code is 
what would be the most interesting to review for introducing Rust.

To give an example of questions we could have:

Why should we have distinct structs for a device, the object 
represented, and its state?

Properties could be returned by a specific function (properties()), 
called at some given point. Would that be a good approach?

Having to implement ObjectImpl and DeviceImpl looks like an 
implementation detail, that could be derived automatically from a 
device. What's wrong with calling a realize that does nothing in the end?

Could device state be serialized with something like serde?

This is the kind of things that could be discussed, on a reduced 
example, without specially looking at how to implement that concretely, 
in a first time.

> But there are many steps towards converting the PL011 device to Rust:
>
> - declarative definitions of bitfields and registers (done)
> - using RefCell for mutable state
> - using wrappers for class and property definition
> - defining and using wrappers for Chardev/CharBackend
> - defining and using wrappers for MemoryRegion callbacks
> - defining and using wrappers for SysBusDevice functionality
> - defining and using wrappers for VMState functionality
>
> At each step we need to design "how we want to do that in Rust" and
> all the things you mention above. This prototype covers the second and
> third steps, and it's already big enough! :)
>
> I expect that code like this one would be merged together with the
> corresponding changes to PL011: the glue code is added to the qemu/
> crate and used in the hw/core/pl011/ directory. This way, both authors
> and reviewers will have a clue of what becomes more readable/idiomatic
> in the resulting code.
>
> > I agree this glue can be a source of technical debt, but on the other
> > side, it should be easy to refactor it, if we decided first what the
> > "clean and idiomatic" Rust API should look like.
>
> That's true, especially if we want to add more macro magic on top. We
> can decide later where to draw the line between complexity of glue
> code and cleanliness of Rust code, and also move the line as we see
> fit.

Automatic derivation macros could be ok (to derive some trait or 
internal stuff), but usage of magic macros as syntactic sugar should 
indeed be thought twice.

Return a collection of QDevProperty could be better than having a magic 
@properties syntactic sugar.

Another thing that could be discussed is: do we want to have the whole 
inheritance mechanism for Rust devices? Is it really needed?

> On the other hand, if we believe that _this_ code is already too much,
> that's also a data point. Again: I don't think it is, but I want us to
> make an informed decision.

Between having a clean and simple Device definition, with a bit more of 
magic glue (hidden internally), and requires devices to do some magic 
initialization to satisfy existing architecture, what would be the best?

Even though it takes 1000 more lines, I would be in favor to have 
Devices that are as clean and simple as possible. Because this is the 
kind of code what will be the most added/modified, compared to the glue 
part.

> > A compromise where you have to manually translate some structs, or copy
> > memory to traverse languages borders at some point, could be worth the
> > safety and expressiveness.
>
> Yep, Error is an example of this. It's not the common case, so it's
> totally fine to convert to and from C Error* (which also includes
> copying the inner error string, from a String to malloc-ed char*) to
> keep the APIs nicer.

Yes, it's a good approach!

> > > We should have an idea of what this glue code looks like, in order to make
> > > an informed choice.  If we think we're not comfortable with reviewing it,
> > > well, we should be ready to say so and stick with C until we are.
> >
> > While it is important that this glue code is maintainable and looks
> > great, I don't think it should be the main reason to accept usage of a
> > new language.
>
> Not the main reason, but an important factor in judging if we are
> *able* to accept usage of a new language. Maybe it's just a formal
> step, but it feels safer to have _some_ code to show and to read, even
> if it's just a prototype that barely compiles.
>
> > We could have a specific trait with functions to handle various kind of
> > events. And the glue code could be responsible to translate this to
> > callbacks for the C side (and calling Rust code accordingly, eventually
> > serializing this on a single thread to avoid any race issues).
>
> That's a possibility, yes. Something like (totally random):
>
> impl CharBackend {
>      pub fn start<T: CharBackendCallbacks>(&mut self, obj: &T) {
>          qemu_chr_fe_set_handlers(self.0,
>                               Some(obj::can_receive),
> Some(obj::receive, obj::event),
>                               Some(obj::be_change), obj as *const _ as
> *const c_void,
>                               ptr::null(), true);
>      }
>      pub fn stop(&mut self) {
>          qemu_chr_fe_set_handlers(self.0, None, None,
>                               None, ptr::null(), ptr::null(), true);
>      }
> }

Or have multiple traits matching every possible operation, and allow a 
device to implement it or not, like Read/Write traits. And the glue code 
could call qemu_chr_fe_set_handlers.

> but I left for later because I focused just on the lowest levels code
> where you could have "one" design. For example, for memory regions
> some devices are going to have more than one, so there could be
> reasons to do callbacks in different ways. By the way, all of this
> passing of Rust references as opaque pointers needs, at least in
> theory, to be annotated for pinning. Do we have to make sure that
> pinning is handled correctly, or can we just assume that QOM objects
> are pinned? I am not sure I am ready to answer the question...

I hope my answer helped to understand more my point that discussing the 
interface is more important than discussing the glue needed.

> Paolo