Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Hey Zhao,
On Thu, 11 Jul 2024 at 07:05, Zhao Liu wrote:
>
> Hi Manos and all,
>
> On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
> > diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml
> > new file mode 100644
> > index 00..ebecb99fe0
> > --- /dev/null
> > +++ b/rust/rustfmt.toml
> > @@ -0,0 +1,7 @@
> > +edition = "2021"
> > +format_generated_files = false
> > +format_code_in_doc_comments = true
> > +format_strings = true
> > +imports_granularity = "Crate"
> > +group_imports = "StdExternalCrate"
> > +wrap_comments = true
> >
>
> I find it's stiil necessary to strictly limit the width of the lines by
> "error_on_line_overflow = true" [1].
>
> Currently rustfmt defaults the width limitation with "max_width = 100",
> but it has bugs and doesn't always work. For example, the line of
> rust/qemu-api/src/device_class.rs:108 comes to 157 characters and is
> ignored by rustfmt, which doesn't even fit in one line of my screen!
>
> Of course I think it's feasible to manually review and fix similar cases,
> but it's definitely better to have readily available tool that can help
> us rigorously formatted...
>
> [1]:
> https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#error_on_line_overflow
>
> -Zhao
Thank you for pointing that out, I hadn't noticed it! The problem is
that macro definitions are also macros and macros aren't formatted
because they have to be expanded, IIUC. I agree that a hard error on
an overflow is necessary for readable code.
By the way, my usual go-to workaround for this bug is to format the
macro body outside the macro_rules! { } scope, (rustfmt works even if
the code does not compile) and then put it back in.
Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Hi Manos and all, On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote: > diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml > new file mode 100644 > index 00..ebecb99fe0 > --- /dev/null > +++ b/rust/rustfmt.toml > @@ -0,0 +1,7 @@ > +edition = "2021" > +format_generated_files = false > +format_code_in_doc_comments = true > +format_strings = true > +imports_granularity = "Crate" > +group_imports = "StdExternalCrate" > +wrap_comments = true > I find it's stiil necessary to strictly limit the width of the lines by "error_on_line_overflow = true" [1]. Currently rustfmt defaults the width limitation with "max_width = 100", but it has bugs and doesn't always work. For example, the line of rust/qemu-api/src/device_class.rs:108 comes to 157 characters and is ignored by rustfmt, which doesn't even fit in one line of my screen! Of course I think it's feasible to manually review and fix similar cases, but it's definitely better to have readily available tool that can help us rigorously formatted... [1]: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#error_on_line_overflow -Zhao
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Il mer 19 giu 2024, 18:54 Daniel P. Berrangé ha scritto: > >build/ > > rust/ > >.cargo/ > > config.toml # generated by configure or meson.build > >Cargo.toml # workspace generated by configure or meson.build > >Cargo.lock # can be either linked to srctree or generated > >qemu/ # symlink to srctree/rust/qemu > >aarch64-softmmu-hw/ > > Cargo.toml# generated by meson.build (*) > > src/ # symlink to srctree/rust/hw/ > >i386-softmmu-hw/ > > Cargo.toml# generated by meson.build > > src/ # symlink to srctree/rust/hw/ > >generated/ # files generated by rust/generated/meson.build > > If we're generating a build tree to invoke cargo on, can we then > avoid creating a completely separate dir hierarchy in the source > tree rooted at /rust, and just have rust source within our existing > hierarchy. > Maybe... I hadn't even considered the possibility of having a single cargo invocation (and thus a cargo workspace in the build tree) until Richard pointed out the duplication in configuration files. I suppose you could just link rust/aarch64-softmmu-hw to srctree/hw, and have a srctree/hw/lib.rs file in there to prime the search for submodules. I think the resulting hierarchy would feel a little foreign though. Without seeing the code I can't judge but my impression is that, if we wanted to go that way, I would also move all C code under src/. Perhaps we can consider such a unification later, once we have more experience, but for now keep Rust and C code separate? Paolo > eg > > aarch64-softmmu-hw/ > Cargo.toml# generated by meson.build (*) > src/ > pl011/ # symlink to srctree/hw/p1011/ > > > With regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org-o- > https://www.instagram.com/dberrange :| > >
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Wed, Jun 19, 2024 at 06:43:01PM +0200, Paolo Bonzini wrote:
> On 6/19/24 07:34, Richard Henderson wrote:
> > First silly question: how much of this is boiler plate that gets moved
> > the moment that the second rust subdirectory is added?
>
> If my suggestion at
> https://lore.kernel.org/qemu-devel/CABgObfaP7DRD8dbSKNmUzhZNyxeHWO0MztaW3_EFYt=vf6s...@mail.gmail.com/
> works, we'd have only two directories that have a Cargo.toml in it. For
> example it could be rust/qemu/ (common code) and rust/hw/ (code that depends
> on Kconfig).
>
> I think we can also have a rust/Cargo.toml file as in
> https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace,
> and then the various configuration files under rust/ will be valid for all
> subpackages.
>
> > > +[build]
> > > +rustflags = ["-Crelocation-model=pic", "-Ctarget-feature=+crt-static"]
> >
> > It seems certain that this is not specific to pl011, and will be commot
> > to other rust subdirectories. Or, given the .cargo directory name, is
> > this generated by cargo and committed by mistake?
>
> -Crelocation-mode should be pie. But also, I am not sure it works because I
> think it's always going to be overridden by cargo_wrapper.py? See
> https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags.
>
> (I'm not sure what +crt-static is for).
>
> I think the generate_cfg_flags() mechanism of cargo_wrapper.py has to be
> rewritten from Python to Rust and moved to build.rs (using
> cargo::rustc-cfg). By doing this, the cfg flags are added to whatever is in
> .cargo/config.toml, rather than replaced.
>
> > > diff --git a/rust/pl011/.gitignore b/rust/pl011/.gitignore
> > > new file mode 100644
> > > index 00..28a02c847f
> > > --- /dev/null
> > > +++ b/rust/pl011/.gitignore
> > > @@ -0,0 +1,2 @@
> > > +target
> > > +src/generated.rs.inc
> >
> > Is this a symptom of generating files into the source directory and not
> > build directory?
>
> If I understand correctly, Manos considered two possible ways to invoke
> cargo on the Rust code:
>
> - directly, in which case you need to copy the generated source file to
> rust/pl011/src/generated.rs.inc, because cargo does not know where the build
> tree
>
> - do everything through meson, which does the right thing because
> cargo_wrapper.py knows about the build tree and passes the information.
>
> To avoid this, the first workflow could be adjusted so that cargo can still
> be invoked directly, _but from the build tree_, not the source tree. For
> example configure could generate a .../build/.cargo/config.toml with
>
>[env]
>MESON_BUILD_ROOT = ".../build"
>
> (extra advantage: -Crelocation-model=pie can be added based on
> --enable-pie/--disable-pie). configure can also create symlinks in the
> build tree for the source tree's rust/, Cargo.toml and Cargo.lock.
>
> This allows rust/pl011/src/generated.rs (which probably will become
> something like rust/common/src/generated.rs) to be:
>
>include!(concat!(env!("MESON_BUILD_ROOT"), "/generated.rs.inc"));
>
> when cargo is invoked from the build tree.
>
> Putting everything together you'd have
>
>build/
> rust/
>.cargo/
> config.toml # generated by configure or meson.build
>Cargo.toml # workspace generated by configure or meson.build
>Cargo.lock # can be either linked to srctree or generated
>qemu/ # symlink to srctree/rust/qemu
>aarch64-softmmu-hw/
> Cargo.toml# generated by meson.build (*)
> src/ # symlink to srctree/rust/hw/
>i386-softmmu-hw/
> Cargo.toml# generated by meson.build
> src/ # symlink to srctree/rust/hw/
>generated/ # files generated by rust/generated/meson.build
If we're generating a build tree to invoke cargo on, can we then
avoid creating a completely separate dir hierarchy in the source
tree rooted at /rust, and just have rust source within our existing
hierarchy.
eg
aarch64-softmmu-hw/
Cargo.toml# generated by meson.build (*)
src/
pl011/ # symlink to srctree/hw/p1011/
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On 6/19/24 07:34, Richard Henderson wrote:
First silly question: how much of this is boiler plate that gets moved
the moment that the second rust subdirectory is added?
If my suggestion at
https://lore.kernel.org/qemu-devel/CABgObfaP7DRD8dbSKNmUzhZNyxeHWO0MztaW3_EFYt=vf6s...@mail.gmail.com/
works, we'd have only two directories that have a Cargo.toml in it. For
example it could be rust/qemu/ (common code) and rust/hw/ (code that
depends on Kconfig).
I think we can also have a rust/Cargo.toml file as in
https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace,
and then the various configuration files under rust/ will be valid for
all subpackages.
+[build]
+rustflags = ["-Crelocation-model=pic", "-Ctarget-feature=+crt-static"]
It seems certain that this is not specific to pl011, and will be commot to other rust
subdirectories. Or, given the .cargo directory name, is this generated by cargo and
committed by mistake?
-Crelocation-mode should be pie. But also, I am not sure it works
because I think it's always going to be overridden by cargo_wrapper.py?
See https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags.
(I'm not sure what +crt-static is for).
I think the generate_cfg_flags() mechanism of cargo_wrapper.py has to be
rewritten from Python to Rust and moved to build.rs (using
cargo::rustc-cfg). By doing this, the cfg flags are added to whatever
is in .cargo/config.toml, rather than replaced.
diff --git a/rust/pl011/.gitignore b/rust/pl011/.gitignore
new file mode 100644
index 00..28a02c847f
--- /dev/null
+++ b/rust/pl011/.gitignore
@@ -0,0 +1,2 @@
+target
+src/generated.rs.inc
Is this a symptom of generating files into the source directory and not
build directory?
If I understand correctly, Manos considered two possible ways to invoke
cargo on the Rust code:
- directly, in which case you need to copy the generated source file to
rust/pl011/src/generated.rs.inc, because cargo does not know where the
build tree
- do everything through meson, which does the right thing because
cargo_wrapper.py knows about the build tree and passes the information.
To avoid this, the first workflow could be adjusted so that cargo can
still be invoked directly, _but from the build tree_, not the source
tree. For example configure could generate a
.../build/.cargo/config.toml with
[env]
MESON_BUILD_ROOT = ".../build"
(extra advantage: -Crelocation-model=pie can be added based on
--enable-pie/--disable-pie). configure can also create symlinks in the
build tree for the source tree's rust/, Cargo.toml and Cargo.lock.
This allows rust/pl011/src/generated.rs (which probably will become
something like rust/common/src/generated.rs) to be:
include!(concat!(env!("MESON_BUILD_ROOT"), "/generated.rs.inc"));
when cargo is invoked from the build tree.
Putting everything together you'd have
build/
rust/
.cargo/
config.toml # generated by configure or meson.build
Cargo.toml # workspace generated by configure or meson.build
Cargo.lock # can be either linked to srctree or generated
qemu/ # symlink to srctree/rust/qemu
aarch64-softmmu-hw/
Cargo.toml# generated by meson.build (*)
src/ # symlink to srctree/rust/hw/
i386-softmmu-hw/
Cargo.toml# generated by meson.build
src/ # symlink to srctree/rust/hw/
generated/ # files generated by rust/generated/meson.build
(*) these have to be generated to change the package name, so
configure_file() seems like a good match for it.
This is suspiciously similar to what tests/tcg/ looks like, except that
tests/tcg/*/Makefile is just a symbolic link. I tried creating a
similar directory structure in a toy project, and it seemed to work...
Second silly question: does this really need to be committed to the
repository? It *appears* to be specific to the host+os-version of the
build. It is certainly very specific about versions and checksums...
Generally the idea is that libraries should not commit it, while
applications should commit it. The idea is that the Cargo.lock file
reproduces a working configuration, and dependencies are updated to
known-good releases when CI passes. I don't think I like this, but it
is what it is. I ascribe it to me being from the Jurassic.
But for now I would consider not committing Cargo.lock, because we don't
have the infrastructure to do that automatic dependency update (assuming
we want it). But we could generate it at release time so that it is
included in tarballs, and create the symlink from
srctree/rust/Cargo.lock into build/rust/ only if the file is present in
the source tree.
diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
[...]
+# bilge deps included here to include them with docs
+[dependencies]
+arbitrary-int = { version = "1.2.7" }
+bilge = { version = "0.2.0" }
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On 6/11/24 03:33, Manos Pitsidianakis wrote:
This commit adds a re-implementation of hw/char/pl011.c in Rust.
It uses generated Rust bindings (produced by `ninja
aarch64-softmmu-generated.rs`) to
register itself as a QOM type/class.
How to build:
1. Make sure rust, cargo and bindgen (cargo install bindgen-cli) are
installed
Ah hah. Nevermind my previous question -- cargo install produces bindgen v0.69.4, quite a
bit newer than the ubuntu 22.04 packaged version. I have progressed a bit.
Please bear with me as I attempt to learn Rust in the process of reviewing this. I
promise no bikesheding and only to ask silly questions.
rust/pl011/.cargo/config.toml | 2 +
rust/pl011/.gitignore | 2 +
rust/pl011/Cargo.lock | 120 +++
rust/pl011/Cargo.toml | 66
rust/pl011/README.md | 42 +++
rust/pl011/build.rs| 44 +++
rust/pl011/deny.toml | 57
rust/pl011/meson.build | 7 +
rust/pl011/rustfmt.toml| 1 +
First silly question: how much of this is boiler plate that gets moved the moment that the
second rust subdirectory is added?
diff --git a/rust/pl011/.cargo/config.toml b/rust/pl011/.cargo/config.toml
new file mode 100644
index 00..241210ffa7
--- /dev/null
+++ b/rust/pl011/.cargo/config.toml
@@ -0,0 +1,2 @@
+[build]
+rustflags = ["-Crelocation-model=pic", "-Ctarget-feature=+crt-static"]
It seems certain that this is not specific to pl011, and will be commot to other rust
subdirectories. Or, given the .cargo directory name, is this generated by cargo and
committed by mistake?
diff --git a/rust/pl011/.gitignore b/rust/pl011/.gitignore
new file mode 100644
index 00..28a02c847f
--- /dev/null
+++ b/rust/pl011/.gitignore
@@ -0,0 +1,2 @@
+target
+src/generated.rs.inc
Is this a symptom of generating files into the source directory and not build
directory?
diff --git a/rust/pl011/Cargo.lock b/rust/pl011/Cargo.lock
new file mode 100644
index 00..d0fa46f9f5
--- /dev/null
+++ b/rust/pl011/Cargo.lock
@@ -0,0 +1,120 @@
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
Second silly question: does this really need to be committed to the repository? It
*appears* to be specific to the host+os-version of the build. It is certainly very
specific about versions and checksums...
diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
new file mode 100644
index 00..db74f2b59f
--- /dev/null
+++ b/rust/pl011/Cargo.toml
@@ -0,0 +1,66 @@
+[package]
+name = "pl011"
+version = "0.1.0"
+edition = "2021"
+authors = ["Manos Pitsidianakis "]
+license = "GPL-2.0 OR GPL-3.0-or-later"
+readme = "README.md"
+homepage = "https://www.qemu.org";
+description = "pl011 device model for QEMU"
+repository = "https://gitlab.com/epilys/rust-for-qemu";
+resolver = "2"
+publish = false
+keywords = []
+categories = []
+
+[lib]
+crate-type = ["staticlib"]
+
+# bilge deps included here to include them with docs
+[dependencies]
+arbitrary-int = { version = "1.2.7" }
+bilge = { version = "0.2.0" }
+bilge-impl = { version = "0.2.0" }
Likewise.
diff --git a/rust/pl011/deny.toml b/rust/pl011/deny.toml
new file mode 100644
index 00..3992380509
--- /dev/null
+++ b/rust/pl011/deny.toml
@@ -0,0 +1,57 @@
+# cargo-deny configuration file
+
+[graph]
+targets = [
+"aarch64-unknown-linux-gnu",
+"x86_64-unknown-linux-gnu",
+"x86_64-apple-darwin",
+"aarch64-apple-darwin",
+"x86_64-pc-windows-gnu",
+"aarch64-pc-windows-gnullvm",
+]
Very much likewise.
Since aarch64-pc-windows-gnullvm is not a host that qemu supports, if this is not
auto-generated, I am confused.
diff --git a/rust/pl011/rustfmt.toml b/rust/pl011/rustfmt.toml
new file mode 12
index 00..39f97b043b
--- /dev/null
+++ b/rust/pl011/rustfmt.toml
@@ -0,0 +1 @@
+../rustfmt.toml
\ No newline at end of file
Newline. Also, third silly question: is there a way we can avoid replicating this within
every rust subdirectory? E.g. some search path option within one of the build tools?
+++ b/rust/pl011/src/definitions.rs
+++ b/rust/pl011/src/device.rs
+++ b/rust/pl011/src/device_class.rs
+++ b/rust/pl011/src/lib.rs
+++ b/rust/pl011/src/memory_ops.rs
Eek! Actual Rust! Skipping until I am better educated.
diff --git a/rust/pl011/src/generated.rs b/rust/pl011/src/generated.rs
new file mode 100644
index 00..670e7b541d
--- /dev/null
+++ b/rust/pl011/src/generated.rs
@@ -0,0 +1,5 @@
+#[cfg(MESON_GENERATED_RS)]
+include!(concat!(env!("OUT_DIR"), "/generated.rs"));
+
+#[cfg(not(MESON_GENERATED_RS))]
+include!("generated.rs.inc");
Is this indicative of Rust not being prepared to separate source and build directories?
I'm surprised there would have to be a file in the source to direct the compiler to look
for a file in the build.
r~
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Tue, 18 Jun 2024 at 10:30, Paolo Bonzini wrote: > > On Tue, Jun 18, 2024 at 11:13 AM Daniel P. Berrangé > wrote: > > I wonder if starting with a device implementation is perhaps the > > wrong idea, in terms of a practical yet simple first step. > > > > As devices go, the pl011 device is simple, but compared to other > > QOM impls in QEMU, devices are still relatively complex things, > > especially if we want to write against safe abstraction. > > It's true, but I think _some_ complexity provides a better guide as to > what are the next step. > > I think it's clear that they are, not in this order: > * calling QOM methods (Chardev) > * implementing QOM objects > * implementing QOM devices > ** qdev properties > ** MemoryRegion callbacks > * implementing Chardev callbacks > * general technique for bindings for C structs (Error, QAPI) Right, this is why I suggested the pl011 as a device: I felt it provided enough complexity in terms of where it interconnects to the rest of QEMU to be a realistic way to find out where the points of difficulty are, without being super complicated simply as a device. We don't need to have fully worked out solutions to these things in the first pass, but I agree with Paolo that we do want to have a clear path forward that says "this is what we're expecting the solutions to these points of difficulty to end up looking like and how we plan to get there". thanks -- PMM
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Tue, Jun 18, 2024 at 11:13 AM Daniel P. Berrangé wrote: > I wonder if starting with a device implementation is perhaps the > wrong idea, in terms of a practical yet simple first step. > > As devices go, the pl011 device is simple, but compared to other > QOM impls in QEMU, devices are still relatively complex things, > especially if we want to write against safe abstraction. It's true, but I think _some_ complexity provides a better guide as to what are the next step. I think it's clear that they are, not in this order: * calling QOM methods (Chardev) * implementing QOM objects * implementing QOM devices ** qdev properties ** MemoryRegion callbacks * implementing Chardev callbacks * general technique for bindings for C structs (Error, QAPI) > If we did this I think we would not have to give a "free pass" > for a hackish C-like first Rust impl. We would have something > designed well from day 1, showing small, but tangible benefits, > with a path to incrementally broadening the effort. I don't think it's that easy to have something self contained for a single submission. Reviewing the build system is a completely different proposition than reviewing generic-heavy QOM bindings. Paolo
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Mon, Jun 17, 2024 at 01:32:54PM +0200, Paolo Bonzini wrote:
> Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
> manos.pitsidiana...@linaro.org> ha scritto:
>
> > >qdev_define_type!(c"test-device", TestDevice);
> > >impl ObjectImpl for TestDevice {}
> > >impl DeviceImpl for TestDevice {}
> > >
> > >fn main() {
> > >let d = TestDevice::new();
> > >d.cold_reset();
> > >}
> > >
> > >Of course the code makes no sense but it's a start.
> >
> > Let's not rush into making interfaces without the need for them arising
> > first. It's easy to wander off into bikeshedding territory; case in
> > point, there has been little discussion on the code of this RFC and much
> > more focus on hypotheticals.
> >
>
> I see your point but I think it's important to understand the road ahead of
> us.
>
> Knowing that we can build and maintain a usable (does not have to be
> perfect) interface to QOM is important, and in fact it's already useful for
> the pl011 device's chardev. It's also important to play with more advanced
> usage of the language to ascertain what features of the language will be
> useful; for example, my current implementation uses generic associated
> types which are not available on Debian Bookworm—it should be easy to
> remove them but if I am wrong that's also a data point, and an important
> one.
>
> Don't get me wrong: *for this first device* only, it makes a lot of sense
> to have a very C-ish implementation. It lets us sort out the build system
> integration, tackle idiomatic bindings one piece at a time, and is easier
> to review than Marc-André's approach of building the whole QAPI bindings.
> But at the same time, I don't consider a C-ish device better just because
> it's written in Rust: as things stand your code has all the disadvantages
> of C and all the disadvantages of Rust. What makes it (extremely) valuable
> is that your code can lead us along the path towards reaping the advantages
> of Rust.
I wonder if starting with a device implementation is perhaps the
wrong idea, in terms of a practical yet simple first step.
As devices go, the pl011 device is simple, but compared to other
QOM impls in QEMU, devices are still relatively complex things,
especially if we want to write against safe abstraction.
How about we go simpler still, and focus on one of the object
backends. For example, the RNG backend interface is practically
the most trivial QOM impl we can do in QEMU. It has one virtual
method that needs to be implemented, which is passed a callback
to receive entropy, and one native method to call to indicate
completion.
Providing a safe Rust abstraction for implementing an RNG
backend looks like a much quicker proposition that a safe
abstraction for implementing a device. The various RNG impls
have a few places where they touch other QEMU code (rng-builtin
uses qemu_bh, rng-egd lightly touches chardev APIs, rng-random
touches main loop FD handlers). Each of those things though, are
small & useful API problems to look it solving.
If we did this I think we would not have to give a "free pass"
for a hackish C-like first Rust impl. We would have something
designed well from day 1, showing small, but tangible benefits,
with a path to incrementally broadening the effort.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Il lun 17 giu 2024, 23:45 Manos Pitsidianakis ha scritto: > Secondly, are you implying that these callbacks are not operated under > the BQL? No, I'm implying that if you had the following nested calls: unsafe read callback receives the opaque point -> cast to &mut to call safe read callback -> chardev function accessing the opaque value -> unsafe chardev callback receives the opaque pointer -> cast to & or &mut to call safe chardev callback Then you would violate the Rust invariant that there can only be one active reference at a single time if it is &mut. The only exception is that you can create an &mut UnsafeCell for an active &UnsafeCell. It doesn't matter if this cannot happen because of invariants in the QEMU code. If you are writing safe Rust, every cast to &mut requires very strong justification. The way you do it instead is to use RefCell, and borrow_mut() instead of casting to &mut. This way, at least, the invariant is checked at runtime. And in fact this _can_ happen. I hadn't checked until now because I was mostly worried about the conceptual problem, not any specific way undefined behavior can happen. But here it is: PL011State::read takes &mut -> qemu_chr_fe_accept_input -> mux_chr_accept_input -> pl011_can_receive -> creates & which is undefined behavior You probably can write a 20 lines test case in pure Rust that miri complains about. Should we make it a requirement for v3, that all callback methods in PL011State (read, write, can_receive, receive, event) take a &self and achieve interior mutability through RefCell? Probably not, a comment is enough even though technically this is _not_ valid Rust. But it is a high priority task after the initial commit. > Just say this directly instead of writing all these amounts of text I said in the first review that the state should be behind a RefCell. https://lore.kernel.org/qemu-devel/CABgObfY8BS0yCw2CxgDQTBA4np9BZgGJF3N=t6eoBcdACAE=n...@mail.gmail.com/ > Finally, this is Rust code. You cannot turn off the type system, you > cannot turn off the borrow checker, you can only go around creating new > types and references out of raw memory addresses and tell the compiler > 'trust me on this' I am sorry if this sounds condescending. But this is _exactly_ what I'm complaining about: that there is too much trust placed in the programmer. Converting from * to & does not turn off the borrow checker, but still you cannot trust anymore what the borrow checker says; and that's why you do it inside an abstraction, not in each and every callback of each and every device. This is what Marc-André did for QAPI; and he probably erred in the other direction for a PoC, but we _must_ agree that something as complete as his work is the direction that we _have_ to take. Again: I am sorry that you feel this way about my remark, because I thought I had only praised your work. I didn't think I was being condescending or dismissing. But I am worried that without the proper abstractions this is going to be a technical debt magnet with only marginal benefits over C. And frankly I am saying this from experience. Check out CVE-2019-18960 which was an out of bounds access in Firecracker caused by not using proper abstractions for DMA. And that's in a 100% Rust code base. Since we're starting from and interfacing with C it's only going to be worse; skimping on abstractions is simply something that we cannot afford. It's also the way Linux is introducing Rust in the code base. Whenever something needs access to C functionality they introduce bindings to it. It's slower, but it's sustainable. > Ignoring that misses the entire point of Rust. The > statement 'this is not Rust code but as good as C' is dishonest and > misguided. Check for example the source code of the nix crate, which > exposes libc and various POSIX/*nix APIs. Is it the same as C and not > Rust code? That's exactly my point. Libc provides mostly unsafe functions, which is on the level of what bindgen generates. Other crates on top provide *safe abstractions* such as nix's Dir (https://docs.rs/nix/0.29.0/nix/dir/struct.Dir.html). If possible, leaf crates use safe Rust (nix), and avoid unsafe (libc) as much as possible. Instead you're using the unsafe functions in the device itself. It's missing an equivalent of nix. > If you have specific scenarios in mind where such things exist in the > code and end up doing invalid behavior please be kind and write them > down explicitly and demonstrate them on code review. It doesn't matter whether they exist or not. The point of Rust is that the type system ensures that these invalid behaviors are caught either at compile time or (with RefCell) at run time. As things stand, your code cannot catch them and the language provides a false sense of security. On the other hand, I want to be clear agin that this is not a problem at all—but only as long as we agree that it's not the desired final state > This approach
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Tue, Jun 18, 2024 at 1:33 AM Pierrick Bouvier
wrote:
>
> On 6/17/24 14:04, Manos Pitsidianakis wrote:
> > On Mon, 17 Jun 2024 17:32, Paolo Bonzini wrote:
> >> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
> >> wrote:
> >>> I respectfully disagree and recommend taking another look at the code.
> >>>
> >>> The device actually performs all logic in non-unsafe methods and is
> >>> typed instead of operating on raw integers as fields/state. The C stuff
> >>> is the FFI boundary calls which you cannot avoid; they are the same
> >>> things you'd wrap under these bindings we're talking about.
> >>
> >> Indeed, but the whole point is that the bindings wrap unsafe code in
> >> such a way that the safety invariants hold. Not doing this, especially
> >> for a device that does not do DMA (so that there are very few ways
> >> that things can go wrong in the first place), runs counter to the
> >> whole philosophy of Rust.
> >>
> >> For example, you have:
> >>
> >> pub fn realize(&mut self) {
> >> unsafe {
> >> qemu_chr_fe_set_handlers(
> >> addr_of_mut!(self.char_backend),
> >> Some(pl011_can_receive),
> >> Some(pl011_receive),
> >> Some(pl011_event),
> >> None,
> >> addr_of_mut!(*self).cast::(),
> >> core::ptr::null_mut(),
> >> true,
> >> );
> >> }
> >> }
> >>
> >> where you are implicitly relying on the fact that pl011_can_receive(),
> >> pl011_receive(), pl011_event() are never called from the
> >> MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
> >> mutable references at the same time, one as an argument to the
> >> callbacks:
> >>
> >>pub fn read(&mut self, offset: hwaddr, ...
> >>
> >> and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
> >>
> >> This is not Rust code. It makes no attempt at enforcing the whole
> >> "shared XOR mutable" which is the basis of Rust's reference semantics.
> >> In other words, this is as safe as C code---sure, it can use nice
> >> abstractions for register access, it has "unsafe" added in front of
> >> pointer dereferences, but it is not safe.
> >>
> >> Again, I'm not saying it's a bad first step. It's *awesome* if we
> >> treat it as what it is: a guide towards providing safe bindings
> >> between Rust and C (which often implies them being idiomatic). But if
> >> we don't accept this, if there is no plan to make the code safe, it is
> >> a potential huge source of technical debt.
> >>
> >>> QEMU calls the device's FFI callbacks with its pointer and arguments,
> >>> the pointer gets dereferenced to the actual Rust type which qemu
> >>> guarantees is valid, then the Rust struct's methods are called to handle
> >>> each functionality. There is nothing actually unsafe here, assuming
> >>> QEMU's invariants and code are correct.
> >>
> >> The same can be said of C code, can't it? There is nothing unsafe in C
> >> code, assuming there are no bugs...
> >
> > Paolo, first please tone down your condescending tone, it's incredibly
> > offensive. I'm honestly certain this is not on purpose otherwise I'd not
> > engage at all.
>
> The best compliment you had was "I'm not saying it's a bad first step",
> and I would say this differently: It's a great first step!
Don't quote me out of context; I said It's an "awesome" first step,
though I qualified that we should treat it as "a guide towards
providing safe bindings between Rust and C". That is, as long as we
agree that it is not production quality code. Which it doesn't have to
be!
> We should have a first version where we stick to the C API, with all the
> Rust code being as Rusty as possible: benefit from typed enums, error
> handling, bounds checking and other nice things.
>
> It's useless to iterate/debate for two years on the list before landing
> something upstream. We can start with this, have one or two devices that
> use this build system, and then focus on designing a good interface for
> this.
I never said that I want perfection before landing upstream. I want _a path_.
When I read "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"[1], then sorry but I think that it's better to
stick with C.
On the other hand, if there is a path towards proper, maintainable
Rust, then I am even okay even with committing something that
technically contains undefined behavior.
[1] https://lore.kernel.org/qemu-devel/ez270.x96k6aeu0...@linaro.org/
> As I mentioned in my previous answer, this device already makes a good
> progress: it eliminates a whole class of memory errors, and the only
> issue brought by unsafe code is concurrency issues. And this should be
> our focus once we get the build infrastructure done!
Let's not exaggerate the benefits: no pointers were converted to RAII
(Box<> or Rc<>) in the course of writing the Rust co
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On 6/17/24 14:04, Manos Pitsidianakis wrote:
On Mon, 17 Jun 2024 17:32, Paolo Bonzini wrote:
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
wrote:
I respectfully disagree and recommend taking another look at the code.
The device actually performs all logic in non-unsafe methods and is
typed instead of operating on raw integers as fields/state. The C stuff
is the FFI boundary calls which you cannot avoid; they are the same
things you'd wrap under these bindings we're talking about.
Indeed, but the whole point is that the bindings wrap unsafe code in
such a way that the safety invariants hold. Not doing this, especially
for a device that does not do DMA (so that there are very few ways
that things can go wrong in the first place), runs counter to the
whole philosophy of Rust.
For example, you have:
pub fn realize(&mut self) {
unsafe {
qemu_chr_fe_set_handlers(
addr_of_mut!(self.char_backend),
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
addr_of_mut!(*self).cast::(),
core::ptr::null_mut(),
true,
);
}
}
where you are implicitly relying on the fact that pl011_can_receive(),
pl011_receive(), pl011_event() are never called from the
MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
mutable references at the same time, one as an argument to the
callbacks:
pub fn read(&mut self, offset: hwaddr, ...
and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
This is not Rust code. It makes no attempt at enforcing the whole
"shared XOR mutable" which is the basis of Rust's reference semantics.
In other words, this is as safe as C code---sure, it can use nice
abstractions for register access, it has "unsafe" added in front of
pointer dereferences, but it is not safe.
Again, I'm not saying it's a bad first step. It's *awesome* if we
treat it as what it is: a guide towards providing safe bindings
between Rust and C (which often implies them being idiomatic). But if
we don't accept this, if there is no plan to make the code safe, it is
a potential huge source of technical debt.
QEMU calls the device's FFI callbacks with its pointer and arguments,
the pointer gets dereferenced to the actual Rust type which qemu
guarantees is valid, then the Rust struct's methods are called to handle
each functionality. There is nothing actually unsafe here, assuming
QEMU's invariants and code are correct.
The same can be said of C code, can't it? There is nothing unsafe in C
code, assuming there are no bugs...
Paolo, first please tone down your condescending tone, it's incredibly
offensive. I'm honestly certain this is not on purpose otherwise I'd not
engage at all.
The best compliment you had was "I'm not saying it's a bad first step",
and I would say this differently: It's a great first step!
We should have a first version where we stick to the C API, with all the
Rust code being as Rusty as possible: benefit from typed enums, error
handling, bounds checking and other nice things.
It's useless to iterate/debate for two years on the list before landing
something upstream. We can start with this, have one or two devices that
use this build system, and then focus on designing a good interface for
this.
Secondly, are you implying that these callbacks are not operated under
the BQL? I'm not seeing the C UART devices using mutexes. If they are
not running under the BQL, then gladly we add mutexes, big deal. Just
say this directly instead of writing all these amounts of text. If it's
true I'd just accept it and move on with a new iteration. Isn't that the
point of code review? It is really that simple. Why not do this right
away? I'm frankly puzzled.
As I mentioned in my previous answer, this device already makes a good
progress: it eliminates a whole class of memory errors, and the only
issue brought by unsafe code is concurrency issues. And this should be
our focus once we get the build infrastructure done!
Finally, this is Rust code. You cannot turn off the type system, you
cannot turn off the borrow checker, you can only go around creating new
types and references out of raw memory addresses and tell the compiler
'trust me on this'. Ignoring that misses the entire point of Rust. The
statement 'this is not Rust code but as good as C' is dishonest and
misguided. Check for example the source code of the nix crate, which
exposes libc and various POSIX/*nix APIs. Is it the same as C and not
Rust code?
If you have specific scenarios in mind where such things exist in the
code and end up doing invalid behavior please be kind and write them
down explicitly and demonstrate them on code review. This approach of
'yes but no' is not constructive because it is not addressing any
specific problems directly. Instead it comes out as vague dismissive FUD
and I'm sure that is no
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On 6/17/24 07:32, Paolo Bonzini wrote:
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
wrote:
I respectfully disagree and recommend taking another look at the code.
The device actually performs all logic in non-unsafe methods and is
typed instead of operating on raw integers as fields/state. The C stuff
is the FFI boundary calls which you cannot avoid; they are the same
things you'd wrap under these bindings we're talking about.
Indeed, but the whole point is that the bindings wrap unsafe code in
such a way that the safety invariants hold. Not doing this, especially
for a device that does not do DMA (so that there are very few ways
that things can go wrong in the first place), runs counter to the
whole philosophy of Rust.
You are raising an interesting point, and should be a central discussion
when designing the future Rust API for this subsystem.
It may not be intuitive to people that even a code without any unsafe
section could still call code in a sequence that will violate some
invariants, and break Rust rules.
IMHO, this is one of the big challenge with the Rust/C interfacing,
including for Linux.
But it's *not yet* the goal of this series. First, we should see how to
build one device (I would even like to see a second, to factorize all
the boilerplate), and then, focus on which interface we want to have to
make it really better than the C version.
For example, you have:
pub fn realize(&mut self) {
unsafe {
qemu_chr_fe_set_handlers(
addr_of_mut!(self.char_backend),
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
addr_of_mut!(*self).cast::(),
core::ptr::null_mut(),
true,
);
}
}
where you are implicitly relying on the fact that pl011_can_receive(),
pl011_receive(), pl011_event() are never called from the
MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
mutable references at the same time, one as an argument to the
callbacks:
pub fn read(&mut self, offset: hwaddr, ...
and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
This is not Rust code. It makes no attempt at enforcing the whole
"shared XOR mutable" which is the basis of Rust's reference semantics.
In other words, this is as safe as C code---sure, it can use nice
abstractions for register access, it has "unsafe" added in front of
pointer dereferences, but it is not safe.
I think that Manos did a great amount of work to reduce the use of
unsafe code. Basically, he wrote the device as Rusty as possible, while
still using the QEMU C API part that is inevitable today.
In more, given the current design, yes some race conditions are possible
(depends on how QEMU calls callbacks installed), but a whole class of
problems is still eliminated.
From the start of this series, it was precised that focus was on build
system, and it seemed to me that we shifted on the hot debate of "How to
interface with C code?".
Again, I'm not saying it's a bad first step. It's *awesome* if we
treat it as what it is: a guide towards providing safe bindings
between Rust and C (which often implies them being idiomatic). But if
we don't accept this, if there is no plan to make the code safe, it is
a potential huge source of technical debt.
I agree, it should be the next direction after a first device was
written: How to remove almost all usage of unsafe code and maintain good
invariants in the API?
While talking about idiomatic, Rust tends to favor message passing to
memory share when it comes to concurrency (and IMHO, it's the right
thing). And it's definitely now how QEMU is architected at this time.
With extra locking, we should be able to have a first correct interface
that offers strong guarantees, and we can then work on making it faster
with concurrency.
QEMU calls the device's FFI callbacks with its pointer and arguments,
the pointer gets dereferenced to the actual Rust type which qemu
guarantees is valid, then the Rust struct's methods are called to handle
each functionality. There is nothing actually unsafe here, assuming
QEMU's invariants and code are correct.
The same can be said of C code, can't it? There is nothing unsafe in C
code, assuming there are no bugs...
Not the same, you still get other compile/runtime guarantees:
- strong typed enums, so no case is forgotten
- good error handling
- safe type conversions
- bound check of fifo access
The only open issue by calling unsafe code in this context is related to
(potential) concurrency. If a first step to have a good Rust API is to
run everything under a lock, there is good chance you already started to
design the device in the right way to support real concurrency later, so
it's still useful.
Pierrick
Paolo
I think we're actually in a great position. We have a PoC from Marc-André,
plus the
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Mon, 17 Jun 2024 17:32, Paolo Bonzini wrote:
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
wrote:
I respectfully disagree and recommend taking another look at the code.
The device actually performs all logic in non-unsafe methods and is
typed instead of operating on raw integers as fields/state. The C stuff
is the FFI boundary calls which you cannot avoid; they are the same
things you'd wrap under these bindings we're talking about.
Indeed, but the whole point is that the bindings wrap unsafe code in
such a way that the safety invariants hold. Not doing this, especially
for a device that does not do DMA (so that there are very few ways
that things can go wrong in the first place), runs counter to the
whole philosophy of Rust.
For example, you have:
pub fn realize(&mut self) {
unsafe {
qemu_chr_fe_set_handlers(
addr_of_mut!(self.char_backend),
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
addr_of_mut!(*self).cast::(),
core::ptr::null_mut(),
true,
);
}
}
where you are implicitly relying on the fact that pl011_can_receive(),
pl011_receive(), pl011_event() are never called from the
MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
mutable references at the same time, one as an argument to the
callbacks:
pub fn read(&mut self, offset: hwaddr, ...
and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
This is not Rust code. It makes no attempt at enforcing the whole
"shared XOR mutable" which is the basis of Rust's reference semantics.
In other words, this is as safe as C code---sure, it can use nice
abstractions for register access, it has "unsafe" added in front of
pointer dereferences, but it is not safe.
Again, I'm not saying it's a bad first step. It's *awesome* if we
treat it as what it is: a guide towards providing safe bindings
between Rust and C (which often implies them being idiomatic). But if
we don't accept this, if there is no plan to make the code safe, it is
a potential huge source of technical debt.
QEMU calls the device's FFI callbacks with its pointer and arguments,
the pointer gets dereferenced to the actual Rust type which qemu
guarantees is valid, then the Rust struct's methods are called to handle
each functionality. There is nothing actually unsafe here, assuming
QEMU's invariants and code are correct.
The same can be said of C code, can't it? There is nothing unsafe in C
code, assuming there are no bugs...
Paolo, first please tone down your condescending tone, it's incredibly
offensive. I'm honestly certain this is not on purpose otherwise I'd not
engage at all.
Secondly, are you implying that these callbacks are not operated under
the BQL? I'm not seeing the C UART devices using mutexes. If they are
not running under the BQL, then gladly we add mutexes, big deal. Just
say this directly instead of writing all these amounts of text. If it's
true I'd just accept it and move on with a new iteration. Isn't that the
point of code review? It is really that simple. Why not do this right
away? I'm frankly puzzled.
Finally, this is Rust code. You cannot turn off the type system, you
cannot turn off the borrow checker, you can only go around creating new
types and references out of raw memory addresses and tell the compiler
'trust me on this'. Ignoring that misses the entire point of Rust. The
statement 'this is not Rust code but as good as C' is dishonest and
misguided. Check for example the source code of the nix crate, which
exposes libc and various POSIX/*nix APIs. Is it the same as C and not
Rust code?
If you have specific scenarios in mind where such things exist in the
code and end up doing invalid behavior please be kind and write them
down explicitly and demonstrate them on code review. This approach of
'yes but no' is not constructive because it is not addressing any
specific problems directly. Instead it comes out as vague dismissive FUD
and I'm sure that is not what you or anyone else wants.
Please take some time to understand my POV here, it'd help both of us
immensely.
Sincerely thank you in advance,
Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
wrote:
> I respectfully disagree and recommend taking another look at the code.
>
> The device actually performs all logic in non-unsafe methods and is
> typed instead of operating on raw integers as fields/state. The C stuff
> is the FFI boundary calls which you cannot avoid; they are the same
> things you'd wrap under these bindings we're talking about.
Indeed, but the whole point is that the bindings wrap unsafe code in
such a way that the safety invariants hold. Not doing this, especially
for a device that does not do DMA (so that there are very few ways
that things can go wrong in the first place), runs counter to the
whole philosophy of Rust.
For example, you have:
pub fn realize(&mut self) {
unsafe {
qemu_chr_fe_set_handlers(
addr_of_mut!(self.char_backend),
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
addr_of_mut!(*self).cast::(),
core::ptr::null_mut(),
true,
);
}
}
where you are implicitly relying on the fact that pl011_can_receive(),
pl011_receive(), pl011_event() are never called from the
MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
mutable references at the same time, one as an argument to the
callbacks:
pub fn read(&mut self, offset: hwaddr, ...
and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
This is not Rust code. It makes no attempt at enforcing the whole
"shared XOR mutable" which is the basis of Rust's reference semantics.
In other words, this is as safe as C code---sure, it can use nice
abstractions for register access, it has "unsafe" added in front of
pointer dereferences, but it is not safe.
Again, I'm not saying it's a bad first step. It's *awesome* if we
treat it as what it is: a guide towards providing safe bindings
between Rust and C (which often implies them being idiomatic). But if
we don't accept this, if there is no plan to make the code safe, it is
a potential huge source of technical debt.
> QEMU calls the device's FFI callbacks with its pointer and arguments,
> the pointer gets dereferenced to the actual Rust type which qemu
> guarantees is valid, then the Rust struct's methods are called to handle
> each functionality. There is nothing actually unsafe here, assuming
> QEMU's invariants and code are correct.
The same can be said of C code, can't it? There is nothing unsafe in C
code, assuming there are no bugs...
Paolo
> >
> >I think we're actually in a great position. We have a PoC from Marc-André,
> >plus the experience of glib-rs (see below), that shows us that our goal of
> >idiomatic bindings is doable; and a complementary PoC from you that will
> >guide us and let us reach it in little steps. The first 90% of the work is
> >done, now we only have to do the second 90%... :) but then we can open the
> >floodgates for Rust code in QEMU.
> >
> >For what it's worth, in my opinion looking at glib-rs for inspiration is
> >> a bad idea, because that project has to support an immutable public
> >> interface (glib) while we do not.
> >>
> >
> >glib-rs includes support for GObject, which was itself an inspiration for
> >QOM (with differences, granted).
> >
> >There are a lot of libraries that we can take inspiration from: in addition
> >to glib-rs, zbus has an interesting approach to
> >serialization/deserialization for example that could inform future work on
> >QAPI. It's not a coincidence that these libraries integrate with more
> >traditional C code, because we are doing the same. Rust-vmm crates will
> >also become an interesting topic sooner or later.
> >
> >There's more to discuss about this topic which I am open to continuing
> >> on IRC instead!
> >>
> >
> >Absolutely, I will try to post code soonish and also review the build
> >system integration.
> >
> >Thanks,
> >
> >Paolo
> >
> >
> >> -- Manos Pitsidianakis
> >> Emulation and Virtualization Engineer at Linaro Ltd
> >>
> >> >
> >> >One thing that would be very useful is to have an Error
> >> >implementation. Looking at what Marc-André did for Error*
> >> >(
> >> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/
> >> ),
> >> >his precise implementation relies on his code to go back and forth
> >> >between Rust representation, borrowed C object with Rust bindings and
> >> >owned C object with Rust bindings. But I think we can at least have
> >> >something like this:
> >> >
> >> >// qemu::Error
> >> >pub struct Error {
> >> >msg: String,
> >> >/// Appends the print string of the error to the msg if not None
> >> >cause: Option>,
> >> >location: Option<(String, u32)>
> >> >}
> >> >
> >> >impl std::error::Error for Error { ... }
> >> >
> >> >impl Error {
> >> > ...
> >> > fn into_c_error(self) -> *const bindings::Er
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Mon, 17 Jun 2024 14:32, Paolo Bonzini wrote:
Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
manos.pitsidiana...@linaro.org> ha scritto:
>qdev_define_type!(c"test-device", TestDevice);
>impl ObjectImpl for TestDevice {}
>impl DeviceImpl for TestDevice {}
>
>fn main() {
>let d = TestDevice::new();
>d.cold_reset();
>}
>
>Of course the code makes no sense but it's a start.
Let's not rush into making interfaces without the need for them arising
first. It's easy to wander off into bikeshedding territory; case in
point, there has been little discussion on the code of this RFC and much
more focus on hypotheticals.
I see your point but I think it's important to understand the road ahead of
us.
Knowing that we can build and maintain a usable (does not have to be
perfect) interface to QOM is important, and in fact it's already useful for
the pl011 device's chardev. It's also important to play with more advanced
usage of the language to ascertain what features of the language will be
useful; for example, my current implementation uses generic associated
types which are not available on Debian Bookworm—it should be easy to
remove them but if I am wrong that's also a data point, and an important
one.
Don't get me wrong: *for this first device* only, it makes a lot of sense
to have a very C-ish implementation. It lets us sort out the build system
integration, tackle idiomatic bindings one piece at a time, and is easier
to review than Marc-André's approach of building the whole QAPI bindings.
But at the same time, I don't consider a C-ish device better just because
it's written in Rust: as things stand your code has all the disadvantages
of C and all the disadvantages of Rust. What makes it (extremely) valuable
is that your code can lead us along the path towards reaping the advantages
of Rust.
I respectfully disagree and recommend taking another look at the code.
The device actually performs all logic in non-unsafe methods and is
typed instead of operating on raw integers as fields/state. The C stuff
is the FFI boundary calls which you cannot avoid; they are the same
things you'd wrap under these bindings we're talking about.
QEMU calls the device's FFI callbacks with its pointer and arguments,
the pointer gets dereferenced to the actual Rust type which qemu
guarantees is valid, then the Rust struct's methods are called to handle
each functionality. There is nothing actually unsafe here, assuming
QEMU's invariants and code are correct.
I think we're actually in a great position. We have a PoC from Marc-André,
plus the experience of glib-rs (see below), that shows us that our goal of
idiomatic bindings is doable; and a complementary PoC from you that will
guide us and let us reach it in little steps. The first 90% of the work is
done, now we only have to do the second 90%... :) but then we can open the
floodgates for Rust code in QEMU.
For what it's worth, in my opinion looking at glib-rs for inspiration is
a bad idea, because that project has to support an immutable public
interface (glib) while we do not.
glib-rs includes support for GObject, which was itself an inspiration for
QOM (with differences, granted).
There are a lot of libraries that we can take inspiration from: in addition
to glib-rs, zbus has an interesting approach to
serialization/deserialization for example that could inform future work on
QAPI. It's not a coincidence that these libraries integrate with more
traditional C code, because we are doing the same. Rust-vmm crates will
also become an interesting topic sooner or later.
There's more to discuss about this topic which I am open to continuing
on IRC instead!
Absolutely, I will try to post code soonish and also review the build
system integration.
Thanks,
Paolo
-- Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
>
>One thing that would be very useful is to have an Error
>implementation. Looking at what Marc-André did for Error*
>(
https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/
),
>his precise implementation relies on his code to go back and forth
>between Rust representation, borrowed C object with Rust bindings and
>owned C object with Rust bindings. But I think we can at least have
>something like this:
>
>// qemu::Error
>pub struct Error {
>msg: String,
>/// Appends the print string of the error to the msg if not None
>cause: Option>,
>location: Option<(String, u32)>
>}
>
>impl std::error::Error for Error { ... }
>
>impl Error {
> ...
> fn into_c_error(self) -> *const bindings::Error { ... }
>}
>
>// qemu::Result
>type Result = Result;
>
>which can be implemented without too much code. This way any "bool
>f(..., Error *)" function (example: realize :)) could be implemented
>as returning qemu::Result<()>.
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
manos.pitsidiana...@linaro.org> ha scritto:
> >qdev_define_type!(c"test-device", TestDevice);
> >impl ObjectImpl for TestDevice {}
> >impl DeviceImpl for TestDevice {}
> >
> >fn main() {
> >let d = TestDevice::new();
> >d.cold_reset();
> >}
> >
> >Of course the code makes no sense but it's a start.
>
> Let's not rush into making interfaces without the need for them arising
> first. It's easy to wander off into bikeshedding territory; case in
> point, there has been little discussion on the code of this RFC and much
> more focus on hypotheticals.
>
I see your point but I think it's important to understand the road ahead of
us.
Knowing that we can build and maintain a usable (does not have to be
perfect) interface to QOM is important, and in fact it's already useful for
the pl011 device's chardev. It's also important to play with more advanced
usage of the language to ascertain what features of the language will be
useful; for example, my current implementation uses generic associated
types which are not available on Debian Bookworm—it should be easy to
remove them but if I am wrong that's also a data point, and an important
one.
Don't get me wrong: *for this first device* only, it makes a lot of sense
to have a very C-ish implementation. It lets us sort out the build system
integration, tackle idiomatic bindings one piece at a time, and is easier
to review than Marc-André's approach of building the whole QAPI bindings.
But at the same time, I don't consider a C-ish device better just because
it's written in Rust: as things stand your code has all the disadvantages
of C and all the disadvantages of Rust. What makes it (extremely) valuable
is that your code can lead us along the path towards reaping the advantages
of Rust.
I think we're actually in a great position. We have a PoC from Marc-André,
plus the experience of glib-rs (see below), that shows us that our goal of
idiomatic bindings is doable; and a complementary PoC from you that will
guide us and let us reach it in little steps. The first 90% of the work is
done, now we only have to do the second 90%... :) but then we can open the
floodgates for Rust code in QEMU.
For what it's worth, in my opinion looking at glib-rs for inspiration is
> a bad idea, because that project has to support an immutable public
> interface (glib) while we do not.
>
glib-rs includes support for GObject, which was itself an inspiration for
QOM (with differences, granted).
There are a lot of libraries that we can take inspiration from: in addition
to glib-rs, zbus has an interesting approach to
serialization/deserialization for example that could inform future work on
QAPI. It's not a coincidence that these libraries integrate with more
traditional C code, because we are doing the same. Rust-vmm crates will
also become an interesting topic sooner or later.
There's more to discuss about this topic which I am open to continuing
> on IRC instead!
>
Absolutely, I will try to post code soonish and also review the build
system integration.
Thanks,
Paolo
> -- Manos Pitsidianakis
> Emulation and Virtualization Engineer at Linaro Ltd
>
> >
> >One thing that would be very useful is to have an Error
> >implementation. Looking at what Marc-André did for Error*
> >(
> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/
> ),
> >his precise implementation relies on his code to go back and forth
> >between Rust representation, borrowed C object with Rust bindings and
> >owned C object with Rust bindings. But I think we can at least have
> >something like this:
> >
> >// qemu::Error
> >pub struct Error {
> >msg: String,
> >/// Appends the print string of the error to the msg if not None
> >cause: Option>,
> >location: Option<(String, u32)>
> >}
> >
> >impl std::error::Error for Error { ... }
> >
> >impl Error {
> > ...
> > fn into_c_error(self) -> *const bindings::Error { ... }
> >}
> >
> >// qemu::Result
> >type Result = Result;
> >
> >which can be implemented without too much code. This way any "bool
> >f(..., Error *)" function (example: realize :)) could be implemented
> >as returning qemu::Result<()>.
>
>
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Fri, 14 Jun 2024 20:50, Paolo Bonzini wrote:
On Fri, Jun 14, 2024 at 9:04 AM Manos Pitsidianakis
wrote:
On Thu, 13 Jun 2024 23:57, Paolo Bonzini wrote:
>On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé
wrote:
>> I guess there's a balance to be had somewhere on the spectrum between doing
>> everything against the raw C binding, vs everything against a perfectly
>> idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
>> but from a pragmmatic POV I doubt we want the barrier to entry to be that
>> high.
>
>Yes, I agree. I guess we could make things work step by step, even
>committing something that only focuses on the build system like
>Manos's work (I'll review it).
>
>I can try to look at the basic QOM interface.
>
>Manos, can you create a page on the wiki? Something like
>https://wiki.qemu.org/Features/Meson.
Certainly! Just to make sure I understood correctly, you mean a wiki
page describing how things work and tracking the progress?
I added https://wiki.qemu.org/Features/Meson/Rust
I moved it to https://wiki.qemu.org/Features/Rust/Meson :) and wrote
https://wiki.qemu.org/Features/Rust/QOM. I got to the point where at
least this compiles:
qdev_define_type!(c"test-device", TestDevice);
impl ObjectImpl for TestDevice {}
impl DeviceImpl for TestDevice {}
fn main() {
let d = TestDevice::new();
d.cold_reset();
}
Of course the code makes no sense but it's a start.
Let's not rush into making interfaces without the need for them arising
first. It's easy to wander off into bikeshedding territory; case in
point, there has been little discussion on the code of this RFC and much
more focus on hypotheticals.
For what it's worth, in my opinion looking at glib-rs for inspiration is
a bad idea, because that project has to support an immutable public
interface (glib) while we do not.
There's more to discuss about this topic which I am open to continuing
on IRC instead!
-- Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
One thing that would be very useful is to have an Error
implementation. Looking at what Marc-André did for Error*
(https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/),
his precise implementation relies on his code to go back and forth
between Rust representation, borrowed C object with Rust bindings and
owned C object with Rust bindings. But I think we can at least have
something like this:
// qemu::Error
pub struct Error {
msg: String,
/// Appends the print string of the error to the msg if not None
cause: Option>,
location: Option<(String, u32)>
}
impl std::error::Error for Error { ... }
impl Error {
...
fn into_c_error(self) -> *const bindings::Error { ... }
}
// qemu::Result
type Result = Result;
which can be implemented without too much code. This way any "bool
f(..., Error *)" function (example: realize :)) could be implemented
as returning qemu::Result<()>.
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Fri, Jun 14, 2024 at 9:04 AM Manos Pitsidianakis
wrote:
>
> On Thu, 13 Jun 2024 23:57, Paolo Bonzini wrote:
> >On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé
> >wrote:
> >> I guess there's a balance to be had somewhere on the spectrum between doing
> >> everything against the raw C binding, vs everything against a perfectly
> >> idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
> >> but from a pragmmatic POV I doubt we want the barrier to entry to be that
> >> high.
> >
> >Yes, I agree. I guess we could make things work step by step, even
> >committing something that only focuses on the build system like
> >Manos's work (I'll review it).
> >
> >I can try to look at the basic QOM interface.
> >
> >Manos, can you create a page on the wiki? Something like
> >https://wiki.qemu.org/Features/Meson.
>
>
> Certainly! Just to make sure I understood correctly, you mean a wiki
> page describing how things work and tracking the progress?
>
> I added https://wiki.qemu.org/Features/Meson/Rust
I moved it to https://wiki.qemu.org/Features/Rust/Meson :) and wrote
https://wiki.qemu.org/Features/Rust/QOM. I got to the point where at
least this compiles:
qdev_define_type!(c"test-device", TestDevice);
impl ObjectImpl for TestDevice {}
impl DeviceImpl for TestDevice {}
fn main() {
let d = TestDevice::new();
d.cold_reset();
}
Of course the code makes no sense but it's a start.
One thing that would be very useful is to have an Error
implementation. Looking at what Marc-André did for Error*
(https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/),
his precise implementation relies on his code to go back and forth
between Rust representation, borrowed C object with Rust bindings and
owned C object with Rust bindings. But I think we can at least have
something like this:
// qemu::Error
pub struct Error {
msg: String,
/// Appends the print string of the error to the msg if not None
cause: Option>,
location: Option<(String, u32)>
}
impl std::error::Error for Error { ... }
impl Error {
...
fn into_c_error(self) -> *const bindings::Error { ... }
}
// qemu::Result
type Result = Result;
which can be implemented without too much code. This way any "bool
f(..., Error *)" function (example: realize :)) could be implemented
as returning qemu::Result<()>.
Paolo
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, 13 Jun 2024 23:57, Paolo Bonzini wrote: On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé wrote: I guess there's a balance to be had somewhere on the spectrum between doing everything against the raw C binding, vs everything against a perfectly idiomatic Rust API wrapping the C bniding. The latter might be the ideal, but from a pragmmatic POV I doubt we want the barrier to entry to be that high. Yes, I agree. I guess we could make things work step by step, even committing something that only focuses on the build system like Manos's work (I'll review it). I can try to look at the basic QOM interface. Manos, can you create a page on the wiki? Something like https://wiki.qemu.org/Features/Meson. Certainly! Just to make sure I understood correctly, you mean a wiki page describing how things work and tracking the progress? I added https://wiki.qemu.org/Features/Meson/Rust And a Meson category https://wiki.qemu.org/Category:Meson Thanks, Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé wrote: > I guess there's a balance to be had somewhere on the spectrum between doing > everything against the raw C binding, vs everything against a perfectly > idiomatic Rust API wrapping the C bniding. The latter might be the ideal, > but from a pragmmatic POV I doubt we want the barrier to entry to be that > high. Yes, I agree. I guess we could make things work step by step, even committing something that only focuses on the build system like Manos's work (I'll review it). I can try to look at the basic QOM interface. Manos, can you create a page on the wiki? Something like https://wiki.qemu.org/Features/Meson. Paolo > Is this not something we can figure out organically as part of the code > design and review processes ? > > e.g. if during review we see a device impl doing something where a higher > level API would have unambiguous benefits, and creatino of such a higher > level API is a practically achieveable task, then ask for it. If a higher > level API is desirable, but possibly not practical, then raise it as an > potential idea, but be willing to accept the technical debt. > > With regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| >
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, Jun 13, 2024 at 6:06 PM Zhao Liu wrote: > I think deeper and higher level bindings will have more opens and will > likely require more discussion and exploration. So could we explore this > direction on another reference Rust device? > > I also think there won’t be too many Rust devices in the short term. > Going back to tweak or enhance existing Rust devices may not require too > much effort, once we have a definitive answer. > > I wonder if x86 could also implement a rust device (like the x86 timer > you mentioned before, hw/i386/kvm/i8254.c or hw/timer/i8254.c IIRC) to > try this? Or would you recommend another x86 device? :-) A timer device is a good idea, just because it's another pretty stable low-level QEMU API. The problem with hw/timer/i8254.c is that it has the KVM version, as you found. The HPET is an alternative though. Paolo
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Hi Paolo, On Thu, Jun 13, 2024 at 09:56:39AM +0200, Paolo Bonzini wrote: > Date: Thu, 13 Jun 2024 09:56:39 +0200 > From: Paolo Bonzini > Subject: Re: [RFC PATCH v2 3/5] rust: add PL011 device model > > Il gio 13 giu 2024, 09:13 Daniel P. Berrangé ha > scritto: > > > On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > > > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > > > manos.pitsidiana...@linaro.org> ha scritto: > > > > > > > In any case, it is out of scope for this RFC. Introducing wrappers > > would > > > > be a gradual process. > > > > > > > > > > Sure, how would you feel about such bindings being developed on list, and > > > maintained in a (somewhat) long-lived experimental branch? > > > > IMHO any higher level binding APIs for Rust should be acceptable in the > > main QEMU tree as soon as we accept Rust functionality. They can evolve > > in-tree based on the needs of whomever is creating and/or consuming them. > > > > My question is the opposite, should we accept Rust functionality without > proper high level bindings? I am afraid that, if more Rust devices are > contributed, it becomes technical debt to have a mix of idiomatic and C-ish > code. If the answer is no, then this PL011 device has to be developed out > of tree. I think deeper and higher level bindings will have more opens and will likely require more discussion and exploration. So could we explore this direction on another reference Rust device? I also think there won’t be too many Rust devices in the short term. Going back to tweak or enhance existing Rust devices may not require too much effort, once we have a definitive answer. I wonder if x86 could also implement a rust device (like the x86 timer you mentioned before, hw/i386/kvm/i8254.c or hw/timer/i8254.c IIRC) to try this? Or would you recommend another x86 device? :-) I'd be willing to help Manos try it if you think it's fine. Regards, Zhao
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, Jun 13, 2024 at 11:59:12AM +0300, Manos Pitsidianakis wrote: > On Thu, 13 Jun 2024 11:53, "Daniel P. Berrangé" wrote: > > On Thu, Jun 13, 2024 at 11:41:38AM +0300, Manos Pitsidianakis wrote: > > > > > diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml > > > > > new file mode 100644 > > > > > index 00..ebecb99fe0 > > > > > --- /dev/null > > > > > +++ b/rust/rustfmt.toml > > > > > @@ -0,0 +1,7 @@ > > > > > +edition = "2021" > > > > > +format_generated_files = false > > > > > +format_code_in_doc_comments = true > > > > > +format_strings = true > > > > > +imports_granularity = "Crate" > > > > > +group_imports = "StdExternalCrate" > > > > > +wrap_comments = true > > > > > > > About the Rust style, inspired from the discussion on my > > > previous > > > > simpletrace-rust [1], it looks like people prefer the default rust style > > > > and use the default check without custom configurations. > > > > > More style requirements are also an open, especially for > > > unstable ones, > > > > and it would be better to split this part into a separate patch, so that > > > > the discussion about style doesn't overshadow the focus on your example. > > > > > [1]: > > > https://lore.kernel.org/qemu-devel/zlnbgwk29ds9f...@redhat.com/ > > > > > > > > > > I had read that discussion and had that in mind. There's no need to worry > > > about format inconsistencies; these options are unstable -nightly only- > > > format options and they don't affect the default rust style (they actually > > > follow it). If you run a stable cargo fmt you will see the code won't > > > change > > > (but might complain that these settings are nightly only). > > > > > > What they do is extra work on top of the default style. If anything ends > > > up > > > incompatible with stable I agree it must be removed, there's no sense in > > > having a custom Rust style when the defaults are so reasonable. > > > > This doesn't make sense. One the one hand you're saying the rules don't > > have any effect on the code style vs the default, but on the otherhand > > saying they do "extra work" on top of the default style. Those can't > > both be true. > > No, I fear there's a confusion here. It means that if you run the stable > rustfmt with the default options the code doesn't change. I.e. it does not > conflict with the default style. > > What it does is group imports, format text in doc comments (which stable > rustfmt doesn't touch at all) and also splits long strings into several > lines, which are all helpful for e-mail patch reviews. Ah ok. Are we expecting these options to become part of stable rustfmt ? I would expect our contributors to primarily be using the Rust toolchain that comes with their distro, and not unstable -nightly toolchain. So I still wonder if this rustfmt config will have much real world benefit ? With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, Jun 13, 2024 at 09:56:39AM +0200, Paolo Bonzini wrote: > Il gio 13 giu 2024, 09:13 Daniel P. Berrangé ha > scritto: > > > On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > > > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > > > manos.pitsidiana...@linaro.org> ha scritto: > > > > > > > In any case, it is out of scope for this RFC. Introducing wrappers > > would > > > > be a gradual process. > > > > > > > > > > Sure, how would you feel about such bindings being developed on list, and > > > maintained in a (somewhat) long-lived experimental branch? > > > > IMHO any higher level binding APIs for Rust should be acceptable in the > > main QEMU tree as soon as we accept Rust functionality. They can evolve > > in-tree based on the needs of whomever is creating and/or consuming them. > > > > My question is the opposite, should we accept Rust functionality without > proper high level bindings? I am afraid that, if more Rust devices are > contributed, it becomes technical debt to have a mix of idiomatic and C-ish > code. If the answer is no, then this PL011 device has to be developed out > of tree. I guess there's a balance to be had somewhere on the spectrum between doing everything against the raw C binding, vs everything against a perfectly idiomatic Rust API wrapping the C bniding. The latter might be the ideal, but from a pragmmatic POV I doubt we want the barrier to entry to be that high. Is this not something we can figure out organically as part of the code design and review processes ? e.g. if during review we see a device impl doing something where a higher level API would have unambiguous benefits, and creatino of such a higher level API is a practically achieveable task, then ask for it. If a higher level API is desirable, but possibly not practical, then raise it as an potential idea, but be willing to accept the technical debt. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, 13 Jun 2024 11:53, "Daniel P. Berrangé" wrote: On Thu, Jun 13, 2024 at 11:41:38AM +0300, Manos Pitsidianakis wrote: > > diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml > > new file mode 100644 > > index 00..ebecb99fe0 > > --- /dev/null > > +++ b/rust/rustfmt.toml > > @@ -0,0 +1,7 @@ > > +edition = "2021" > > +format_generated_files = false > > +format_code_in_doc_comments = true > > +format_strings = true > > +imports_granularity = "Crate" > > +group_imports = "StdExternalCrate" > > +wrap_comments = true > > > > About the Rust style, inspired from the discussion on my previous > simpletrace-rust [1], it looks like people prefer the default rust style > and use the default check without custom configurations. > > More style requirements are also an open, especially for unstable ones, > and it would be better to split this part into a separate patch, so that > the discussion about style doesn't overshadow the focus on your example. > > [1]: https://lore.kernel.org/qemu-devel/zlnbgwk29ds9f...@redhat.com/ > I had read that discussion and had that in mind. There's no need to worry about format inconsistencies; these options are unstable -nightly only- format options and they don't affect the default rust style (they actually follow it). If you run a stable cargo fmt you will see the code won't change (but might complain that these settings are nightly only). What they do is extra work on top of the default style. If anything ends up incompatible with stable I agree it must be removed, there's no sense in having a custom Rust style when the defaults are so reasonable. This doesn't make sense. One the one hand you're saying the rules don't have any effect on the code style vs the default, but on the otherhand saying they do "extra work" on top of the default style. Those can't both be true. No, I fear there's a confusion here. It means that if you run the stable rustfmt with the default options the code doesn't change. I.e. it does not conflict with the default style. What it does is group imports, format text in doc comments (which stable rustfmt doesn't touch at all) and also splits long strings into several lines, which are all helpful for e-mail patch reviews. Thanks, Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, 13 Jun 2024 10:56, Paolo Bonzini wrote: Il gio 13 giu 2024, 09:13 Daniel P. Berrangé ha scritto: On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > manos.pitsidiana...@linaro.org> ha scritto: > > > In any case, it is out of scope for this RFC. Introducing wrappers would > > be a gradual process. > > > > Sure, how would you feel about such bindings being developed on list, and > maintained in a (somewhat) long-lived experimental branch? IMHO any higher level binding APIs for Rust should be acceptable in the main QEMU tree as soon as we accept Rust functionality. They can evolve in-tree based on the needs of whomever is creating and/or consuming them. My question is the opposite, should we accept Rust functionality without proper high level bindings? I am afraid that, if more Rust devices are contributed, it becomes technical debt to have a mix of idiomatic and C-ish code. If the answer is no, then this PL011 device has to be developed out of tree. Paolo Getting Rust into QEMU, at least for our team at Linaro, is a long term commitment, so we will be responsible for preventing and fixing technical debt. And it will be up to the hypothetical rust maintainers as well to "keep the garden tidy" so to speak. To put it another way, I personally plan on making sure any bindings and any QEMU-ffi idioms that arise are all homogeneous and don't end up being a burden for the code base. Your concern is valid, and thank you for raising it. I feel it is important to figure out how this will be managed, since it's also an argument for the final say in whether any of this code ends up in the upstream tree. Thanks Paolo, Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Thu, Jun 13, 2024 at 11:41:38AM +0300, Manos Pitsidianakis wrote: > > > diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml > > > new file mode 100644 > > > index 00..ebecb99fe0 > > > --- /dev/null > > > +++ b/rust/rustfmt.toml > > > @@ -0,0 +1,7 @@ > > > +edition = "2021" > > > +format_generated_files = false > > > +format_code_in_doc_comments = true > > > +format_strings = true > > > +imports_granularity = "Crate" > > > +group_imports = "StdExternalCrate" > > > +wrap_comments = true > > > > > > > About the Rust style, inspired from the discussion on my previous > > simpletrace-rust [1], it looks like people prefer the default rust style > > and use the default check without custom configurations. > > > > More style requirements are also an open, especially for unstable ones, > > and it would be better to split this part into a separate patch, so that > > the discussion about style doesn't overshadow the focus on your example. > > > > [1]: https://lore.kernel.org/qemu-devel/zlnbgwk29ds9f...@redhat.com/ > > > > I had read that discussion and had that in mind. There's no need to worry > about format inconsistencies; these options are unstable -nightly only- > format options and they don't affect the default rust style (they actually > follow it). If you run a stable cargo fmt you will see the code won't change > (but might complain that these settings are nightly only). > > What they do is extra work on top of the default style. If anything ends up > incompatible with stable I agree it must be removed, there's no sense in > having a custom Rust style when the defaults are so reasonable. This doesn't make sense. One the one hand you're saying the rules don't have any effect on the code style vs the default, but on the otherhand saying they do "extra work" on top of the default style. Those can't both be true. > To sum it up, the style is essentially the default one, so there's no > problem here! If the style config is essentially the default one, then just remove this config and make it actually the default. Having this file exist sets the (incorrect) expectation that we would be willing to accept changes that diverge from the default rust style, and that's not desirable IMHO. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Hello Zhao :)
On Thu, 13 Jun 2024 11:30, Zhao Liu wrote:
On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
Date: Tue, 11 Jun 2024 13:33:32 +0300
From: Manos Pitsidianakis
Subject: [RFC PATCH v2 3/5] rust: add PL011 device model
X-Mailer: git-send-email 2.44.0
This commit adds a re-implementation of hw/char/pl011.c in Rust.
It uses generated Rust bindings (produced by `ninja
aarch64-softmmu-generated.rs`) to
register itself as a QOM type/class.
How to build:
1. Make sure rust, cargo and bindgen (cargo install bindgen-cli) are
installed
2. Configure a QEMU build with:
--enable-system --target-list=aarch64-softmmu --enable-with-rust
3. Launching a VM with qemu-system-aarch64 should use the Rust version
of the pl011 device (unless it is not set up so in hw/arm/virt.c; the
type of the UART device is hardcoded).
To confirm, inspect `info qom-tree` in the monitor and look for an
`x-pl011-rust` device.
Signed-off-by: Manos Pitsidianakis
---
Hi Manos,
Thanks for your example!
diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
new file mode 100644
index 00..db74f2b59f
--- /dev/null
+++ b/rust/pl011/Cargo.toml
@@ -0,0 +1,66 @@
...
+[lints]
+[lints.rustdoc]
+broken_intra_doc_links = "deny"
+redundant_explicit_links = "deny"
+[lints.clippy]
+# lint groups
+correctness = { level = "deny", priority = -1 }
+suspicious = { level = "deny", priority = -1 }
+complexity = { level = "deny", priority = -1 }
+perf = { level = "deny", priority = -1 }
+cargo = { level = "deny", priority = -1 }
+nursery = { level = "deny", priority = -1 }
+style = { level = "deny", priority = -1 }
+# restriction group
+dbg_macro = "deny"
+rc_buffer = "deny"
+as_underscore = "deny"
+assertions_on_result_states = "deny"
+# pedantic group
+doc_markdown = "deny"
+expect_fun_call = "deny"
+borrow_as_ptr = "deny"
+case_sensitive_file_extension_comparisons = "deny"
+cast_lossless = "deny"
+cast_ptr_alignment = "allow"
+large_futures = "deny"
+waker_clone_wake = "deny"
+unused_enumerate_index = "deny"
+unnecessary_fallible_conversions = "deny"
+struct_field_names = "deny"
+manual_hash_one = "deny"
+into_iter_without_iter = "deny"
+option_if_let_else = "deny"
+missing_const_for_fn = "deny"
+significant_drop_tightening = "deny"
+multiple_crate_versions = "deny"
+significant_drop_in_scrutinee = "deny"
+cognitive_complexity = "deny"
+missing_safety_doc = "allow"
...
diff --git a/rust/pl011/rustfmt.toml b/rust/pl011/rustfmt.toml
new file mode 12
index 00..39f97b043b
--- /dev/null
+++ b/rust/pl011/rustfmt.toml
@@ -0,0 +1 @@
+../rustfmt.toml
...
diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml
new file mode 100644
index 00..ebecb99fe0
--- /dev/null
+++ b/rust/rustfmt.toml
@@ -0,0 +1,7 @@
+edition = "2021"
+format_generated_files = false
+format_code_in_doc_comments = true
+format_strings = true
+imports_granularity = "Crate"
+group_imports = "StdExternalCrate"
+wrap_comments = true
About the Rust style, inspired from the discussion on my previous
simpletrace-rust [1], it looks like people prefer the default rust style
and use the default check without custom configurations.
More style requirements are also an open, especially for unstable ones,
and it would be better to split this part into a separate patch, so that
the discussion about style doesn't overshadow the focus on your example.
[1]: https://lore.kernel.org/qemu-devel/zlnbgwk29ds9f...@redhat.com/
I had read that discussion and had that in mind. There's no need to
worry about format inconsistencies; these options are unstable -nightly
only- format options and they don't affect the default rust style (they
actually follow it). If you run a stable cargo fmt you will see the code
won't change (but might complain that these settings are nightly only).
What they do is extra work on top of the default style. If anything ends
up incompatible with stable I agree it must be removed, there's no sense
in having a custom Rust style when the defaults are so reasonable.
To sum it up, the style is essentially the default one, so there's no
problem here!
Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
> Date: Tue, 11 Jun 2024 13:33:32 +0300
> From: Manos Pitsidianakis
> Subject: [RFC PATCH v2 3/5] rust: add PL011 device model
> X-Mailer: git-send-email 2.44.0
>
> This commit adds a re-implementation of hw/char/pl011.c in Rust.
>
> It uses generated Rust bindings (produced by `ninja
> aarch64-softmmu-generated.rs`) to
> register itself as a QOM type/class.
>
> How to build:
>
> 1. Make sure rust, cargo and bindgen (cargo install bindgen-cli) are
>installed
> 2. Configure a QEMU build with:
>--enable-system --target-list=aarch64-softmmu --enable-with-rust
> 3. Launching a VM with qemu-system-aarch64 should use the Rust version
>of the pl011 device (unless it is not set up so in hw/arm/virt.c; the
>type of the UART device is hardcoded).
>
>To confirm, inspect `info qom-tree` in the monitor and look for an
>`x-pl011-rust` device.
>
> Signed-off-by: Manos Pitsidianakis
> ---
Hi Manos,
Thanks for your example!
> diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
> new file mode 100644
> index 00..db74f2b59f
> --- /dev/null
> +++ b/rust/pl011/Cargo.toml
> @@ -0,0 +1,66 @@
...
> +[lints]
> +[lints.rustdoc]
> +broken_intra_doc_links = "deny"
> +redundant_explicit_links = "deny"
> +[lints.clippy]
> +# lint groups
> +correctness = { level = "deny", priority = -1 }
> +suspicious = { level = "deny", priority = -1 }
> +complexity = { level = "deny", priority = -1 }
> +perf = { level = "deny", priority = -1 }
> +cargo = { level = "deny", priority = -1 }
> +nursery = { level = "deny", priority = -1 }
> +style = { level = "deny", priority = -1 }
> +# restriction group
> +dbg_macro = "deny"
> +rc_buffer = "deny"
> +as_underscore = "deny"
> +assertions_on_result_states = "deny"
> +# pedantic group
> +doc_markdown = "deny"
> +expect_fun_call = "deny"
> +borrow_as_ptr = "deny"
> +case_sensitive_file_extension_comparisons = "deny"
> +cast_lossless = "deny"
> +cast_ptr_alignment = "allow"
> +large_futures = "deny"
> +waker_clone_wake = "deny"
> +unused_enumerate_index = "deny"
> +unnecessary_fallible_conversions = "deny"
> +struct_field_names = "deny"
> +manual_hash_one = "deny"
> +into_iter_without_iter = "deny"
> +option_if_let_else = "deny"
> +missing_const_for_fn = "deny"
> +significant_drop_tightening = "deny"
> +multiple_crate_versions = "deny"
> +significant_drop_in_scrutinee = "deny"
> +cognitive_complexity = "deny"
> +missing_safety_doc = "allow"
...
> diff --git a/rust/pl011/rustfmt.toml b/rust/pl011/rustfmt.toml
> new file mode 12
> index 00..39f97b043b
> --- /dev/null
> +++ b/rust/pl011/rustfmt.toml
> @@ -0,0 +1 @@
> +../rustfmt.toml
...
> diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml
> new file mode 100644
> index 00..ebecb99fe0
> --- /dev/null
> +++ b/rust/rustfmt.toml
> @@ -0,0 +1,7 @@
> +edition = "2021"
> +format_generated_files = false
> +format_code_in_doc_comments = true
> +format_strings = true
> +imports_granularity = "Crate"
> +group_imports = "StdExternalCrate"
> +wrap_comments = true
>
About the Rust style, inspired from the discussion on my previous
simpletrace-rust [1], it looks like people prefer the default rust style
and use the default check without custom configurations.
More style requirements are also an open, especially for unstable ones,
and it would be better to split this part into a separate patch, so that
the discussion about style doesn't overshadow the focus on your example.
[1]: https://lore.kernel.org/qemu-devel/zlnbgwk29ds9f...@redhat.com/
Regards,
Zhao
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Il gio 13 giu 2024, 09:13 Daniel P. Berrangé ha scritto: > On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > > manos.pitsidiana...@linaro.org> ha scritto: > > > > > In any case, it is out of scope for this RFC. Introducing wrappers > would > > > be a gradual process. > > > > > > > Sure, how would you feel about such bindings being developed on list, and > > maintained in a (somewhat) long-lived experimental branch? > > IMHO any higher level binding APIs for Rust should be acceptable in the > main QEMU tree as soon as we accept Rust functionality. They can evolve > in-tree based on the needs of whomever is creating and/or consuming them. > My question is the opposite, should we accept Rust functionality without proper high level bindings? I am afraid that, if more Rust devices are contributed, it becomes technical debt to have a mix of idiomatic and C-ish code. If the answer is no, then this PL011 device has to be developed out of tree. Paolo > > With regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org-o- > https://www.instagram.com/dberrange :| > >
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > manos.pitsidiana...@linaro.org> ha scritto: > > > In any case, it is out of scope for this RFC. Introducing wrappers would > > be a gradual process. > > > > Sure, how would you feel about such bindings being developed on list, and > maintained in a (somewhat) long-lived experimental branch? IMHO any higher level binding APIs for Rust should be acceptable in the main QEMU tree as soon as we accept Rust functionality. They can evolve in-tree based on the needs of whomever is creating and/or consuming them. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Good morning Paolo, On Thu, 13 Jun 2024 00:27, Paolo Bonzini wrote: Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < manos.pitsidiana...@linaro.org> ha scritto: In any case, it is out of scope for this RFC. Introducing wrappers would be a gradual process. Sure, how would you feel about such bindings being developed on list, and maintained in a (somewhat) long-lived experimental branch? Hm the only thing that worries me is keeping it synced and postponing merge indefinitely. If we declare the rust parts as "experimental" we could evolve them quickly even on master. What do the other maintainers think? Thanks, Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < manos.pitsidiana...@linaro.org> ha scritto: > In any case, it is out of scope for this RFC. Introducing wrappers would > be a gradual process. > Sure, how would you feel about such bindings being developed on list, and maintained in a (somewhat) long-lived experimental branch? Paolo > Thanks, > Manos > >
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Wed, 12 Jun 2024 19:06, "Daniel P. Berrangé" wrote:
On Wed, Jun 12, 2024 at 05:14:56PM +0300, Manos Pitsidianakis wrote:
On Wed, 12 Jun 2024 15:29, Paolo Bonzini wrote:
> I think this is extremely useful to show where we could go in the task
> of creating more idiomatic bindings.
>
> On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
> wrote:
> > +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
> > +name: TYPE_PL011.as_ptr(),
> > +parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
> > +instance_size: core::mem::size_of::(),
> > +instance_align: core::mem::align_of::(),
> > +instance_init: Some(pl011_init),
> > +instance_post_init: None,
> > +instance_finalize: None,
> > +abstract_: false,
> > +class_size: 0,
> > +class_init: Some(pl011_class_init),
> > +class_base_init: None,
> > +class_data: core::ptr::null_mut(),
> > +interfaces: core::ptr::null_mut(),
> > +};
>
> QOM is certainly a major part of what we need to do for idiomatic
> bindings. This series includes both using QOM objects (chardev) and
> defining them.
>
> For using QOM objects, there is only one strategy that we need to
> implement for both Chardev and DeviceState/SysBusDevice which is nice.
> We can probably take inspiration from glib-rs, see for example
> - https://docs.rs/glib/latest/glib/object/trait.Cast.html
> - https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
> - https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
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 think that is over-stating things. This was only mentioned in passing
and my feeling was that we didn't have that detailed of a discussion
because at this stage such a discussion is a bit like putting the cart
before the horse.
While the initial focus might be to just consume a Rust API that is a
1:1 mapping of the C API, I expect that over time we'll end up writing
various higher level Rust wrappers around the C API. If we didn't do that,
then in effect we'd be making ourselves write psuedo-C code in Rust,
undermining many of the benefits we could get.
In any case, it is out of scope for this RFC. Introducing wrappers would
be a gradual process.
Thanks,
Manos
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Wed, Jun 12, 2024 at 05:14:56PM +0300, Manos Pitsidianakis wrote:
> On Wed, 12 Jun 2024 15:29, Paolo Bonzini wrote:
> > I think this is extremely useful to show where we could go in the task
> > of creating more idiomatic bindings.
> >
> > On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
> > wrote:
> > > +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
> > > +name: TYPE_PL011.as_ptr(),
> > > +parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
> > > +instance_size: core::mem::size_of::(),
> > > +instance_align: core::mem::align_of::(),
> > > +instance_init: Some(pl011_init),
> > > +instance_post_init: None,
> > > +instance_finalize: None,
> > > +abstract_: false,
> > > +class_size: 0,
> > > +class_init: Some(pl011_class_init),
> > > +class_base_init: None,
> > > +class_data: core::ptr::null_mut(),
> > > +interfaces: core::ptr::null_mut(),
> > > +};
> >
> > QOM is certainly a major part of what we need to do for idiomatic
> > bindings. This series includes both using QOM objects (chardev) and
> > defining them.
> >
> > For using QOM objects, there is only one strategy that we need to
> > implement for both Chardev and DeviceState/SysBusDevice which is nice.
> > We can probably take inspiration from glib-rs, see for example
> > - https://docs.rs/glib/latest/glib/object/trait.Cast.html
> > - https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
> > - https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
>
>
> 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 think that is over-stating things. This was only mentioned in passing
and my feeling was that we didn't have that detailed of a discussion
because at this stage such a discussion is a bit like putting the cart
before the horse.
While the initial focus might be to just consume a Rust API that is a
1:1 mapping of the C API, I expect that over time we'll end up writing
various higher level Rust wrappers around the C API. If we didn't do that,
then in effect we'd be making ourselves write psuedo-C code in Rust,
undermining many of the benefits we could get.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Wed, Jun 12, 2024 at 4:42 PM Manos Pitsidianakis
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,
}
struct MyDevice {
parent_obj: qemu::bindings::SysBusDevice,
private: ManuallyDrop,
}
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, 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;
and all the magic (for example Borrow, 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
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Wed, 12 Jun 2024 15:29, Paolo Bonzini wrote:
I think this is extremely useful to show where we could go in the task
of creating more idiomatic bindings.
On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
wrote:
+fn main() {
+println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");
+println!("cargo::rerun-if-env-changed=MESON_BUILD_ROOT");
+println!("cargo::rerun-if-changed=src/generated.rs.inc");
Why do you need this rerun-if-changed?
To show an error from build.rs in case the file is deleted and the build
is not via meson
+pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
+name: TYPE_PL011.as_ptr(),
+parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
+instance_size: core::mem::size_of::(),
+instance_align: core::mem::align_of::(),
+instance_init: Some(pl011_init),
+instance_post_init: None,
+instance_finalize: None,
+abstract_: false,
+class_size: 0,
+class_init: Some(pl011_class_init),
+class_base_init: None,
+class_data: core::ptr::null_mut(),
+interfaces: core::ptr::null_mut(),
+};
QOM is certainly a major part of what we need to do for idiomatic
bindings. This series includes both using QOM objects (chardev) and
defining them.
For using QOM objects, there is only one strategy that we need to
implement for both Chardev and DeviceState/SysBusDevice which is nice.
We can probably take inspiration from glib-rs, see for example
- https://docs.rs/glib/latest/glib/object/trait.Cast.html
- https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
- https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
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
It's not necessary to make bindings to write idiomatic Rust. If you
notice, most callbacks QEMU calls cast the pointer into the Rust struct
which then calls its idiomatic type methods. I like abstraction a lot,
but it has diminishing returns. We'll see how future Rust devices look
like and we can then decide if extra code for abstractions is a good
trade-off.
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.
struct PL011_Inner {
iomem: MemoryRegion,
readbuff: u32.
...
}
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. Consider a
Default impl that sets parent_obj as a field memset to zero; on instance
initialization we can do
*self = Self {
parent_obj: self.parent_obj,
..Self::default(),
};
"private" probably should be RefCell, avoiding the unsafe
state.as_mut().read(addr, size)
RefCell etc are not FFI safe. Also, nested fields must be visible so
that the offset_of! macro works, for QOM properties. 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.
There should also be macros to define the wrappers for MMIO MemoryRegions.
Do you mean the MemoryRegionOps?
+pub fn realize(&mut self) {
+unsafe {
+qemu_chr_fe_set_handlers(
+addr_of_mut!(self.char_backend),
+Some(pl011_can_receive),
+Some(pl011_receive),
+Some(pl011_event),
+None,
+addr_of_mut!(*self).cast::(),
+core::ptr::null_mut(),
+true,
+);
+}
Probably each set of callbacks (MemoryRegion, Chardev) needs to be a
struct that implements a trait.
+#[macro_export]
+macro_rules! define_property {
+($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default =
$defval:expr) => {
+$crate::generated::Property {
+name: $name,
+info: $prop,
+offset: ::core::mem::offset_of!($state, $field) as _,
+bitnr: 0,
+bitmask: 0,
+set_default: true,
+defval: $crate::generated::Property__bindgen_ty_1 { u:
$defval.into() },
+arrayoffset: 0,
+arrayinfo: ::core::ptr::null(),
+arrayfieldsize: 0,
+link_type: ::core::ptr::null()
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
I think this is extremely useful to show where we could go in the task
of creating more idiomatic bindings.
On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
wrote:
> +fn main() {
> +println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");
> +println!("cargo::rerun-if-env-changed=MESON_BUILD_ROOT");
> +println!("cargo::rerun-if-changed=src/generated.rs.inc");
Why do you need this rerun-if-changed?
> +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
> +name: TYPE_PL011.as_ptr(),
> +parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
> +instance_size: core::mem::size_of::(),
> +instance_align: core::mem::align_of::(),
> +instance_init: Some(pl011_init),
> +instance_post_init: None,
> +instance_finalize: None,
> +abstract_: false,
> +class_size: 0,
> +class_init: Some(pl011_class_init),
> +class_base_init: None,
> +class_data: core::ptr::null_mut(),
> +interfaces: core::ptr::null_mut(),
> +};
QOM is certainly a major part of what we need to do for idiomatic
bindings. This series includes both using QOM objects (chardev) and
defining them.
For using QOM objects, there is only one strategy that we need to
implement for both Chardev and DeviceState/SysBusDevice which is nice.
We can probably take inspiration from glib-rs, see for example
- https://docs.rs/glib/latest/glib/object/trait.Cast.html
- https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
- https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
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).
struct PL011_Inner {
iomem: MemoryRegion,
readbuff: u32.
...
}
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,
}
"private" probably should be RefCell, avoiding the unsafe
state.as_mut().read(addr, size)
There should also be macros to define the wrappers for MMIO MemoryRegions.
> +pub fn realize(&mut self) {
> +unsafe {
> +qemu_chr_fe_set_handlers(
> +addr_of_mut!(self.char_backend),
> +Some(pl011_can_receive),
> +Some(pl011_receive),
> +Some(pl011_event),
> +None,
> +addr_of_mut!(*self).cast::(),
> +core::ptr::null_mut(),
> +true,
> +);
> +}
Probably each set of callbacks (MemoryRegion, Chardev) needs to be a
struct that implements a trait.
> +#[macro_export]
> +macro_rules! define_property {
> +($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default =
> $defval:expr) => {
> +$crate::generated::Property {
> +name: $name,
> +info: $prop,
> +offset: ::core::mem::offset_of!($state, $field) as _,
> +bitnr: 0,
> +bitmask: 0,
> +set_default: true,
> +defval: $crate::generated::Property__bindgen_ty_1 { u:
> $defval.into() },
> +arrayoffset: 0,
> +arrayinfo: ::core::ptr::null(),
> +arrayfieldsize: 0,
> +link_type: ::core::ptr::null(),
> +}
> +};
Perhaps we can define macros similar to the C DEFINE_PROP_*, and const
functions can be used to enforce some kind of type safety.
pub const fn check_type(_x: &T, y: T) -> T { y }
static VAR: i16 = 42i16;
static TEST: i8 = check_type(&VAR, 43i8);
pub fn main() { println!("{}", TEST); }
error[E0308]: mismatched types
--> ff.rs:4:30
|
4 | static TEST: i8 = check_type(&VAR, 43i8);
| -- expected `&i8`, found `&i16`
| |
| arguments to this function are incorrect
|
= note: expected reference `&i8`
found reference `&i16`
Anyhow I think this is already very useful, because as the
abstractions are developed, it is possible to see how the device code
evolves.
Paolo
[RFC PATCH v2 3/5] rust: add PL011 device model
This commit adds a re-implementation of hw/char/pl011.c in Rust.
It uses generated Rust bindings (produced by `ninja
aarch64-softmmu-generated.rs`) to
register itself as a QOM type/class.
How to build:
1. Make sure rust, cargo and bindgen (cargo install bindgen-cli) are
installed
2. Configure a QEMU build with:
--enable-system --target-list=aarch64-softmmu --enable-with-rust
3. Launching a VM with qemu-system-aarch64 should use the Rust version
of the pl011 device (unless it is not set up so in hw/arm/virt.c; the
type of the UART device is hardcoded).
To confirm, inspect `info qom-tree` in the monitor and look for an
`x-pl011-rust` device.
Signed-off-by: Manos Pitsidianakis
---
MAINTAINERS| 6 +
meson.build| 4 +
rust/meson.build | 2 +
rust/pl011/.cargo/config.toml | 2 +
rust/pl011/.gitignore | 2 +
rust/pl011/Cargo.lock | 120 +++
rust/pl011/Cargo.toml | 66
rust/pl011/README.md | 42 +++
rust/pl011/build.rs| 44 +++
rust/pl011/deny.toml | 57
rust/pl011/meson.build | 7 +
rust/pl011/rustfmt.toml| 1 +
rust/pl011/src/definitions.rs | 95 ++
rust/pl011/src/device.rs | 531 ++
rust/pl011/src/device_class.rs | 95 ++
rust/pl011/src/generated.rs| 5 +
rust/pl011/src/lib.rs | 581 +
rust/pl011/src/memory_ops.rs | 38 +++
rust/rustfmt.toml | 7 +
19 files changed, 1705 insertions(+)
create mode 100644 rust/pl011/.cargo/config.toml
create mode 100644 rust/pl011/.gitignore
create mode 100644 rust/pl011/Cargo.lock
create mode 100644 rust/pl011/Cargo.toml
create mode 100644 rust/pl011/README.md
create mode 100644 rust/pl011/build.rs
create mode 100644 rust/pl011/deny.toml
create mode 100644 rust/pl011/meson.build
create mode 12 rust/pl011/rustfmt.toml
create mode 100644 rust/pl011/src/definitions.rs
create mode 100644 rust/pl011/src/device.rs
create mode 100644 rust/pl011/src/device_class.rs
create mode 100644 rust/pl011/src/generated.rs
create mode 100644 rust/pl011/src/lib.rs
create mode 100644 rust/pl011/src/memory_ops.rs
create mode 100644 rust/rustfmt.toml
diff --git a/MAINTAINERS b/MAINTAINERS
index ff6117f41d..e77a3d4169 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1181,6 +1181,11 @@ F: include/hw/*/microbit*.h
F: tests/qtest/microbit-test.c
F: docs/system/arm/nrf.rst
+ARM PL011 Rust device
+M: Manos Pitsidianakis
+S: Maintained
+F: rust/pl011/
+
AVR Machines
-
@@ -4229,6 +4234,7 @@ S: Maintained
F: scripts/cargo_wrapper.py
F: rust/meson.build
F: rust/wrapper.h
+F: rust/rustfmt.toml
Miscellaneous
-
diff --git a/meson.build b/meson.build
index a08c975ef9..23e8c43562 100644
--- a/meson.build
+++ b/meson.build
@@ -296,6 +296,10 @@ if get_option('with_rust').allowed()
endif
with_rust = cargo.found()
+if with_rust
+ subdir('rust')
+endif
+
# default flags for all hosts
# We use -fwrapv to tell the compiler that we require a C dialect where
# left shift of signed integers is well defined and has the expected
diff --git a/rust/meson.build b/rust/meson.build
index e9660a3045..264787198f 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -67,6 +67,8 @@ endif
rust_hw_target_list = {}
+subdir('pl011')
+
foreach rust_hw_target, rust_hws: rust_hw_target_list
foreach rust_hw_dev: rust_hws
output = meson.current_build_dir() / rust_target_triple / rs_build_type /
rust_hw_dev['output']
diff --git a/rust/pl011/.cargo/config.toml b/rust/pl011/.cargo/config.toml
new file mode 100644
index 00..241210ffa7
--- /dev/null
+++ b/rust/pl011/.cargo/config.toml
@@ -0,0 +1,2 @@
+[build]
+rustflags = ["-Crelocation-model=pic", "-Ctarget-feature=+crt-static"]
diff --git a/rust/pl011/.gitignore b/rust/pl011/.gitignore
new file mode 100644
index 00..28a02c847f
--- /dev/null
+++ b/rust/pl011/.gitignore
@@ -0,0 +1,2 @@
+target
+src/generated.rs.inc
diff --git a/rust/pl011/Cargo.lock b/rust/pl011/Cargo.lock
new file mode 100644
index 00..d0fa46f9f5
--- /dev/null
+++ b/rust/pl011/Cargo.lock
@@ -0,0 +1,120 @@
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
+version = 3
+
+[[package]]
+name = "arbitrary-int"
+version = "1.2.7"
+source = "registry+https://github.com/rust-lang/crates.io-index";
+checksum = "c84fc003e338a6f69fbd4f7fe9f92b535ff13e9af8997f3b14b6ddff8b1df46d"
+
+[[package]]
+name = "bilge"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index";
+checksum = "dc707ed8ebf81de5cd6c7f48f54b4c8621760926cdf35a57000747c512e67b57"
+dependencies = [
+ "arbitrary-int",
+ "bilge-impl",
+]
+
+[[package]]
+name = "bilge-impl"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index";
+checksum = "feb11e002038ad243af39c2068c8a72bcf147acf05025dcd
