| Message ID | 0fde311846394e9f7633be5d72cc30b25587d7a1.1718101832.git.manos.pitsidianakis@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Implement ARM PL011 in Rust | expand |
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 <manos.pitsidianakis@linaro.org> 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::<PL011State>(), > + instance_align: core::mem::align_of::<PL011State>(), > + 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<PL011_Inner>, 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::<c_void>(), > + 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<T>(_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
On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> 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 ><manos.pitsidianakis@linaro.org> 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::<PL011State>(), >> + instance_align: core::mem::align_of::<PL011State>(), >> + 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<PL011_Inner>, 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::<c_void>(), >> + 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_*, Hopefully if I end up doing something like this, it'd be in a standalone crate for other devices to use > >and const >functions can be used to enforce some kind of type safety. > >pub const fn check_type<T>(_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` Yes this kind of type safety trick is easy to do (already done for a register macro in src/lib.rs). 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. Thanks Paolo, Manos
On Wed, Jun 12, 2024 at 4:42 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > There was consensus in the community call that we won't be writing Rust > APIs for internal C QEMU interfaces; or at least, that's not the goal I disagree with that. We need _some_ kind of bindings, otherwise we have too much unsafe code, and the benefit of Rust becomes so much lower that I doubt the utility. If something is used by only one device then fine, but when some kind of unsafe code repeats across most if not all devices, that is a problem. It can be macros, it can be smart pointers, that remains to be seen---but repetition should be a warning signal that _something_ is necessary. > >For definining new classes I think it's okay if Rust does not support > >writing superclasses yet, only leaves. > > > >I would make a QOM class written in Rust a struct that only contains > >the new fields. The struct must implement Default and possibly Drop > >(for finalize). > > The object is allocated and freed from C, hence it is not Dropped. We're > only ever accessing it from a reference retrieved from a QEMU provided > raw pointer. If the struct gains heap object fields like Box or Vec or > String, they'd have to be dropped manually on _unrealize. That's my point, if you have struct MyDevice_Inner { data: Vec<u8>, } struct MyDevice { parent_obj: qemu::bindings::SysBusDevice, private: ManuallyDrop<PL011_Inner>, } then the instance_finalize method can simply do pub instance_finalize(self: *c_void) { let dev = self as *mut MyDevice; unsafe { ManuallyDrop::drop(dev.private) } } Don't do it on _unrealize, create macros that do it for you. > >and then a macro defines a wrapper struct that includes just two > >fields, one for the superclass and one for the Rust struct. > >instance_init can initialize the latter with Default::default(). > > > > struct PL011 { > > parent_obj: qemu::bindings::SysBusDevice, > > private: PL011_Inner, > > } > > a nested struct is not necessary for using the Default trait Agreed, but a nested struct is nice anyway in my opinion as a boundary between the C-ish and Rust idiomatic code. > >"private" probably should be RefCell<PL011_Inner>, avoiding the unsafe > > > > state.as_mut().read(addr, size) > > > RefCell etc are not FFI safe. Why does it matter? Everything after the SysBusDevice is private. > Also, nested fields must be visible so that the offset_of! macro works, for QOM properties. Note that QOM properties do not use offset_of; qdev properties do. Using qdev properties is much easier because they hide visitors, but again - not necessary, sometimes going lower-level can be easier if the API you wrap is less C-ish. Also, you can define constants (including properties) in contexts where non-public fields are visible: use std::mem; pub struct Foo { _x: i32, y: i32, } impl Foo { pub const OFFSET_Y: usize = mem::offset_of!(Foo, y); } fn main() { println!("{}", Foo::OFFSET_Y); } Any offset needed to go past the SysBusDevice and any other fields before MyDevice_Inner can be added via macros. Also note that it doesn't _have_ to be RefCell; RefCell isn't particularly magic. We can implement our own interior mutability thingy that is more friendly to qdev properties, or that includes the ManuallyDrop<> thing from above, or both. For example you could have type PL011 = QOMImpl<qemu::bindings::SysBusDevice, PL011_Inner>; and all the magic (for example Borrow<PL011_Inner>, the TypeInfo, the instance_init and instance_finalize function) would be in QOMImpl. My point is: let's not focus on having a C-like API. It's the easiest thing to do but not the target. > Finally, > > state.as_mut().read(addr, size) > > Is safe since we receive a valid pointer from QEMU. This fact cannot be > derived by the compiler, which is why it has an `unsafe` keyword. That > does not mean that the use here is unsafe. Yes, it is safe otherwise it would be undefined behavior, but there are no checks of the kind that you have in Rust whenever you have &mut. state.as_mut() implies that no other references to state are in use; but there are (you pass it as the opaque value to both the MemoryRegionOps and the chardev frontend callbacks). This is why I think something like RefCell is needed to go from a shared reference to an exclusive one (interior mutability). > >There should also be macros to define the wrappers for MMIO MemoryRegions. > > Do you mean the MemoryRegionOps? Yes. > I wanted to focus on the build system integration for the first RFC > which is why there are some macros but not in every place it makes > sense. Yes, absolutely. We need to start somewhere. Paolo
On Wed, Jun 12, 2024 at 05:14:56PM +0300, Manos Pitsidianakis wrote: > On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> 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 > > <manos.pitsidianakis@linaro.org> 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::<PL011State>(), > > > + instance_align: core::mem::align_of::<PL011State>(), > > > + 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
On Wed, 12 Jun 2024 19:06, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >On Wed, Jun 12, 2024 at 05:14:56PM +0300, Manos Pitsidianakis wrote: >> On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> 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 >> > <manos.pitsidianakis@linaro.org> 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::<PL011State>(), >> > > + instance_align: core::mem::align_of::<PL011State>(), >> > > + 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
Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < manos.pitsidianakis@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 > >
Good morning Paolo, On Thu, 13 Jun 2024 00:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < >manos.pitsidianakis@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
On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > manos.pitsidianakis@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
Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> 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.pitsidianakis@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 :| > >
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 <manos.pitsidianakis@linaro.org> > 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 <manos.pitsidianakis@linaro.org> > --- Hi Manos, Thanks for your example! > diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml > new file mode 100644 > index 0000000000..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 120000 > index 0000000000..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 0000000000..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/ZlnBGwk29Ds9FjUA@redhat.com/ Regards, Zhao
Hello Zhao :) On Thu, 13 Jun 2024 11:30, Zhao Liu <zhao1.liu@intel.com> 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 <manos.pitsidianakis@linaro.org> >> 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 <manos.pitsidianakis@linaro.org> >> --- > >Hi Manos, > >Thanks for your example! > >> diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml >> new file mode 100644 >> index 0000000000..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 120000 >> index 0000000000..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 0000000000..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/ZlnBGwk29Ds9FjUA@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
On Thu, 13 Jun 2024 10:56, Paolo Bonzini <pbonzini@redhat.com> wrote: >Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> 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.pitsidianakis@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
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 0000000000..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/ZlnBGwk29Ds9FjUA@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
On Thu, 13 Jun 2024 11:53, "Daniel P. Berrangé" <berrange@redhat.com> 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 0000000000..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/ZlnBGwk29Ds9FjUA@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
On Thu, Jun 13, 2024 at 09:56:39AM +0200, Paolo Bonzini wrote: > Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> 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.pitsidianakis@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
On Thu, Jun 13, 2024 at 11:59:12AM +0300, Manos Pitsidianakis wrote: > On Thu, 13 Jun 2024 11:53, "Daniel P. Berrangé" <berrange@redhat.com> 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 0000000000..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/ZlnBGwk29Ds9FjUA@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
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 <pbonzini@redhat.com> > Subject: Re: [RFC PATCH v2 3/5] rust: add PL011 device model > > Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> 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.pitsidianakis@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
On Thu, Jun 13, 2024 at 6:06 PM Zhao Liu <zhao1.liu@intel.com> 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
On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> 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 :| >
On Thu, 13 Jun 2024 23:57, Paolo Bonzini <pbonzini@redhat.com> wrote: >On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> 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
On Fri, Jun 14, 2024 at 9:04 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > On Thu, 13 Jun 2024 23:57, Paolo Bonzini <pbonzini@redhat.com> wrote: > >On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> 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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<Box<dyn std::error::Error>>, location: Option<(String, u32)> } impl std::error::Error for Error { ... } impl Error { ... fn into_c_error(self) -> *const bindings::Error { ... } } // qemu::Result<T> type Result<T> = Result<T, Error>; 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
On Fri, 14 Jun 2024 20:50, Paolo Bonzini <pbonzini@redhat.com> wrote: >On Fri, Jun 14, 2024 at 9:04 AM Manos Pitsidianakis ><manos.pitsidianakis@linaro.org> wrote: >> >> On Thu, 13 Jun 2024 23:57, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> 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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<Box<dyn std::error::Error>>, > location: Option<(String, u32)> >} > >impl std::error::Error for Error { ... } > >impl Error { > ... > fn into_c_error(self) -> *const bindings::Error { ... } >} > >// qemu::Result<T> >type Result<T> = Result<T, Error>; > >which can be implemented without too much code. This way any "bool >f(..., Error *)" function (example: realize :)) could be implemented >as returning qemu::Result<()>.
Il lun 17 giu 2024, 10:59 Manos Pitsidianakis < manos.pitsidianakis@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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<Box<dyn std::error::Error>>, > > location: Option<(String, u32)> > >} > > > >impl std::error::Error for Error { ... } > > > >impl Error { > > ... > > fn into_c_error(self) -> *const bindings::Error { ... } > >} > > > >// qemu::Result<T> > >type Result<T> = Result<T, Error>; > > > >which can be implemented without too much code. This way any "bool > >f(..., Error *)" function (example: realize :)) could be implemented > >as returning qemu::Result<()>. > >
On Mon, 17 Jun 2024 14:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >Il lun 17 giu 2024, 10:59 Manos Pitsidianakis < >manos.pitsidianakis@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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<Box<dyn std::error::Error>>, >> > location: Option<(String, u32)> >> >} >> > >> >impl std::error::Error for Error { ... } >> > >> >impl Error { >> > ... >> > fn into_c_error(self) -> *const bindings::Error { ... } >> >} >> > >> >// qemu::Result<T> >> >type Result<T> = Result<T, Error>; >> > >> >which can be implemented without too much code. This way any "bool >> >f(..., Error *)" function (example: realize :)) could be implemented >> >as returning qemu::Result<()>. >> >>
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> 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::<c_void>(), 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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<Box<dyn std::error::Error>>, > >> > location: Option<(String, u32)> > >> >} > >> > > >> >impl std::error::Error for Error { ... } > >> > > >> >impl Error { > >> > ... > >> > fn into_c_error(self) -> *const bindings::Error { ... } > >> >} > >> > > >> >// qemu::Result<T> > >> >type Result<T> = Result<T, Error>; > >> > > >> >which can be implemented without too much code. This way any "bool > >> >f(..., Error *)" function (example: realize :)) could be implemented > >> >as returning qemu::Result<()>. > >> > >> >
On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis ><manos.pitsidianakis@linaro.org> 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::<c_void>(), > 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
On 6/17/24 07:32, Paolo Bonzini wrote: > On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> 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::<c_void>(), > 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 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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<Box<dyn std::error::Error>>, >>>>> location: Option<(String, u32)> >>>>> } >>>>> >>>>> impl std::error::Error for Error { ... } >>>>> >>>>> impl Error { >>>>> ... >>>>> fn into_c_error(self) -> *const bindings::Error { ... } >>>>> } >>>>> >>>>> // qemu::Result<T> >>>>> type Result<T> = Result<T, Error>; >>>>> >>>>> which can be implemented without too much code. This way any "bool >>>>> f(..., Error *)" function (example: realize :)) could be implemented >>>>> as returning qemu::Result<()>. >>>> >>>> >> >
On 6/17/24 14:04, Manos Pitsidianakis wrote: > On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis >> <manos.pitsidianakis@linaro.org> 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::<c_void>(), >> 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 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
On Tue, Jun 18, 2024 at 1:33 AM Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote: > > On 6/17/24 14:04, Manos Pitsidianakis wrote: > > On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis > >> <manos.pitsidianakis@linaro.org> 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::<c_void>(), > >> 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.x96k6aeu0rpw@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 code; so there are no memory errors that can be eliminated by the rewrite. In fact, new memory errors can also be introduced, because safe Rust has stricter aliasing rules than C. This is not a problem! It's just that we need to be realistic to understand where to focus next and why. To build our path. (Also, a small correction so that we're on the same page on the fix: it's reentrancy, not concurrency). Paolo
Il lun 17 giu 2024, 23:45 Manos Pitsidianakis <manos.pitsidianakis@linaro.org> 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<T> for an active &UnsafeCell<T>. 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=NA@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 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. I can ask the same though. Please take the time to understand that I am not being dismissive! My position is exactly "yes but no". Yes, this is a great way to introduce Rust in the code base. No, this is not a sustainable way to mix Rust and C—but I am willing to help. Paolo > > Sincerely thank you in advance, > Manos >
On Mon, Jun 17, 2024 at 01:32:54PM +0200, Paolo Bonzini wrote: > Il lun 17 giu 2024, 10:59 Manos Pitsidianakis < > manos.pitsidianakis@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
On Tue, Jun 18, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> 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
On Tue, 18 Jun 2024 at 10:30, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On Tue, Jun 18, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> 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
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 0000000000..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 0000000000..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 0000000000..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 0000000000..db74f2b59f > --- /dev/null > +++ b/rust/pl011/Cargo.toml > @@ -0,0 +1,66 @@ > +[package] > +name = "pl011" > +version = "0.1.0" > +edition = "2021" > +authors = ["Manos Pitsidianakis <manos.pitsidianakis@linaro.org>"] > +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 0000000000..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 120000 > index 0000000000..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 0000000000..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~
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=vf6SbzA@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 0000000000..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" } >> +bilge-impl = { version = "0.2.0" } This one has to be included, but it is less restrictive than it seems. It is equivalent to arbitrary-int = { version = ">=1.2.7, <2.0.0" } bilge = { version = ">=0.2.0, <0.3.0" } bilge-impl = { version = ">=0.2.0, <0.3.0" } That is, it assumes an API breakage when the first nonzero component changes in the version. It is also possible to put a caret in front of the version, so that it's clearer that this is a range; but the behavior is the same. Paolo
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=vf6SbzA@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 0000000000..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
Il mer 19 giu 2024, 18:54 Daniel P. Berrangé <berrange@redhat.com> 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 :| > >
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 0000000000..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
Hey Zhao, On Thu, 11 Jul 2024 at 07:05, Zhao Liu <zhao1.liu@intel.com> 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 0000000000..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
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 <manos.pitsidianakis@linaro.org> +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 0000000000..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 0000000000..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 0000000000..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 = "feb11e002038ad243af39c2068c8a72bcf147acf05025dcdb916fcc000adb2d8" +dependencies = [ + "itertools", + "proc-macro-error", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "either" +version = "1.12.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b" + +[[package]] +name = "itertools" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1c173a5686ce8bfa551b3563d0c2170bf24ca44da99c7ca4bfdab5418c3fe57" +dependencies = [ + "either", +] + +[[package]] +name = "pl011" +version = "0.1.0" +dependencies = [ + "arbitrary-int", + "bilge", + "bilge-impl", +] + +[[package]] +name = "proc-macro-error" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" +dependencies = [ + "proc-macro-error-attr", + "proc-macro2", + "quote", + "version_check", +] + +[[package]] +name = "proc-macro-error-attr" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" +dependencies = [ + "proc-macro2", + "quote", + "version_check", +] + +[[package]] +name = "proc-macro2" +version = "1.0.84" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec96c6a92621310b51366f1e28d05ef11489516e93be030060e5fc12024a49d6" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "quote" +version = "1.0.36" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "syn" +version = "2.0.66" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c42f3f41a2de00b01c0aaad383c5a45241efc8b2d1eda5661812fda5f3cdcff5" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "unicode-ident" +version = "1.0.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" + +[[package]] +name = "version_check" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml new file mode 100644 index 0000000000..db74f2b59f --- /dev/null +++ b/rust/pl011/Cargo.toml @@ -0,0 +1,66 @@ +[package] +name = "pl011" +version = "0.1.0" +edition = "2021" +authors = ["Manos Pitsidianakis <manos.pitsidianakis@linaro.org>"] +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" } + +[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" + +# Do not include in any global workspace +[workspace] diff --git a/rust/pl011/README.md b/rust/pl011/README.md new file mode 100644 index 0000000000..54efe3c0cb --- /dev/null +++ b/rust/pl011/README.md @@ -0,0 +1,42 @@ +# PL011 QEMU Device Model + +This library implements a device model for the PrimeCell® UART (PL011) +device in QEMU. + +The C bindings were generated for commit `01782d6b29`: + +```console +$ git describe 01782d6b29 +v9.0.0-769-g01782d6b29 +``` + +with `bindgen`, using this build target: + +```console +$ ninja aarch64-softmmu-generated.rs +``` + +## Build static lib + +```sh +cargo build --target x86_64-unknown-linux-gnu +``` + +Replace host target triplet if necessary. + +## Generate Rust documentation + +To generate docs for this crate, including private items: + +```sh +cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu +``` + +To include direct dependencies like `bilge` (bitmaps for register types): + +```sh +cargo tree --depth 1 -e normal --prefix none \ + | cut -d' ' -f1 \ + | xargs printf -- '-p %s\n' \ + | xargs cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu +``` diff --git a/rust/pl011/build.rs b/rust/pl011/build.rs new file mode 100644 index 0000000000..169516d8ab --- /dev/null +++ b/rust/pl011/build.rs @@ -0,0 +1,44 @@ +use std::{env, path::Path}; + +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"); + + let out_dir = env::var_os("OUT_DIR").unwrap(); + + if let Some(build_dir) = std::env::var_os("MESON_BUILD_ROOT") { + let mut build_dir = Path::new(&build_dir).to_path_buf(); + let mut out_dir = Path::new(&out_dir).to_path_buf(); + assert!( + build_dir.exists(), + "MESON_BUILD_ROOT value does not exist on filesystem: {}", + build_dir.display() + ); + assert!( + build_dir.is_dir(), + "MESON_BUILD_ROOT value is not actually a directory: {}", + build_dir.display() + ); + build_dir.push("aarch64-softmmu-generated.rs"); + let generated_rs = build_dir; + assert!( + generated_rs.exists(), + "MESON_BUILD_ROOT/aarch64-softmmu-generated.rs does not exist on filesystem: {}", + generated_rs.display() + ); + assert!( + generated_rs.is_file(), + "MESON_BUILD_ROOT/aarch64-softmmu-generated.rs is not a file: {}", + generated_rs.display() + ); + out_dir.push("generated.rs"); + std::fs::copy(generated_rs, out_dir).unwrap(); + println!("cargo::rustc-cfg=MESON_GENERATED_RS"); + } else if !Path::new("src/generated.rs.inc").exists() { + panic!( + "No generated C bindings found! Either build them manually with bindgen or with meson \ + (`ninja aarch64-softmmu-generated.rs`) and copy them to src/generated.rs.inc, or build through meson." + ); + } +} diff --git a/rust/pl011/deny.toml b/rust/pl011/deny.toml new file mode 100644 index 0000000000..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", +] +#exclude = [] +all-features = false +no-default-features = false +#features = [] + +[output] +feature-depth = 1 + +[advisories] +db-path = "$CARGO_HOME/advisory-dbs" +db-urls = ["https://github.com/rustsec/advisory-db"] +ignore = [] + +[licenses] +allow = [ + "GPL-2.0", + "MIT", + "Apache-2.0", + "Unicode-DFS-2016", +] +confidence-threshold = 0.8 +exceptions = [] + +[licenses.private] +ignore = false +registries = [] + +[bans] +multiple-versions = "warn" +wildcards = "deny" +# The graph highlighting used when creating dotgraphs for crates +# with multiple versions +# * lowest-version - The path to the lowest versioned duplicate is highlighted +# * simplest-path - The path to the version with the fewest edges is highlighted +# * all - Both lowest-version and simplest-path are used +highlight = "all" +workspace-default-features = "allow" +external-default-features = "allow" +allow = [] +deny = [] + +[sources] +unknown-registry = "deny" +unknown-git = "deny" +allow-registry = ["https://github.com/rust-lang/crates.io-index"] +allow-git = [] diff --git a/rust/pl011/meson.build b/rust/pl011/meson.build new file mode 100644 index 0000000000..70d1ca1148 --- /dev/null +++ b/rust/pl011/meson.build @@ -0,0 +1,7 @@ +rust_pl011 = { + 'name': 'rust_pl011_cargo', + 'dirname': 'pl011', + 'output': 'libpl011.a', + } + +rust_hw_target_list += {'aarch64-softmmu': [rust_pl011]} diff --git a/rust/pl011/rustfmt.toml b/rust/pl011/rustfmt.toml new file mode 120000 index 0000000000..39f97b043b --- /dev/null +++ b/rust/pl011/rustfmt.toml @@ -0,0 +1 @@ +../rustfmt.toml \ No newline at end of file diff --git a/rust/pl011/src/definitions.rs b/rust/pl011/src/definitions.rs new file mode 100644 index 0000000000..f02604c114 --- /dev/null +++ b/rust/pl011/src/definitions.rs @@ -0,0 +1,95 @@ +//! Definitions required by QEMU when registering the device. + +use core::{ffi::CStr, mem::MaybeUninit, ptr::NonNull}; + +use crate::{device::PL011State, device_class::pl011_class_init, generated::*}; + +pub const TYPE_PL011: &CStr = unsafe { CStr::from_bytes_with_nul_unchecked(b"x-pl011-rust\0") }; + +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::<PL011State>(), + instance_align: core::mem::align_of::<PL011State>(), + 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(), +}; + +pub const VMSTATE_PL011: VMStateDescription = VMStateDescription { + name: PL011_ARM_INFO.name, + unmigratable: true, + ..unsafe { MaybeUninit::<VMStateDescription>::zeroed().assume_init() } +}; +//version_id : 2, +//minimum_version_id : 2, +//post_load : pl011_post_load, +//fields = (const VMStateField[]) { +// VMSTATE_UINT32(readbuff, PL011State), +// VMSTATE_UINT32(flags, PL011State), +// VMSTATE_UINT32(lcr, PL011State), +// VMSTATE_UINT32(rsr, PL011State), +// VMSTATE_UINT32(cr, PL011State), +// VMSTATE_UINT32(dmacr, PL011State), +// VMSTATE_UINT32(int_enabled, PL011State), +// VMSTATE_UINT32(int_level, PL011State), +// VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH), +// VMSTATE_UINT32(ilpr, PL011State), +// VMSTATE_UINT32(ibrd, PL011State), +// VMSTATE_UINT32(fbrd, PL011State), +// VMSTATE_UINT32(ifl, PL011State), +// VMSTATE_INT32(read_pos, PL011State), +// VMSTATE_INT32(read_count, PL011State), +// VMSTATE_INT32(read_trigger, PL011State), +// VMSTATE_END_OF_LIST() +//}, +//.subsections = (const VMStateDescription * const []) { +// &vmstate_pl011_clock, +// NULL +//} + +#[no_mangle] +pub unsafe extern "C" fn pl011_init(obj: *mut Object) { + assert!(!obj.is_null()); + let mut state = NonNull::new_unchecked(obj.cast::<PL011State>()); + state.as_mut().init(); +} + +use crate::generated::module_init_type_MODULE_INIT_QOM; + +#[macro_export] +macro_rules! module_init { + ($func:expr, $type:expr) => { + #[cfg_attr(target_os = "linux", link_section = ".ctors")] + #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] + #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] + pub static LOAD_MODULE: extern "C" fn() = { + assert!($type < $crate::generated::module_init_type_MODULE_INIT_MAX); + + extern "C" fn __load() { + // ::std::panic::set_hook(::std::boxed::Box::new(|_| {})); + + unsafe { + $crate::generated::register_module_init(Some($func), $type); + } + } + + __load + }; + }; +} + +#[no_mangle] +unsafe extern "C" fn register_type() { + crate::generated::type_register_static(&PL011_ARM_INFO); +} + +module_init! { + register_type, module_init_type_MODULE_INIT_QOM +} diff --git a/rust/pl011/src/device.rs b/rust/pl011/src/device.rs new file mode 100644 index 0000000000..405f590f03 --- /dev/null +++ b/rust/pl011/src/device.rs @@ -0,0 +1,531 @@ +use core::{ + ffi::{c_int, c_uchar, c_uint, c_void, CStr}, + mem::MaybeUninit, + ptr::{addr_of, addr_of_mut, NonNull}, +}; + +use crate::{ + definitions::PL011_ARM_INFO, + generated::{self, *}, + memory_ops::PL011_OPS, + registers::{self, Interrupt}, + RegisterOffset, +}; + +static PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]; + +const DATA_BREAK: u32 = 1 << 10; + +/// QEMU sourced constant. +pub const PL011_FIFO_DEPTH: usize = 16_usize; + +#[repr(C)] +#[derive(Debug)] +/// PL011 Device Model in QEMU +pub struct PL011State { + pub parent_obj: SysBusDevice, + pub iomem: MemoryRegion, + pub readbuff: u32, + #[doc(alias = "fr")] + pub flags: registers::Flags, + #[doc(alias = "lcr")] + pub line_control: registers::LineControl, + #[doc(alias = "rsr")] + pub receive_status_error_clear: registers::ReceiveStatusErrorClear, + #[doc(alias = "cr")] + pub control: registers::Control, + pub dmacr: u32, + pub int_enabled: u32, + pub int_level: u32, + pub read_fifo: [u32; PL011_FIFO_DEPTH], + pub ilpr: u32, + pub ibrd: u32, + pub fbrd: u32, + pub ifl: u32, + pub read_pos: usize, + pub read_count: usize, + pub read_trigger: usize, + #[doc(alias = "chr")] + pub char_backend: CharBackend, + /// QEMU interrupts + /// + /// ```text + /// * sysbus MMIO region 0: device registers + /// * sysbus IRQ 0: `UARTINTR` (combined interrupt line) + /// * sysbus IRQ 1: `UARTRXINTR` (receive FIFO interrupt line) + /// * sysbus IRQ 2: `UARTTXINTR` (transmit FIFO interrupt line) + /// * sysbus IRQ 3: `UARTRTINTR` (receive timeout interrupt line) + /// * sysbus IRQ 4: `UARTMSINTR` (momem status interrupt line) + /// * sysbus IRQ 5: `UARTEINTR` (error interrupt line) + /// ``` + #[doc(alias = "irq")] + pub interrupts: [qemu_irq; 6usize], + #[doc(alias = "clk")] + pub clock: NonNull<Clock>, + #[doc(alias = "migrate_clk")] + pub migrate_clock: bool, +} + +impl PL011State { + pub fn init(&mut self) { + unsafe { + memory_region_init_io( + addr_of_mut!(self.iomem), + addr_of_mut!(*self).cast::<Object>(), + &PL011_OPS, + addr_of_mut!(*self).cast::<c_void>(), + PL011_ARM_INFO.name, + 0x1000, + ); + let sbd = addr_of_mut!(*self).cast::<SysBusDevice>(); + let dev = addr_of_mut!(*self).cast::<DeviceState>(); + sysbus_init_mmio(sbd, addr_of_mut!(self.iomem)); + for irq in self.interrupts.iter_mut() { + sysbus_init_irq(sbd, irq); + } + const CLK_NAME: &CStr = unsafe { CStr::from_bytes_with_nul_unchecked(b"clk\0") }; + self.clock = NonNull::new(qdev_init_clock_in( + dev, + CLK_NAME.as_ptr(), + None, /* pl011_clock_update */ + addr_of_mut!(*self).cast::<c_void>(), + ClockEvent_ClockUpdate, + )) + .unwrap(); + } + } + + pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 { + use RegisterOffset::*; + + match RegisterOffset::try_from(offset) { + Err(v) if (0x3f8..0x400).contains(&v) => { + u64::from(PL011_ID_ARM[((offset - 0xfe0) >> 2) as usize]) + } + Err(_) => { + // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset); + 0 + } + Ok(DR) => { + // s->flags &= ~PL011_FLAG_RXFF; + self.flags.set_receive_fifo_full(false); + let c = self.read_fifo[self.read_pos]; + if self.read_count > 0 { + self.read_count -= 1; + self.read_pos = (self.read_pos + 1) & (self.fifo_depth() - 1); + } + if self.read_count == 0 { + // self.flags |= PL011_FLAG_RXFE; + self.flags.set_receive_fifo_empty(true); + } + if self.read_count + 1 == self.read_trigger { + //self.int_level &= ~ INT_RX; + self.int_level &= !registers::INT_RX; + } + // Update error bits. + self.receive_status_error_clear = c.to_be_bytes()[3].into(); + self.update(); + unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; + c.into() + } + Ok(RSR) => u8::from(self.receive_status_error_clear).into(), + Ok(FR) => u16::from(self.flags).into(), + Ok(FBRD) => self.fbrd.into(), + Ok(ILPR) => self.ilpr.into(), + Ok(IBRD) => self.ibrd.into(), + Ok(LCR_H) => u16::from(self.line_control).into(), + Ok(CR) => { + // We exercise our self-control. + u16::from(self.control).into() + } + Ok(FLS) => self.ifl.into(), + Ok(IMSC) => self.int_enabled.into(), + Ok(RIS) => self.int_level.into(), + Ok(MIS) => u64::from(self.int_level & self.int_enabled), + Ok(ICR) => { + // "The UARTICR Register is the interrupt clear register and is write-only" + // Source: ARM DDI 0183G 3.3.13 Interrupt Clear Register, UARTICR + 0 + } + Ok(DMACR) => self.dmacr.into(), + } + } + + pub fn write(&mut self, offset: hwaddr, value: u64) { + // eprintln!("write offset {offset} value {value}"); + use RegisterOffset::*; + let value: u32 = value as u32; + match RegisterOffset::try_from(offset) { + Err(_bad_offset) => { + eprintln!("write bad offset {offset} value {value}"); + } + Ok(DR) => { + // ??? Check if transmitter is enabled. + let ch: u8 = value as u8; + // XXX this blocks entire thread. Rewrite to use + // qemu_chr_fe_write and background I/O callbacks + unsafe { + qemu_chr_fe_write_all(addr_of_mut!(self.char_backend), &ch, 1); + } + self.loopback_tx(value); + self.int_level |= registers::INT_TX; + self.update(); + } + Ok(RSR) => { + self.receive_status_error_clear = 0.into(); + } + Ok(FR) => { + // flag writes are ignored + } + Ok(ILPR) => { + self.ilpr = value; + } + Ok(IBRD) => { + self.ibrd = value; + } + Ok(FBRD) => { + self.fbrd = value; + } + Ok(LCR_H) => { + let value = value as u16; + let new_val: registers::LineControl = value.into(); + // Reset the FIFO state on FIFO enable or disable + if bool::from(self.line_control.fifos_enabled()) + ^ bool::from(new_val.fifos_enabled()) + { + self.reset_fifo(); + } + if self.line_control.send_break() ^ new_val.send_break() { + let mut break_enable: c_int = new_val.send_break().into(); + unsafe { + qemu_chr_fe_ioctl( + addr_of_mut!(self.char_backend), + CHR_IOCTL_SERIAL_SET_BREAK as i32, + addr_of_mut!(break_enable).cast::<c_void>(), + ); + } + self.loopback_break(break_enable > 0); + } + self.line_control = new_val; + self.set_read_trigger(); + } + Ok(CR) => { + // ??? Need to implement the enable bit. + let value = value as u16; + self.control = value.into(); + self.loopback_mdmctrl(); + } + Ok(FLS) => { + self.ifl = value; + self.set_read_trigger(); + } + Ok(IMSC) => { + self.int_enabled = value; + self.update(); + } + Ok(RIS) => {} + Ok(MIS) => {} + Ok(ICR) => { + self.int_level &= !value; + self.update(); + } + Ok(DMACR) => { + self.dmacr = value; + if value & 3 > 0 { + // qemu_log_mask(LOG_UNIMP, "pl011: DMA not implemented\n"); + eprintln!("pl011: DMA not implemented"); + } + } + } + } + + #[inline] + fn loopback_tx(&mut self, value: u32) { + if !self.loopback_enabled() { + return; + } + + // Caveat: + // + // In real hardware, TX loopback happens at the serial-bit level + // and then reassembled by the RX logics back into bytes and placed + // into the RX fifo. That is, loopback happens after TX fifo. + // + // Because the real hardware TX fifo is time-drained at the frame + // rate governed by the configured serial format, some loopback + // bytes in TX fifo may still be able to get into the RX fifo + // that could be full at times while being drained at software + // pace. + // + // In such scenario, the RX draining pace is the major factor + // deciding which loopback bytes get into the RX fifo, unless + // hardware flow-control is enabled. + // + // For simplicity, the above described is not emulated. + self.put_fifo(value); + } + + fn loopback_mdmctrl(&mut self) { + if !self.loopback_enabled() { + return; + } + + /* + * Loopback software-driven modem control outputs to modem status inputs: + * FR.RI <= CR.Out2 + * FR.DCD <= CR.Out1 + * FR.CTS <= CR.RTS + * FR.DSR <= CR.DTR + * + * The loopback happens immediately even if this call is triggered + * by setting only CR.LBE. + * + * CTS/RTS updates due to enabled hardware flow controls are not + * dealt with here. + */ + + //fr = s->flags & ~(PL011_FLAG_RI | PL011_FLAG_DCD | + // PL011_FLAG_DSR | PL011_FLAG_CTS); + //fr |= (cr & CR_OUT2) ? PL011_FLAG_RI : 0; + //fr |= (cr & CR_OUT1) ? PL011_FLAG_DCD : 0; + //fr |= (cr & CR_RTS) ? PL011_FLAG_CTS : 0; + //fr |= (cr & CR_DTR) ? PL011_FLAG_DSR : 0; + // + self.flags.set_ring_indicator(self.control.out_2()); + self.flags.set_data_carrier_detect(self.control.out_1()); + self.flags.set_clear_to_send(self.control.request_to_send()); + self.flags + .set_data_set_ready(self.control.data_transmit_ready()); + + // Change interrupts based on updated FR + let mut il = self.int_level; + + il &= !Interrupt::MS; + //il |= (fr & PL011_FLAG_DSR) ? INT_DSR : 0; + //il |= (fr & PL011_FLAG_DCD) ? INT_DCD : 0; + //il |= (fr & PL011_FLAG_CTS) ? INT_CTS : 0; + //il |= (fr & PL011_FLAG_RI) ? INT_RI : 0; + + if self.flags.data_set_ready() { + il |= Interrupt::DSR as u32; + } + if self.flags.data_carrier_detect() { + il |= Interrupt::DCD as u32; + } + if self.flags.clear_to_send() { + il |= Interrupt::CTS as u32; + } + if self.flags.ring_indicator() { + il |= Interrupt::RI as u32; + } + self.int_level = il; + self.update(); + } + + fn loopback_break(&mut self, enable: bool) { + if enable { + self.loopback_tx(DATA_BREAK); + } + } + + fn set_read_trigger(&mut self) { + //#if 0 + // /* The docs say the RX interrupt is triggered when the FIFO exceeds + // the threshold. However linux only reads the FIFO in response to an + // interrupt. Triggering the interrupt when the FIFO is non-empty seems + // to make things work. */ + // if (s->lcr & LCR_FEN) + // s->read_trigger = (s->ifl >> 1) & 0x1c; + // else + //#endif + self.read_trigger = 1; + } + + 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::<c_void>(), + core::ptr::null_mut(), + true, + ); + } + } + + pub fn reset(&mut self) { + self.line_control.reset(); + self.receive_status_error_clear.reset(); + self.dmacr = 0; + self.int_enabled = 0; + self.int_level = 0; + self.ilpr = 0; + self.ibrd = 0; + self.fbrd = 0; + self.read_trigger = 1; + self.ifl = 0x12; + self.control.reset(); + self.flags = 0.into(); + self.reset_fifo(); + } + + pub fn reset_fifo(&mut self) { + self.read_count = 0; + self.read_pos = 0; + + /* Reset FIFO flags */ + self.flags.reset(); + } + + pub fn can_receive(&self) -> bool { + // trace_pl011_can_receive(s->lcr, s->read_count, r); + self.read_count < self.fifo_depth() + } + + pub fn event(&mut self, event: QEMUChrEvent) { + if event == generated::QEMUChrEvent_CHR_EVENT_BREAK && !self.fifo_enabled() { + self.put_fifo(DATA_BREAK); + self.receive_status_error_clear.set_break_error(true); + } + } + + #[inline] + pub fn fifo_enabled(&self) -> bool { + matches!(self.line_control.fifos_enabled(), registers::Mode::FIFO) + } + + #[inline] + pub fn loopback_enabled(&self) -> bool { + self.control.enable_loopback() + } + + #[inline] + pub fn fifo_depth(&self) -> usize { + // Note: FIFO depth is expected to be power-of-2 + if self.fifo_enabled() { + return PL011_FIFO_DEPTH; + } + 1 + } + + pub fn put_fifo(&mut self, value: c_uint) { + let depth = self.fifo_depth(); + assert!(depth > 0); + let slot = (self.read_pos + self.read_count) & (depth - 1); + self.read_fifo[slot] = value; + self.read_count += 1; + // s->flags &= ~PL011_FLAG_RXFE; + self.flags.set_receive_fifo_empty(false); + if self.read_count == depth { + //s->flags |= PL011_FLAG_RXFF; + self.flags.set_receive_fifo_full(true); + } + + if self.read_count == self.read_trigger { + self.int_level |= registers::INT_RX; + self.update(); + } + } + + pub fn update(&mut self) { + let flags = self.int_level & self.int_enabled; + for (irq, i) in self.interrupts.iter().zip(IRQMASK) { + unsafe { qemu_set_irq(*irq, i32::from(flags & i != 0)) }; + } + } +} + +/// Which bits in the interrupt status matter for each outbound IRQ line ? +pub const IRQMASK: [u32; 6] = [ + /* combined IRQ */ + Interrupt::E + | Interrupt::MS + | Interrupt::RT as u32 + | Interrupt::TX as u32 + | Interrupt::RX as u32, + Interrupt::RX as u32, + Interrupt::TX as u32, + Interrupt::RT as u32, + Interrupt::MS, + Interrupt::E, +]; + +pub unsafe extern "C" fn pl011_can_receive(opaque: *mut c_void) -> c_int { + assert!(!opaque.is_null()); + let state = NonNull::new_unchecked(opaque.cast::<PL011State>()); + state.as_ref().can_receive().into() +} +pub unsafe extern "C" fn pl011_receive( + opaque: *mut core::ffi::c_void, + buf: *const u8, + size: core::ffi::c_int, +) { + assert!(!opaque.is_null()); + let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>()); + if state.as_ref().loopback_enabled() { + return; + } + if size > 0 { + assert!(!buf.is_null()); + state.as_mut().put_fifo(*buf.cast::<c_uint>()) + } +} + +pub unsafe extern "C" fn pl011_event(opaque: *mut core::ffi::c_void, event: QEMUChrEvent) { + assert!(!opaque.is_null()); + let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>()); + state.as_mut().event(event) +} + +pub const VMSTATE_PL011: VMStateDescription = VMStateDescription { + name: PL011_ARM_INFO.name, + unmigratable: true, + ..unsafe { MaybeUninit::<VMStateDescription>::zeroed().assume_init() } +}; +//version_id : 2, +//minimum_version_id : 2, +//post_load : pl011_post_load, +//fields = (const VMStateField[]) { +// VMSTATE_UINT32(readbuff, PL011State), +// VMSTATE_UINT32(flags, PL011State), +// VMSTATE_UINT32(lcr, PL011State), +// VMSTATE_UINT32(rsr, PL011State), +// VMSTATE_UINT32(cr, PL011State), +// VMSTATE_UINT32(dmacr, PL011State), +// VMSTATE_UINT32(int_enabled, PL011State), +// VMSTATE_UINT32(int_level, PL011State), +// VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH), +// VMSTATE_UINT32(ilpr, PL011State), +// VMSTATE_UINT32(ibrd, PL011State), +// VMSTATE_UINT32(fbrd, PL011State), +// VMSTATE_UINT32(ifl, PL011State), +// VMSTATE_INT32(read_pos, PL011State), +// VMSTATE_INT32(read_count, PL011State), +// VMSTATE_INT32(read_trigger, PL011State), +// VMSTATE_END_OF_LIST() +//}, +//.subsections = (const VMStateDescription * const []) { +// &vmstate_pl011_clock, +// NULL +//} + +pub unsafe extern "C" fn pl011_create( + addr: u64, + irq: qemu_irq, + chr: *mut Chardev, +) -> *mut DeviceState { + let dev: *mut DeviceState = unsafe { qdev_new(PL011_ARM_INFO.name) }; + assert!(!dev.is_null()); + let sysbus: *mut SysBusDevice = dev as *mut SysBusDevice; + + unsafe { + qdev_prop_set_chr(dev, generated::TYPE_CHARDEV.as_ptr(), chr); + sysbus_realize_and_unref(sysbus, addr_of!(error_fatal) as *mut *mut Error); + sysbus_mmio_map(sysbus, 0, addr); + sysbus_connect_irq(sysbus, 0, irq); + } + dev +} diff --git a/rust/pl011/src/device_class.rs b/rust/pl011/src/device_class.rs new file mode 100644 index 0000000000..50d91eb527 --- /dev/null +++ b/rust/pl011/src/device_class.rs @@ -0,0 +1,95 @@ +use core::{mem::MaybeUninit, ptr::NonNull}; +use std::sync::OnceLock; + +use crate::{ + device::{PL011State, VMSTATE_PL011}, + generated::*, +}; + +#[no_mangle] +pub unsafe extern "C" fn pl011_class_init(klass: *mut ObjectClass, _: *mut core::ffi::c_void) { + let mut dc = NonNull::new(klass.cast::<DeviceClass>()).unwrap(); + dc.as_mut().realize = Some(pl011_realize); + dc.as_mut().reset = Some(pl011_reset); + dc.as_mut().vmsd = &VMSTATE_PL011; + _ = PL011_PROPERTIES.get_or_init(make_pl011_properties); + device_class_set_props( + dc.as_mut(), + PL011_PROPERTIES.get_mut().unwrap().as_mut_ptr(), + ); +} + +#[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(), + } + }; + ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr) => { + $crate::generated::Property { + name: $name, + info: $prop, + offset: ::core::mem::offset_of!($state, $field) as _, + bitnr: 0, + bitmask: 0, + set_default: false, + defval: $crate::generated::Property__bindgen_ty_1 { i: 0 }, + arrayoffset: 0, + arrayinfo: ::core::ptr::null(), + arrayfieldsize: 0, + link_type: ::core::ptr::null(), + } + }; +} + +#[no_mangle] +pub static mut PL011_PROPERTIES: OnceLock<[Property; 3]> = OnceLock::new(); + +unsafe impl Send for Property {} +unsafe impl Sync for Property {} + +#[no_mangle] +fn make_pl011_properties() -> [Property; 3] { + [ + define_property!( + c"chardev".as_ptr(), + PL011State, + char_backend, + unsafe { &qdev_prop_chr }, + CharBackend + ), + define_property!( + c"migrate-clk".as_ptr(), + PL011State, + migrate_clock, + unsafe { &qdev_prop_bool }, + bool + ), + unsafe { MaybeUninit::<Property>::zeroed().assume_init() }, + ] +} + +#[no_mangle] +pub unsafe extern "C" fn pl011_realize(dev: *mut DeviceState, _errp: *mut *mut Error) { + assert!(!dev.is_null()); + let mut state = NonNull::new_unchecked(dev.cast::<PL011State>()); + state.as_mut().realize(); +} + +#[no_mangle] +pub unsafe extern "C" fn pl011_reset(dev: *mut DeviceState) { + assert!(!dev.is_null()); + let mut state = NonNull::new_unchecked(dev.cast::<PL011State>()); + state.as_mut().reset(); +} diff --git a/rust/pl011/src/generated.rs b/rust/pl011/src/generated.rs new file mode 100644 index 0000000000..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"); diff --git a/rust/pl011/src/lib.rs b/rust/pl011/src/lib.rs new file mode 100644 index 0000000000..bb2899dbc2 --- /dev/null +++ b/rust/pl011/src/lib.rs @@ -0,0 +1,581 @@ +// PL011 QEMU Device Model +// +// This library implements a device model for the PrimeCell® UART (PL011) +// device in QEMU. +// +#![doc = include_str!("../README.md")] +//! # Library crate +//! +//! See [`PL011State`](crate::device::PL011State) for the device model type and +//! the [`registers`] module for register types. + +// FIXME: remove improper_ctypes +#[allow( + improper_ctypes_definitions, + improper_ctypes, + non_camel_case_types, + non_snake_case, + non_upper_case_globals +)] +#[allow( + clippy::missing_const_for_fn, + clippy::useless_transmute, + clippy::too_many_arguments, + clippy::approx_constant, + clippy::use_self, + clippy::cast_lossless, +)] +#[rustfmt::skip] +pub mod generated; + +pub mod definitions; +pub mod device; +pub mod device_class; +pub mod memory_ops; + +/// Offset of each register from the base memory address of the device. +/// +/// # Source +/// ARM DDI 0183G, Table 3-1 p.3-3 +#[doc(alias = "offset")] +#[allow(non_camel_case_types)] +#[repr(u64)] +#[derive(Debug)] +pub enum RegisterOffset { + /// Data Register + /// + /// A write to this register initiates the actual data transmission + #[doc(alias = "UARTDR")] + DR = 0x000, + /// Receive Status Register or Error Clear Register + #[doc(alias = "UARTRSR")] + #[doc(alias = "UARTECR")] + RSR = 0x004, + /// Flag Register + /// + /// A read of this register shows if transmission is complete + #[doc(alias = "UARTFR")] + FR = 0x018, + /// Fractional Baud Rate Register + /// + /// responsible for baud rate speed + #[doc(alias = "UARTFBRD")] + FBRD = 0x028, + /// `IrDA` Low-Power Counter Register + #[doc(alias = "UARTILPR")] + ILPR = 0x020, + /// Integer Baud Rate Register + /// + /// Responsible for baud rate speed + #[doc(alias = "UARTIBRD")] + IBRD = 0x024, + /// line control register (data frame format) + #[doc(alias = "UARTLCR_H")] + LCR_H = 0x02C, + /// Toggle UART, transmission or reception + #[doc(alias = "UARTCR")] + CR = 0x030, + /// Interrupt FIFO Level Select Register + #[doc(alias = "UARTIFLS")] + FLS = 0x034, + /// Interrupt Mask Set/Clear Register + #[doc(alias = "UARTIMSC")] + IMSC = 0x038, + /// Raw Interrupt Status Register + #[doc(alias = "UARTRIS")] + RIS = 0x03C, + /// Masked Interrupt Status Register + #[doc(alias = "UARTMIS")] + MIS = 0x040, + /// Interrupt Clear Register + #[doc(alias = "UARTICR")] + ICR = 0x044, + /// DMA control Register + #[doc(alias = "UARTDMACR")] + DMACR = 0x048, + ///// Reserved, offsets `0x04C` to `0x07C`. + //Reserved = 0x04C, +} + +impl core::convert::TryFrom<u64> for RegisterOffset { + type Error = u64; + + fn try_from(value: u64) -> Result<Self, Self::Error> { + macro_rules! case { + ($($discriminant:ident),*$(,)*) => { + /* check that matching on all macro arguments compiles, which means we are not + * missing any enum value; if the type definition ever changes this will stop + * compiling. + */ + const fn _assert_exhaustive(val: RegisterOffset) { + match val { + $(RegisterOffset::$discriminant => (),)* + } + } + + match value { + $(x if x == Self::$discriminant as u64 => Ok(Self::$discriminant),)* + _ => Err(value), + } + } + } + case! { DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR } + } +} + +pub mod registers { + //! Device registers exposed as typed structs which are backed by arbitrary + //! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc. + //! + //! All PL011 registers are essentially 32-bit wide, but are typed here as + //! bitmaps with only the necessary width. That is, if a struct bitmap + //! in this module is for example 16 bits long, it should be conceived + //! as a 32-bit register where the unmentioned higher bits are always + //! unused thus treated as zero when read or written. + use bilge::prelude::*; + + // TODO: FIFO Mode has different semantics + /// Data Register, `UARTDR` + /// + /// The `UARTDR` register is the data register. + /// + /// For words to be transmitted: + /// + /// - if the FIFOs are enabled, data written to this location is pushed onto + /// the transmit + /// FIFO + /// - if the FIFOs are not enabled, data is stored in the transmitter + /// holding register (the + /// bottom word of the transmit FIFO). + /// + /// The write operation initiates transmission from the UART. The data is + /// prefixed with a start bit, appended with the appropriate parity bit + /// (if parity is enabled), and a stop bit. The resultant word is then + /// transmitted. + /// + /// For received words: + /// + /// - if the FIFOs are enabled, the data byte and the 4-bit status (break, + /// frame, parity, + /// and overrun) is pushed onto the 12-bit wide receive FIFO + /// - if the FIFOs are not enabled, the data byte and status are stored in + /// the receiving + /// holding register (the bottom word of the receive FIFO). + /// + /// The received data byte is read by performing reads from the `UARTDR` + /// register along with the corresponding status information. The status + /// information can also be read by a read of the `UARTRSR/UARTECR` + /// register. + /// + /// # Note + /// + /// You must disable the UART before any of the control registers are + /// reprogrammed. When the UART is disabled in the middle of + /// transmission or reception, it completes the current character before + /// stopping. + /// + /// # Source + /// ARM DDI 0183G 3.3.1 Data Register, UARTDR + #[bitsize(16)] + #[derive(Clone, Copy, DebugBits, FromBits)] + #[doc(alias = "UARTDR")] + pub struct Data { + _reserved: u4, + pub data: u8, + pub framing_error: bool, + pub parity_error: bool, + pub break_error: bool, + pub overrun_error: bool, + } + + // TODO: FIFO Mode has different semantics + /// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR` + /// + /// The UARTRSR/UARTECR register is the receive status register/error clear + /// register. Receive status can also be read from the `UARTRSR` + /// register. If the status is read from this register, then the status + /// information for break, framing and parity corresponds to the + /// data character read from the [Data register](Data), `UARTDR` prior to + /// reading the UARTRSR register. The status information for overrun is + /// set immediately when an overrun condition occurs. + /// + /// + /// # Note + /// The received data character must be read first from the [Data + /// Register](Data), `UARTDR` before reading the error status associated + /// with that data character from the `UARTRSR` register. This read + /// sequence cannot be reversed, because the `UARTRSR` register is + /// updated only when a read occurs from the `UARTDR` register. However, + /// the status information can also be obtained by reading the `UARTDR` + /// register + /// + /// # Source + /// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register, + /// UARTRSR/UARTECR + #[bitsize(8)] + #[derive(Clone, Copy, DebugBits, FromBits)] + pub struct ReceiveStatusErrorClear { + pub framing_error: bool, + pub parity_error: bool, + pub break_error: bool, + pub overrun_error: bool, + _reserved_unpredictable: u4, + } + + impl ReceiveStatusErrorClear { + pub fn reset(&mut self) { + // All the bits are cleared to 0 on reset. + *self = 0.into(); + } + } + + impl Default for ReceiveStatusErrorClear { + fn default() -> Self { + 0.into() + } + } + + #[bitsize(16)] + #[derive(Clone, Copy, DebugBits, FromBits)] + /// Flag Register, `UARTFR` + #[doc(alias = "UARTFR")] + pub struct Flags { + /// CTS Clear to send. This bit is the complement of the UART clear to + /// send, `nUARTCTS`, modem status input. That is, the bit is 1 + /// when `nUARTCTS` is LOW. + pub clear_to_send: bool, + /// DSR Data set ready. This bit is the complement of the UART data set + /// ready, `nUARTDSR`, modem status input. That is, the bit is 1 when + /// `nUARTDSR` is LOW. + pub data_set_ready: bool, + /// DCD Data carrier detect. This bit is the complement of the UART data + /// carrier detect, `nUARTDCD`, modem status input. That is, the bit is + /// 1 when `nUARTDCD` is LOW. + pub data_carrier_detect: bool, + /// BUSY UART busy. If this bit is set to 1, the UART is busy + /// transmitting data. This bit remains set until the complete + /// byte, including all the stop bits, has been sent from the + /// shift register. This bit is set as soon as the transmit FIFO + /// becomes non-empty, regardless of whether the UART is enabled + /// or not. + pub busy: bool, + /// RXFE Receive FIFO empty. The meaning of this bit depends on the + /// state of the FEN bit in the UARTLCR_H register. If the FIFO + /// is disabled, this bit is set when the receive holding + /// register is empty. If the FIFO is enabled, the RXFE bit is + /// set when the receive FIFO is empty. + pub receive_fifo_empty: bool, + /// TXFF Transmit FIFO full. The meaning of this bit depends on the + /// state of the FEN bit in the UARTLCR_H register. If the FIFO + /// is disabled, this bit is set when the transmit holding + /// register is full. If the FIFO is enabled, the TXFF bit is + /// set when the transmit FIFO is full. + pub transmit_fifo_full: bool, + /// RXFF Receive FIFO full. The meaning of this bit depends on the state + /// of the FEN bit in the UARTLCR_H register. If the FIFO is + /// disabled, this bit is set when the receive holding register + /// is full. If the FIFO is enabled, the RXFF bit is set when + /// the receive FIFO is full. + pub receive_fifo_full: bool, + /// Transmit FIFO empty. The meaning of this bit depends on the state of + /// the FEN bit in the [Line Control register](LineControl), + /// `UARTLCR_H`. If the FIFO is disabled, this bit is set when the + /// transmit holding register is empty. If the FIFO is enabled, + /// the TXFE bit is set when the transmit FIFO is empty. This + /// bit does not indicate if there is data in the transmit shift + /// register. + pub transmit_fifo_empty: bool, + /// `RI`, is `true` when `nUARTRI` is `LOW`. + pub ring_indicator: bool, + _reserved_zero_no_modify: u7, + } + + impl Flags { + pub fn reset(&mut self) { + // After reset TXFF, RXFF, and BUSY are 0, and TXFE and RXFE are 1 + self.set_receive_fifo_full(false); + self.set_transmit_fifo_full(false); + self.set_busy(false); + self.set_receive_fifo_empty(true); + self.set_transmit_fifo_empty(true); + } + } + + impl Default for Flags { + fn default() -> Self { + let mut ret: Self = 0.into(); + ret.reset(); + ret + } + } + + #[bitsize(16)] + #[derive(Clone, Copy, DebugBits, FromBits)] + /// Line Control Register, `UARTLCR_H` + #[doc(alias = "UARTLCR_H")] + pub struct LineControl { + /// 15:8 - Reserved, do not modify, read as zero. + _reserved_zero_no_modify: u8, + /// 7 SPS Stick parity select. + /// 0 = stick parity is disabled + /// 1 = either: + /// • if the EPS bit is 0 then the parity bit is transmitted and checked + /// as a 1 • if the EPS bit is 1 then the parity bit is + /// transmitted and checked as a 0. This bit has no effect when + /// the PEN bit disables parity checking and generation. See Table 3-11 + /// on page 3-14 for the parity truth table. + pub sticky_parity: bool, + /// WLEN Word length. These bits indicate the number of data bits + /// transmitted or received in a frame as follows: b11 = 8 bits + /// b10 = 7 bits + /// b01 = 6 bits + /// b00 = 5 bits. + pub word_length: WordLength, + /// FEN Enable FIFOs: + /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become + /// 1-byte-deep holding registers 1 = transmit and receive FIFO + /// buffers are enabled (FIFO mode). + pub fifos_enabled: Mode, + /// 3 STP2 Two stop bits select. If this bit is set to 1, two stop bits + /// are transmitted at the end of the frame. The receive + /// logic does not check for two stop bits being received. + pub two_stops_bits: bool, + /// EPS Even parity select. Controls the type of parity the UART uses + /// during transmission and reception: + /// - 0 = odd parity. The UART generates or checks for an odd number of + /// 1s in the data and parity bits. + /// - 1 = even parity. The UART generates or checks for an even number + /// of 1s in the data and parity bits. + /// This bit has no effect when the `PEN` bit disables parity checking + /// and generation. See Table 3-11 on page 3-14 for the parity + /// truth table. + pub parity: Parity, + /// 1 PEN Parity enable: + /// + /// - 0 = parity is disabled and no parity bit added to the data frame + /// - 1 = parity checking and generation is enabled. + /// + /// See Table 3-11 on page 3-14 for the parity truth table. + pub parity_enabled: bool, + /// BRK Send break. + /// + /// If this bit is set to `1`, a low-level is continually output on the + /// `UARTTXD` output, after completing transmission of the + /// current character. For the proper execution of the break command, + /// the software must set this bit for at least two complete + /// frames. For normal use, this bit must be cleared to `0`. + pub send_break: bool, + } + + impl LineControl { + pub fn reset(&mut self) { + // All the bits are cleared to 0 when reset. + *self = 0.into(); + } + } + + impl Default for LineControl { + fn default() -> Self { + 0.into() + } + } + + #[bitsize(1)] + #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)] + /// `EPS` "Even parity select", field of [Line Control + /// register](LineControl). + pub enum Parity { + /// - 0 = odd parity. The UART generates or checks for an odd number of + /// 1s in the data and parity bits. + Odd = 0, + /// - 1 = even parity. The UART generates or checks for an even number + /// of 1s in the data and parity bits. + Even = 1, + } + + #[bitsize(1)] + #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)] + /// `FEN` "Enable FIFOs" or Device mode, field of [Line Control + /// register](LineControl). + pub enum Mode { + /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become + /// 1-byte-deep holding registers + Character = 0, + /// 1 = transmit and receive FIFO buffers are enabled (FIFO mode). + FIFO = 1, + } + + impl From<Mode> for bool { + fn from(val: Mode) -> Self { + matches!(val, Mode::FIFO) + } + } + + #[bitsize(2)] + #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)] + /// `WLEN` Word length, field of [Line Control register](LineControl). + /// + /// These bits indicate the number of data bits transmitted or received in a + /// frame as follows: + pub enum WordLength { + /// b11 = 8 bits + _8Bits = 0b11, + /// b10 = 7 bits + _7Bits = 0b10, + /// b01 = 6 bits + _6Bits = 0b01, + /// b00 = 5 bits. + _5Bits = 0b00, + } + + /// Control Register, `UARTCR` + /// + /// The `UARTCR` register is the control register. All the bits are cleared + /// to `0` on reset except for bits `9` and `8` that are set to `1`. + /// + /// # Source + /// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12 + #[bitsize(16)] + #[doc(alias = "UARTCR")] + #[derive(Clone, Copy, DebugBits, FromBits)] + pub struct Control { + /// `UARTEN` UART enable: 0 = UART is disabled. If the UART is disabled + /// in the middle of transmission or reception, it completes the current + /// character before stopping. 1 = the UART is enabled. Data + /// transmission and reception occurs for either UART signals or SIR + /// signals depending on the setting of the SIREN bit. + pub enable_uart: bool, + /// `SIREN` `SIR` enable: 0 = IrDA SIR ENDEC is disabled. `nSIROUT` + /// remains LOW (no light pulse generated), and signal transitions on + /// SIRIN have no effect. 1 = IrDA SIR ENDEC is enabled. Data is + /// transmitted and received on nSIROUT and SIRIN. UARTTXD remains HIGH, + /// in the marking state. Signal transitions on UARTRXD or modem status + /// inputs have no effect. This bit has no effect if the UARTEN bit + /// disables the UART. + pub enable_sir: bool, + /// `SIRLP` SIR low-power IrDA mode. This bit selects the IrDA encoding + /// mode. If this bit is cleared to 0, low-level bits are transmitted as + /// an active high pulse with a width of 3/ 16th of the bit period. If + /// this bit is set to 1, low-level bits are transmitted with a pulse + /// width that is 3 times the period of the IrLPBaud16 input signal, + /// regardless of the selected bit rate. Setting this bit uses less + /// power, but might reduce transmission distances. + pub sir_lowpower_irda_mode: u1, + /// Reserved, do not modify, read as zero. + _reserved_zero_no_modify: u4, + /// `LBE` Loopback enable. If this bit is set to 1 and the SIREN bit is + /// set to 1 and the SIRTEST bit in the Test Control register, UARTTCR + /// on page 4-5 is set to 1, then the nSIROUT path is inverted, and fed + /// through to the SIRIN path. The SIRTEST bit in the test register must + /// be set to 1 to override the normal half-duplex SIR operation. This + /// must be the requirement for accessing the test registers during + /// normal operation, and SIRTEST must be cleared to 0 when loopback + /// testing is finished. This feature reduces the amount of external + /// coupling required during system test. If this bit is set to 1, and + /// the SIRTEST bit is set to 0, the UARTTXD path is fed through to the + /// UARTRXD path. In either SIR mode or UART mode, when this bit is set, + /// the modem outputs are also fed through to the modem inputs. This bit + /// is cleared to 0 on reset, to disable loopback. + pub enable_loopback: bool, + /// `TXE` Transmit enable. If this bit is set to 1, the transmit section + /// of the UART is enabled. Data transmission occurs for either UART + /// signals, or SIR signals depending on the setting of the SIREN bit. + /// When the UART is disabled in the middle of transmission, it + /// completes the current character before stopping. + pub enable_transmit: bool, + /// `RXE` Receive enable. If this bit is set to 1, the receive section + /// of the UART is enabled. Data reception occurs for either UART + /// signals or SIR signals depending on the setting of the SIREN bit. + /// When the UART is disabled in the middle of reception, it completes + /// the current character before stopping. + pub enable_receive: bool, + /// `DTR` Data transmit ready. This bit is the complement of the UART + /// data transmit ready, `nUARTDTR`, modem status output. That is, when + /// the bit is programmed to a 1 then `nUARTDTR` is LOW. + pub data_transmit_ready: bool, + /// `RTS` Request to send. This bit is the complement of the UART + /// request to send, `nUARTRTS`, modem status output. That is, when the + /// bit is programmed to a 1 then `nUARTRTS` is LOW. + pub request_to_send: bool, + /// `Out1` This bit is the complement of the UART Out1 (`nUARTOut1`) + /// modem status output. That is, when the bit is programmed to a 1 the + /// output is 0. For DTE this can be used as Data Carrier Detect (DCD). + pub out_1: bool, + /// `Out2` This bit is the complement of the UART Out2 (`nUARTOut2`) + /// modem status output. That is, when the bit is programmed to a 1, the + /// output is 0. For DTE this can be used as Ring Indicator (RI). + pub out_2: bool, + /// `RTSEn` RTS hardware flow control enable. If this bit is set to 1, + /// RTS hardware flow control is enabled. Data is only requested when + /// there is space in the receive FIFO for it to be received. + pub rts_hardware_flow_control_enable: bool, + /// `CTSEn` CTS hardware flow control enable. If this bit is set to 1, + /// CTS hardware flow control is enabled. Data is only transmitted when + /// the `nUARTCTS` signal is asserted. + pub cts_hardware_flow_control_enable: bool, + } + + impl Control { + pub fn reset(&mut self) { + *self = 0.into(); + self.set_enable_receive(true); + self.set_enable_transmit(true); + } + } + + impl Default for Control { + fn default() -> Self { + let mut ret: Self = 0.into(); + ret.reset(); + ret + } + } + + /// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC + pub const INT_OE: u32 = 1 << 10; + pub const INT_BE: u32 = 1 << 9; + pub const INT_PE: u32 = 1 << 8; + pub const INT_FE: u32 = 1 << 7; + pub const INT_RT: u32 = 1 << 6; + pub const INT_TX: u32 = 1 << 5; + pub const INT_RX: u32 = 1 << 4; + pub const INT_DSR: u32 = 1 << 3; + pub const INT_DCD: u32 = 1 << 2; + pub const INT_CTS: u32 = 1 << 1; + pub const INT_RI: u32 = 1 << 0; + pub const INT_E: u32 = INT_OE | INT_BE | INT_PE | INT_FE; + pub const INT_MS: u32 = INT_RI | INT_DSR | INT_DCD | INT_CTS; + + #[repr(u32)] + pub enum Interrupt { + OE = 1 << 10, + BE = 1 << 9, + PE = 1 << 8, + FE = 1 << 7, + RT = 1 << 6, + TX = 1 << 5, + RX = 1 << 4, + DSR = 1 << 3, + DCD = 1 << 2, + CTS = 1 << 1, + RI = 1 << 0, + } + + impl Interrupt { + pub const E: u32 = INT_OE | INT_BE | INT_PE | INT_FE; + pub const MS: u32 = INT_RI | INT_DSR | INT_DCD | INT_CTS; + } +} + +// State +// TODO You must disable the UART before any of the control registers are +// reprogrammed. When the UART is disabled in the middle of transmission or +// reception, it completes the current character before stopping + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn it_works() {} +} diff --git a/rust/pl011/src/memory_ops.rs b/rust/pl011/src/memory_ops.rs new file mode 100644 index 0000000000..4c7d4577c7 --- /dev/null +++ b/rust/pl011/src/memory_ops.rs @@ -0,0 +1,38 @@ +use core::{mem::MaybeUninit, ptr::NonNull}; + +use crate::{device::PL011State, generated::*}; + +pub const PL011_OPS: MemoryRegionOps = MemoryRegionOps { + read: Some(pl011_read), + write: Some(pl011_write), + read_with_attrs: None, + write_with_attrs: None, + endianness: device_endian_DEVICE_NATIVE_ENDIAN, + valid: unsafe { MaybeUninit::<MemoryRegionOps__bindgen_ty_1>::zeroed().assume_init() }, + impl_: MemoryRegionOps__bindgen_ty_2 { + min_access_size: 4, + max_access_size: 4, + ..unsafe { MaybeUninit::<MemoryRegionOps__bindgen_ty_2>::zeroed().assume_init() } + }, +}; + +unsafe extern "C" fn pl011_read( + opaque: *mut core::ffi::c_void, + addr: hwaddr, + size: core::ffi::c_uint, +) -> u64 { + assert!(!opaque.is_null()); + let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>()); + state.as_mut().read(addr, size) +} + +unsafe extern "C" fn pl011_write( + opaque: *mut core::ffi::c_void, + addr: hwaddr, + data: u64, + _size: core::ffi::c_uint, +) { + assert!(!opaque.is_null()); + let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>()); + state.as_mut().write(addr, data) +} diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml new file mode 100644 index 0000000000..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
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 <manos.pitsidianakis@linaro.org> --- 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 120000 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