On 7/5/24 01:06, Paolo Bonzini wrote:
On Thu, Jul 4, 2024 at 9:26 PM Pierrick Bouvier
<pierrick.bouv...@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
