| Message ID | 20241024-rust-round-2-v1-0-051e7a25b978@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rust device model patches and misc cleanups | expand |
On 10/24/24 16:02, Manos Pitsidianakis wrote: > Hello everyone, the pathological corrosion of QEMU code continues. > This series expands the device model harness work performed in the > initial Rust work from the previous month. In particular: Wow, there's a lot of stuff here! The very good news is that it's basically all the code that is needed to get CI running, after which we can start with the fun stuff. At the same time, "the fun stuff" is also the one that risks introducing technical debt, so we need to switch to quality-over-quantity mode and have a serious design discussion about it. I'll do that later as a reply to the patches. > Code and functionality duplication is not fun, and pl011 was mostly > done as a proof of concept for a Rust device because of its small > complexity. As of this moment we have not decided on a policy for how > to handle these things and it is not in **scope for this patch series > anyway**. That's fine. Looking at the currently posted series, it seems that we have three main themes: 1) small scale cleanups: duplicated and useless code, improved testing. These are in https://lore.kernel.org/qemu-devel/20241021163538.136941-1-pbonzini@redhat.com/T/ and they have been reviewed already. But these two: > Revert "rust: add PL011 device model" > rust: add PL011 device model ... should definitely be moved on top to clean up the authorship in "git blame" and other similar tools. Sorry about that. 2) allow using older rustc/bindgen, extend CI to cover it. This is https://lore.kernel.org/qemu-devel/20241022100956.196657-1-pbonzini@redhat.com/T/ which still needs review. These five: > rust: add support for migration in device models > rust/pl011: move CLK_NAME static to function scope > rust/pl011: add TYPE_PL011_LUMINARY device > rust/pl011: remove commented out C code > rust/pl011: Use correct masks for IBRD and FBRD (minus the usage of #[derive()] should be included in that series, so that qtests pass. It's not a huge amount of work and I can extract it, of course with proper attribution/authorship. The rest are future work: > rust/qemu-api-macros: introduce Device proc macro This is useful as a starting point but it has the limit of being very device-specific. This is of course okay with properties and vmstate, but in my opinion the implementation of class_init should be as generic as possible, and not too specialized for methods in Object or Device. As I said above, we first need to agree on the design. > rust/pl011: move pub callback decl to local scope This depends a lot on how we go implementing bindings to chardev. For example if the callbacks turn out to be a trait, it would have to be undone. Possibly the C callback wrappers would move to rust/qemu-api/chardev. For now I'd leave it aside. > rust/qemu-api: add log module > rust/pl011: log guest/unimp errors This also needs design discussion. Do we want the API to be the same as C, i.e. keep the qemu_* prefix? Do we want wrapper macros that include the format!() call? You also have "pub enum LogMask" to work around the fact that log masks are preprocessor macros. Is that okay, or do we want to modify C code to make the bindings nicer? I for example would prefer the latter and then declaring LogMask as a bitfield in bindgen. Thanks, Paolo > > rust/wrapper.h | 1 + > rust/hw/char/pl011/src/device.rs | 419 +++++++++++++++++--------- > rust/hw/char/pl011/src/device_class.rs | 70 ----- > rust/hw/char/pl011/src/lib.rs | 2 +- > rust/qemu-api-macros/src/device.rs | 370 +++++++++++++++++++++++ > rust/qemu-api-macros/src/lib.rs | 46 +-- > rust/qemu-api-macros/src/object.rs | 107 +++++++ > rust/qemu-api-macros/src/symbols.rs | 57 ++++ > rust/qemu-api-macros/src/utilities.rs | 152 ++++++++++ > rust/qemu-api-macros/src/vmstate.rs | 113 +++++++ > rust/qemu-api/meson.build | 5 +- > rust/qemu-api/src/definitions.rs | 97 ------ > rust/qemu-api/src/device_class.rs | 128 -------- > rust/qemu-api/src/lib.rs | 10 +- > rust/qemu-api/src/log.rs | 140 +++++++++ > rust/qemu-api/src/objects.rs | 90 ++++++ > rust/qemu-api/src/tests.rs | 49 --- > rust/qemu-api/src/vmstate.rs | 403 +++++++++++++++++++++++++ > subprojects/packagefiles/syn-2-rs/meson.build | 1 + > 19 files changed, 1726 insertions(+), 534 deletions(-) > --- > base-commit: 55522f72149fbf95ee3b057f1419da0cad46d0dd > change-id: 20241024-rust-round-2-69fa10c9a0c9 > > -- > γαῖα πυρί μιχθήτω > > >
Hi Paolo, Please reply with review comments underneath individual patches, this is hard to follow and I might miss some points. On Fri, 25 Oct 2024 at 12:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 10/24/24 16:02, Manos Pitsidianakis wrote: > > Hello everyone, the pathological corrosion of QEMU code continues. > > This series expands the device model harness work performed in the > > initial Rust work from the previous month. In particular: > > Wow, there's a lot of stuff here! > > The very good news is that it's basically all the code that is needed to > get CI running, after which we can start with the fun stuff. At the > same time, "the fun stuff" is also the one that risks introducing > technical debt, so we need to switch to quality-over-quantity mode and > have a serious design discussion about it. I'll do that later as a > reply to the patches. > > > Code and functionality duplication is not fun, and pl011 was mostly > > done as a proof of concept for a Rust device because of its small > > complexity. As of this moment we have not decided on a policy for how > > to handle these things and it is not in **scope for this patch series > > anyway**. > > That's fine. > > Looking at the currently posted series, it seems that we have three main > themes: > > 1) small scale cleanups: duplicated and useless code, improved testing. > These are in > https://lore.kernel.org/qemu-devel/20241021163538.136941-1-pbonzini@redhat.com/T/ > and they have been reviewed already. But these two: > > > Revert "rust: add PL011 device model" > > rust: add PL011 device model > > ... should definitely be moved on top to clean up the authorship in "git > blame" and other similar tools. Sorry about that. I will send them on a separate series and merge them from my tree when reviewed. > > 2) allow using older rustc/bindgen, extend CI to cover it. This is > https://lore.kernel.org/qemu-devel/20241022100956.196657-1-pbonzini@redhat.com/T/ > which still needs review. These five: > > > rust: add support for migration in device models > > rust/pl011: move CLK_NAME static to function scope > > rust/pl011: add TYPE_PL011_LUMINARY device > > rust/pl011: remove commented out C code > > rust/pl011: Use correct masks for IBRD and FBRD > > (minus the usage of #[derive()] should be included in that series, so > that qtests pass. It's not a huge amount of work and I can extract it, > of course with proper attribution/authorship. These are independent from CI; i.e. you can merge your CI patches after those. > > The rest are future work: > > > rust/qemu-api-macros: introduce Device proc macro > > This is useful as a starting point but it has the limit of being very > device-specific. This is of course okay with properties and vmstate, > but in my opinion the implementation of class_init should be as generic > as possible, and not too specialized for methods in Object or Device. > > As I said above, we first need to agree on the design. Post your review under the patches please, > > > rust/pl011: move pub callback decl to local scope > > This depends a lot on how we go implementing bindings to chardev. For > example if the callbacks turn out to be a trait, it would have to be > undone. Possibly the C callback wrappers would move to > rust/qemu-api/chardev. For now I'd leave it aside. This patch moves the callbacks scope from public to inside the function, no functional change related. It doesn't change or have anything to do with chardev interfaces > > > rust/qemu-api: add log module > > rust/pl011: log guest/unimp errors > > This also needs design discussion. Do we want the API to be the same as > C, i.e. keep the qemu_* prefix? Do we want wrapper macros that include > the format!() call? I'm guessing you did not see the patch messages, which cover these points. Post your review under the patches please, > > You also have "pub enum LogMask" to work around the fact that log masks > are preprocessor macros. Is that okay, or do we want to modify C code > to make the bindings nicer? I for example would prefer the latter and > then declaring LogMask as a bitfield in bindgen. A bindgen type is definitely preferred but for a Rust idiomatic interface a wrapper type is a UX improvement (`CPU_LOG_PCALL`? `LOG_GUEST_ERROR`? We can use friendlier symbols in the LogMask variants for that) Thanks, Manos > > Thanks, > > Paolo > > > > > rust/wrapper.h | 1 + > > rust/hw/char/pl011/src/device.rs | 419 +++++++++++++++++--------- > > rust/hw/char/pl011/src/device_class.rs | 70 ----- > > rust/hw/char/pl011/src/lib.rs | 2 +- > > rust/qemu-api-macros/src/device.rs | 370 +++++++++++++++++++++++ > > rust/qemu-api-macros/src/lib.rs | 46 +-- > > rust/qemu-api-macros/src/object.rs | 107 +++++++ > > rust/qemu-api-macros/src/symbols.rs | 57 ++++ > > rust/qemu-api-macros/src/utilities.rs | 152 ++++++++++ > > rust/qemu-api-macros/src/vmstate.rs | 113 +++++++ > > rust/qemu-api/meson.build | 5 +- > > rust/qemu-api/src/definitions.rs | 97 ------ > > rust/qemu-api/src/device_class.rs | 128 -------- > > rust/qemu-api/src/lib.rs | 10 +- > > rust/qemu-api/src/log.rs | 140 +++++++++ > > rust/qemu-api/src/objects.rs | 90 ++++++ > > rust/qemu-api/src/tests.rs | 49 --- > > rust/qemu-api/src/vmstate.rs | 403 +++++++++++++++++++++++++ > > subprojects/packagefiles/syn-2-rs/meson.build | 1 + > > 19 files changed, 1726 insertions(+), 534 deletions(-) > > --- > > base-commit: 55522f72149fbf95ee3b057f1419da0cad46d0dd > > change-id: 20241024-rust-round-2-69fa10c9a0c9 > > > > -- > > γαῖα πυρί μιχθήτω > > > > > > >
On Sat, Oct 26, 2024 at 12:06 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > Please reply with review comments underneath individual patches, this > is hard to follow and I might miss some points. Will do. > > > Revert "rust: add PL011 device model" > > > rust: add PL011 device model > > > > ... should definitely be moved on top to clean up the authorship in "git > > blame" and other similar tools. Sorry about that. > > I will send them on a separate series and merge them from my tree when reviewed. Those are trivial to review, just "git diff HEAD^^". No need to separate them into a new submission. > > > rust: add support for migration in device models > > > rust/pl011: move CLK_NAME static to function scope > > > rust/pl011: add TYPE_PL011_LUMINARY device > > > rust/pl011: remove commented out C code > > > rust/pl011: Use correct masks for IBRD and FBRD > > > > (minus the usage of #[derive()] should be included in that series, so > > that qtests pass. It's not a huge amount of work and I can extract it, > > of course with proper attribution/authorship. > > These are independent from CI; i.e. you can merge your CI patches after those. That's what I did: I put them at the beginning of the series of pending patches, so they _are_ indeed merged after the CI patches. The only issue here is that patch 4 ("rust: add support for migration in device models") was dependent on the Device proc macro, but that was easy enough to extract. > > > > The rest are future work: > > > > > rust/qemu-api-macros: introduce Device proc macro > > > > As I said above, we first need to agree on the design. > > Post your review under the patches please, Yep. > > > rust/pl011: move pub callback decl to local scope > > > > This depends a lot on how we go implementing bindings to chardev. For > > example if the callbacks turn out to be a trait, it would have to be > > undone. Possibly the C callback wrappers would move to > > rust/qemu-api/chardev. For now I'd leave it aside. > > This patch moves the callbacks scope from public to inside the > function, no functional change related. It doesn't change or have > anything to do with chardev interfaces I understand. My point is that the callbacks you move (pl011_can_receive, pl011_receive, pl011_event) in the end will belong into the C<->Rust chardev bindings. A patch to remove "pub" would have basically the same benefit without the churn in indentation. > > > rust/qemu-api: add log module > > > rust/pl011: log guest/unimp errors > > > > This also needs design discussion. Do we want the API to be the same as > > C, i.e. keep the qemu_* prefix? Do we want wrapper macros that include > > the format!() call? > > I'm guessing you did not see the patch messages, which cover these > points. Post your review under the patches please, I will but for now I'll say that the commit messages do not address the questions I'm asking above. More in general, we need to establish conventions on what the Rust APIs look like (about qemu_* prefixes, for example). Personally I prefer to have APIs that, while easy enough to connect to the C ones, are idiomatic. But you can post these two patches separately and we can discuss it there. > > You also have "pub enum LogMask" to work around the fact that log masks > > are preprocessor macros. Is that okay, or do we want to modify C code > > to make the bindings nicer? I for example would prefer the latter and > > then declaring LogMask as a bitfield in bindgen. > > A bindgen type is definitely preferred but for a Rust idiomatic > interface a wrapper type is a UX improvement (`CPU_LOG_PCALL`? > `LOG_GUEST_ERROR`? We can use friendlier symbols in the LogMask > variants for that) What I'm saying is that these are discussions where we need to reach an agreement on, and document the choices. Paolo
Hello everyone, the pathological corrosion of QEMU code continues. This series expands the device model harness work performed in the initial Rust work from the previous month. In particular: - A new derive proc macro, Device, is introduced and the Object derive macro is heavily expanded upon. Many hack-like macros and fixups to declare FFI code for QEMU to consume were removed and now everything is generated by macros in the qemu-api-macros procmacro crate. - Add support for device migration by allowing device models to declare their VMState description, fields, subsections like in C code. This should also allow new qemu versions to transparently migrate any VMs that used C device models that were afterwards ported to Rust. - Add a small logging interface in qemu-api crate to allow calling qemu_log() and qemu_log_mask() from Rust code. - Some minor code cleanups in the Rust pl011 device, and also port of one fix patch and one log prints addition patch that was added in the C pl011. In the future if we go forward with rewriting device models in Rust, and keep having both of them in-tree while Rust is not required for building QEMU, we could alter checkpatch.pl to produce a warning if a C device model has changes but the corresponding Rust implementation doesn't. Code and functionality duplication is not fun, and pl011 was mostly done as a proof of concept for a Rust device because of its small complexity. As of this moment we have not decided on a policy for how to handle these things and it is not in **scope for this patch series anyway**. In the meantime there are lots of TODOs written in my personal notes, my personal repos, in the QEMU wiki, in mailing list threads. I plan on consolidating them all in gitlab issues to streamline things and who-does-what to avoid duplicating work. Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- Manos Pitsidianakis (11): Revert "rust: add PL011 device model" rust: add PL011 device model rust/qemu-api-macros: introduce Device proc macro rust: add support for migration in device models rust/pl011: move CLK_NAME static to function scope rust/pl011: add TYPE_PL011_LUMINARY device rust/pl011: move pub callback decl to local scope rust/pl011: remove commented out C code rust/pl011: Use correct masks for IBRD and FBRD rust/qemu-api: add log module rust/pl011: log guest/unimp errors rust/wrapper.h | 1 + rust/hw/char/pl011/src/device.rs | 419 +++++++++++++++++--------- rust/hw/char/pl011/src/device_class.rs | 70 ----- rust/hw/char/pl011/src/lib.rs | 2 +- rust/qemu-api-macros/src/device.rs | 370 +++++++++++++++++++++++ rust/qemu-api-macros/src/lib.rs | 46 +-- rust/qemu-api-macros/src/object.rs | 107 +++++++ rust/qemu-api-macros/src/symbols.rs | 57 ++++ rust/qemu-api-macros/src/utilities.rs | 152 ++++++++++ rust/qemu-api-macros/src/vmstate.rs | 113 +++++++ rust/qemu-api/meson.build | 5 +- rust/qemu-api/src/definitions.rs | 97 ------ rust/qemu-api/src/device_class.rs | 128 -------- rust/qemu-api/src/lib.rs | 10 +- rust/qemu-api/src/log.rs | 140 +++++++++ rust/qemu-api/src/objects.rs | 90 ++++++ rust/qemu-api/src/tests.rs | 49 --- rust/qemu-api/src/vmstate.rs | 403 +++++++++++++++++++++++++ subprojects/packagefiles/syn-2-rs/meson.build | 1 + 19 files changed, 1726 insertions(+), 534 deletions(-) --- base-commit: 55522f72149fbf95ee3b057f1419da0cad46d0dd change-id: 20241024-rust-round-2-69fa10c9a0c9 -- γαῖα πυρί μιχθήτω