qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
@ 2024-10-25 16:01 Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 01/23] rust: add definitions for vmstate Paolo Bonzini
                   ` (26 more replies)
  0 siblings, 27 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Since Manos helpfully posted his vmstate patches, this series is all that
is needed in order to enable Rust for at least the Debian, Fedora and
Ubuntu jobs.  I took his patches and isolated them from the procedural
macro experiment.

There are quite a few changes from the previous posting:

- new patches to bring pl011 mostly up to date with the C code (1-7)

- remove unnecessary .gitattributes file (8)

- apply rustfmt throughout

- add "rust: create a cargo workspace" to ensure a single version of the
  dependencies is used for all cargo commands (14, based on a suggestion by
  Junjie)

- add Junjie's syntax checks to the offset_of! macro.  I added a small
  struct with a From<> implementation, to make compile errors even easier
  to report (15).

- add final patch to enable rust in the Debian and Ubuntu system build
  jobs (23)

Note that this requires "meson subprojects update --reset" in order to do
an incremental build if you have already downloaded the Rust subprojects.
While I have a solution for that (modeled after scripts/git-submodule.sh),
I first need to check with the Meson folks whether my script is using only
stable interfaces.

This series can be found at branch rust-next of my git repository
(https://gitlab.com/bonzini/qemu.git), which also helps with the
problems in applying patch 8.  Everything up to commit f6a46d2a4eb
("rust: do not use TYPE_CHARDEV unnecessarily", 2024-10-25) will be
my next pull request, which I will send early next week (to give
people some more days to complain).

Paolo

Supersedes: <20241022100956.196657-1-pbonzini@redhat.com>

CI: https://gitlab.com/bonzini/qemu/-/pipelines/1512732399


Manos Pitsidianakis (6):
  rust: add definitions for vmstate
  rust/pl011: add support for migration
  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

Paolo Bonzini (17):
  rust/pl011: fix default value for migrate-clock
  rust: patch bilge-impl to allow compilation with 1.63.0
  rust: fix cfgs of proc-macro2 for 1.63.0
  rust: use std::os::raw instead of core::ffi
  rust: introduce a c_str macro
  rust: silence unknown warnings for the sake of old compilers
  rust: synchronize dependencies between subprojects and Cargo.lock
  rust: create a cargo workspace
  rust: introduce alternative implementation of offset_of!
  rust: do not use MaybeUninit::zeroed()
  rust: clean up detection of the language
  rust: allow version 1.63.0 of rustc
  rust: do not use --generate-cstr
  rust: allow older version of bindgen
  rust: make rustfmt optional
  dockerfiles: install bindgen from cargo on Ubuntu 22.04
  ci: enable rust in the Debian and Ubuntu system build job

 docs/about/build-platforms.rst                |  12 +
 meson.build                                   | 102 +++--
 .gitattributes                                |   2 +
 .gitlab-ci.d/buildtest.yml                    |   6 +-
 meson_options.txt                             |   2 +
 rust/{hw/char/pl011 => }/Cargo.lock           |   4 +
 rust/Cargo.toml                               |   7 +
 rust/hw/char/pl011/Cargo.toml                 |   3 -
 rust/hw/char/pl011/src/device.rs              | 158 ++++++--
 rust/hw/char/pl011/src/device_class.rs        |  71 +++-
 rust/hw/char/pl011/src/lib.rs                 |   5 +-
 rust/hw/char/pl011/src/memory_ops.rs          |  14 +-
 rust/qemu-api-macros/Cargo.lock               |  47 ---
 rust/qemu-api-macros/Cargo.toml               |   5 +-
 rust/qemu-api-macros/src/lib.rs               |  81 +++-
 rust/qemu-api/Cargo.lock                      |   7 -
 rust/qemu-api/Cargo.toml                      |  10 +-
 rust/qemu-api/build.rs                        |   9 +
 rust/qemu-api/meson.build                     |  17 +-
 rust/qemu-api/src/c_str.rs                    |  53 +++
 rust/qemu-api/src/definitions.rs              |   2 +-
 rust/qemu-api/src/device_class.rs             |  43 +--
 rust/qemu-api/src/lib.rs                      |  19 +-
 rust/qemu-api/src/offset_of.rs                | 161 ++++++++
 rust/qemu-api/src/vmstate.rs                  | 358 ++++++++++++++++++
 rust/qemu-api/src/zeroable.rs                 |  91 ++++-
 rust/qemu-api/tests/tests.rs                  |  29 +-
 scripts/meson-buildoptions.sh                 |   4 +
 subprojects/bilge-impl-0.2-rs.wrap            |   1 +
 .../packagefiles/bilge-impl-1.63.0.patch      |  45 +++
 .../packagefiles/proc-macro2-1-rs/meson.build |   4 +-
 subprojects/packagefiles/syn-2-rs/meson.build |   1 +
 tests/docker/dockerfiles/ubuntu2204.docker    |   5 +
 tests/lcitool/mappings.yml                    |   4 +
 tests/lcitool/refresh                         |  11 +-
 35 files changed, 1166 insertions(+), 227 deletions(-)
 rename rust/{hw/char/pl011 => }/Cargo.lock (98%)
 create mode 100644 rust/Cargo.toml
 delete mode 100644 rust/qemu-api-macros/Cargo.lock
 delete mode 100644 rust/qemu-api/Cargo.lock
 create mode 100644 rust/qemu-api/src/c_str.rs
 create mode 100644 rust/qemu-api/src/offset_of.rs
 create mode 100644 rust/qemu-api/src/vmstate.rs
 create mode 100644 subprojects/packagefiles/bilge-impl-1.63.0.patch

-- 
2.47.0



^ permalink raw reply	[flat|nested] 67+ messages in thread

* [PATCH 01/23] rust: add definitions for vmstate
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 02/23] rust/pl011: fix default value for migrate-clock Paolo Bonzini
                   ` (25 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Add a new qemu_api module, `vmstate`. Declare a bunch of Rust
macros declared that are equivalent in spirit to the C macros in
include/migration/vmstate.h.

For example the Rust of equivalent of the C macro:

  VMSTATE_UINT32(field_name, struct_name)

is:

  vmstate_uint32!(field_name, StructName)

This breathtaking development will allow us to reach feature parity between
the Rust and C pl011 implementations.

Extracted from a patch by Manos Pitsidianakis.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/meson.build         |   1 +
 rust/qemu-api/src/device_class.rs |  21 --
 rust/qemu-api/src/lib.rs          |   3 +
 rust/qemu-api/src/vmstate.rs      | 358 ++++++++++++++++++++++++++++++
 rust/qemu-api/tests/tests.rs      |  11 +-
 5 files changed, 368 insertions(+), 26 deletions(-)
 create mode 100644 rust/qemu-api/src/vmstate.rs

diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 1b0fd406378..3b849f7c413 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -5,6 +5,7 @@ _qemu_api_rs = static_library(
       'src/lib.rs',
       'src/definitions.rs',
       'src/device_class.rs',
+      'src/vmstate.rs',
       'src/zeroable.rs',
     ],
     {'.' : bindings_rs},
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index aa6088d9d3d..3d40256f60f 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -62,24 +62,3 @@ macro_rules! declare_properties {
         ];
     };
 }
-
-#[macro_export]
-macro_rules! vm_state_description {
-    ($(#[$outer:meta])*
-     $name:ident,
-     $(name: $vname:expr,)*
-     $(unmigratable: $um_val:expr,)*
-    ) => {
-        #[used]
-        $(#[$outer])*
-        pub static $name: $crate::bindings::VMStateDescription = $crate::bindings::VMStateDescription {
-            $(name: {
-                #[used]
-                static VMSTATE_NAME: &::core::ffi::CStr = $vname;
-                $vname.as_ptr()
-            },)*
-            unmigratable: true,
-            ..$crate::zeroable::Zeroable::ZERO
-        };
-    }
-}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index e94a15bb823..10ab3d7e639 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -26,9 +26,12 @@ unsafe impl Send for bindings::Property {}
 unsafe impl Sync for bindings::Property {}
 unsafe impl Sync for bindings::TypeInfo {}
 unsafe impl Sync for bindings::VMStateDescription {}
+unsafe impl Sync for bindings::VMStateField {}
+unsafe impl Sync for bindings::VMStateInfo {}
 
 pub mod definitions;
 pub mod device_class;
+pub mod vmstate;
 pub mod zeroable;
 
 use std::alloc::{GlobalAlloc, Layout};
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
new file mode 100644
index 00000000000..3aa6ed2a781
--- /dev/null
+++ b/rust/qemu-api/src/vmstate.rs
@@ -0,0 +1,358 @@
+// Copyright 2024, Linaro Limited
+// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Helper macros to declare migration state for device models.
+//!
+//! Some macros are direct equivalents to the C macros declared in
+//! `include/migration/vmstate.h` while [`vmstate_subsections`] and
+//! [`vmstate_fields`] are meant to be used when declaring a device model state
+//! struct.
+
+#[doc(alias = "VMSTATE_UNUSED_BUFFER")]
+#[macro_export]
+macro_rules! vmstate_unused_buffer {
+    ($field_exists_fn:expr, $version_id:expr, $size:expr) => {{
+        $crate::bindings::VMStateField {
+            name: c"unused".as_ptr(),
+            err_hint: ::core::ptr::null(),
+            offset: 0,
+            size: $size,
+            start: 0,
+            num: 0,
+            num_offset: 0,
+            size_offset: 0,
+            info: unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_info_unused_buffer) },
+            flags: VMStateFlags::VMS_BUFFER,
+            vmsd: ::core::ptr::null(),
+            version_id: $version_id,
+            struct_version_id: 0,
+            field_exists: $field_exists_fn,
+        }
+    }};
+}
+
+#[doc(alias = "VMSTATE_UNUSED_V")]
+#[macro_export]
+macro_rules! vmstate_unused_v {
+    ($version_id:expr, $size:expr) => {{
+        $crate::vmstate_unused_buffer!(None, $version_id, $size)
+    }};
+}
+
+#[doc(alias = "VMSTATE_UNUSED")]
+#[macro_export]
+macro_rules! vmstate_unused {
+    ($size:expr) => {{
+        $crate::vmstate_unused_v!(0, $size)
+    }};
+}
+
+#[doc(alias = "VMSTATE_SINGLE_TEST")]
+#[macro_export]
+macro_rules! vmstate_single_test {
+    ($field_name:ident, $struct_name:ty, $field_exists_fn:expr, $version_id:expr, $info:block, $size:expr) => {{
+        $crate::bindings::VMStateField {
+            name: ::core::concat!(::core::stringify!($field_name), 0)
+                .as_bytes()
+                .as_ptr() as *const ::core::ffi::c_char,
+            err_hint: ::core::ptr::null(),
+            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            size: $size,
+            start: 0,
+            num: 0,
+            num_offset: 0,
+            size_offset: 0,
+            info: $info,
+            flags: VMStateFlags::VMS_SINGLE,
+            vmsd: ::core::ptr::null(),
+            version_id: $version_id,
+            struct_version_id: 0,
+            field_exists: $field_exists_fn,
+        }
+    }};
+}
+
+#[doc(alias = "VMSTATE_SINGLE")]
+#[macro_export]
+macro_rules! vmstate_single {
+    ($field_name:ident, $struct_name:ty, $version_id:expr, $info:block, $size:expr) => {{
+        $crate::vmstate_single_test!($field_name, $struct_name, None, $version_id, $info, $size)
+    }};
+}
+
+#[doc(alias = "VMSTATE_UINT32_V")]
+#[macro_export]
+macro_rules! vmstate_uint32_v {
+    ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
+        $crate::vmstate_single!(
+            $field_name,
+            $struct_name,
+            $version_id,
+            { unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_info_uint32) } },
+            ::core::mem::size_of::<u32>()
+        )
+    }};
+}
+
+#[doc(alias = "VMSTATE_UINT32")]
+#[macro_export]
+macro_rules! vmstate_uint32 {
+    ($field_name:ident, $struct_name:ty) => {{
+        $crate::vmstate_uint32_v!($field_name, $struct_name, 0)
+    }};
+}
+
+#[doc(alias = "VMSTATE_INT32_V")]
+#[macro_export]
+macro_rules! vmstate_int32_v {
+    ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
+        $crate::vmstate_single!(
+            $field_name,
+            $struct_name,
+            $version_id,
+            { unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_info_int32) } },
+            ::core::mem::size_of::<i32>()
+        )
+    }};
+}
+
+#[doc(alias = "VMSTATE_INT32")]
+#[macro_export]
+macro_rules! vmstate_int32 {
+    ($field_name:ident, $struct_name:ty) => {{
+        $crate::vmstate_int32_v!($field_name, $struct_name, 0)
+    }};
+}
+
+#[doc(alias = "VMSTATE_ARRAY")]
+#[macro_export]
+macro_rules! vmstate_array {
+    ($field_name:ident, $struct_name:ty, $length:expr, $version_id:expr, $info:block, $size:expr) => {{
+        $crate::bindings::VMStateField {
+            name: ::core::concat!(::core::stringify!($field_name), 0)
+                .as_bytes()
+                .as_ptr() as *const ::core::ffi::c_char,
+            err_hint: ::core::ptr::null(),
+            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            size: $size,
+            start: 0,
+            num: $length as _,
+            num_offset: 0,
+            size_offset: 0,
+            info: $info,
+            flags: VMStateFlags::VMS_ARRAY,
+            vmsd: ::core::ptr::null(),
+            version_id: $version_id,
+            struct_version_id: 0,
+            field_exists: None,
+        }
+    }};
+}
+
+#[doc(alias = "VMSTATE_UINT32_ARRAY_V")]
+#[macro_export]
+macro_rules! vmstate_uint32_array_v {
+    ($field_name:ident, $struct_name:ty, $length:expr, $version_id:expr) => {{
+        $crate::vmstate_array!(
+            $field_name,
+            $struct_name,
+            $length,
+            $version_id,
+            { unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_info_uint32) } },
+            ::core::mem::size_of::<u32>()
+        )
+    }};
+}
+
+#[doc(alias = "VMSTATE_UINT32_ARRAY")]
+#[macro_export]
+macro_rules! vmstate_uint32_array {
+    ($field_name:ident, $struct_name:ty, $length:expr) => {{
+        $crate::vmstate_uint32_array_v!($field_name, $struct_name, $length, 0)
+    }};
+}
+
+#[doc(alias = "VMSTATE_STRUCT_POINTER_V")]
+#[macro_export]
+macro_rules! vmstate_struct_pointer_v {
+    ($field_name:ident, $struct_name:ty, $version_id:expr, $vmsd:expr, $type:ty) => {{
+        $crate::bindings::VMStateField {
+            name: ::core::concat!(::core::stringify!($field_name), 0)
+                .as_bytes()
+                .as_ptr() as *const ::core::ffi::c_char,
+            err_hint: ::core::ptr::null(),
+            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            size: ::core::mem::size_of::<*const $type>(),
+            start: 0,
+            num: 0,
+            num_offset: 0,
+            size_offset: 0,
+            info: ::core::ptr::null(),
+            flags: VMStateFlags(VMStateFlags::VMS_STRUCT.0 | VMStateFlags::VMS_POINTER.0),
+            vmsd: $vmsd,
+            version_id: $version_id,
+            struct_version_id: 0,
+            field_exists: None,
+        }
+    }};
+}
+
+#[doc(alias = "VMSTATE_ARRAY_OF_POINTER")]
+#[macro_export]
+macro_rules! vmstate_array_of_pointer {
+    ($field_name:ident, $struct_name:ty, $num:expr, $version_id:expr, $info:expr, $type:ty) => {{
+        $crate::bindings::VMStateField {
+            name: ::core::concat!(::core::stringify!($field_name), 0)
+                .as_bytes()
+                .as_ptr() as *const ::core::ffi::c_char,
+            version_id: $version_id,
+            num: $num as _,
+            info: $info,
+            size: ::core::mem::size_of::<*const $type>(),
+            flags: VMStateFlags(VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0),
+            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            err_hint: ::core::ptr::null(),
+            start: 0,
+            num_offset: 0,
+            size_offset: 0,
+            vmsd: ::core::ptr::null(),
+            struct_version_id: 0,
+            field_exists: None,
+        }
+    }};
+}
+
+#[doc(alias = "VMSTATE_ARRAY_OF_POINTER_TO_STRUCT")]
+#[macro_export]
+macro_rules! vmstate_array_of_pointer_to_struct {
+    ($field_name:ident, $struct_name:ty, $num:expr, $version_id:expr, $vmsd:expr, $type:ty) => {{
+        $crate::bindings::VMStateField {
+            name: ::core::concat!(::core::stringify!($field_name), 0)
+                .as_bytes()
+                .as_ptr() as *const ::core::ffi::c_char,
+            version_id: $version_id,
+            num: $num as _,
+            vmsd: $vmsd,
+            size: ::core::mem::size_of::<*const $type>(),
+            flags: VMStateFlags(
+                VMStateFlags::VMS_ARRAY.0
+                    | VMStateFlags::VMS_STRUCT.0
+                    | VMStateFlags::VMS_ARRAY_OF_POINTER.0,
+            ),
+            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            err_hint: ::core::ptr::null(),
+            start: 0,
+            num_offset: 0,
+            size_offset: 0,
+            vmsd: ::core::ptr::null(),
+            struct_version_id: 0,
+            field_exists: None,
+        }
+    }};
+}
+
+#[doc(alias = "VMSTATE_CLOCK_V")]
+#[macro_export]
+macro_rules! vmstate_clock_v {
+    ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
+        $crate::vmstate_struct_pointer_v!(
+            $field_name,
+            $struct_name,
+            $version_id,
+            { unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_clock) } },
+            $crate::bindings::Clock
+        )
+    }};
+}
+
+#[doc(alias = "VMSTATE_CLOCK")]
+#[macro_export]
+macro_rules! vmstate_clock {
+    ($field_name:ident, $struct_name:ty) => {{
+        $crate::vmstate_clock_v!($field_name, $struct_name, 0)
+    }};
+}
+
+#[doc(alias = "VMSTATE_ARRAY_CLOCK_V")]
+#[macro_export]
+macro_rules! vmstate_array_clock_v {
+    ($field_name:ident, $struct_name:ty, $num:expr, $version_id:expr) => {{
+        $crate::vmstate_array_of_pointer_to_struct!(
+            $field_name,
+            $struct_name,
+            $num,
+            $version_id,
+            { unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_clock) } },
+            $crate::bindings::Clock
+        )
+    }};
+}
+
+#[doc(alias = "VMSTATE_ARRAY_CLOCK")]
+#[macro_export]
+macro_rules! vmstate_array_clock {
+    ($field_name:ident, $struct_name:ty, $num:expr) => {{
+        $crate::vmstate_array_clock_v!($field_name, $struct_name, $name, 0)
+    }};
+}
+
+/// Helper macro to declare a list of
+/// ([`VMStateField`](`crate::bindings::VMStateField`)) into a static and return
+/// a pointer to the array of values it created.
+#[macro_export]
+macro_rules! vmstate_fields {
+    ($($field:expr),*$(,)*) => {{
+        static _FIELDS: &[$crate::bindings::VMStateField] = &[
+            $($field),*,
+            $crate::bindings::VMStateField {
+                name: ::core::ptr::null(),
+                err_hint: ::core::ptr::null(),
+                offset: 0,
+                size: 0,
+                start: 0,
+                num: 0,
+                num_offset: 0,
+                size_offset: 0,
+                info: ::core::ptr::null(),
+                flags: VMStateFlags::VMS_END,
+                vmsd: ::core::ptr::null(),
+                version_id: 0,
+                struct_version_id: 0,
+                field_exists: None,
+            }
+        ];
+        _FIELDS.as_ptr()
+    }}
+}
+
+/// A transparent wrapper type for the `subsections` field of
+/// [`VMStateDescription`](crate::bindings::VMStateDescription).
+///
+/// This is necessary to be able to declare subsection descriptions as statics,
+/// because the only way to implement `Sync` for a foreign type (and `*const`
+/// pointers are foreign types in Rust) is to create a wrapper struct and
+/// `unsafe impl Sync` for it.
+///
+/// This struct is used in the [`vmstate_subsections`] macro implementation.
+#[repr(transparent)]
+pub struct VMStateSubsectionsWrapper(pub &'static [*const crate::bindings::VMStateDescription]);
+
+unsafe impl Sync for VMStateSubsectionsWrapper {}
+
+/// Helper macro to declare a list of subsections
+/// ([`VMStateDescription`](`crate::bindings::VMStateDescription`)) into a
+/// static and return a pointer to the array of pointers it created.
+#[macro_export]
+macro_rules! vmstate_subsections {
+    ($($subsection:expr),*$(,)*) => {{
+        static _SUBSECTIONS: $crate::vmstate::VMStateSubsectionsWrapper = $crate::vmstate::VMStateSubsectionsWrapper(&[
+            $({
+                static _SUBSECTION: $crate::bindings::VMStateDescription = $subsection;
+                ::core::ptr::addr_of!(_SUBSECTION)
+            }),*,
+            ::core::ptr::null()
+        ]);
+        _SUBSECTIONS.0.as_ptr()
+    }}
+}
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index aa1e0568c69..37c4dd44f81 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -8,17 +8,18 @@
     bindings::*,
     declare_properties, define_property,
     definitions::{Class, ObjectImpl},
-    device_class_init, vm_state_description,
+    device_class_init,
+    zeroable::Zeroable,
 };
 
 #[test]
 fn test_device_decl_macros() {
     // Test that macros can compile.
-    vm_state_description! {
-        VMSTATE,
-        name: c"name",
+    pub static VMSTATE: VMStateDescription = VMStateDescription {
+        name: c"name".as_ptr(),
         unmigratable: true,
-    }
+        ..Zeroable::ZERO
+    };
 
     #[repr(C)]
     #[derive(qemu_api_macros::Object)]
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 02/23] rust/pl011: fix default value for migrate-clock
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 01/23] rust: add definitions for vmstate Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 03/23] rust/pl011: add support for migration Paolo Bonzini
                   ` (24 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device_class.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 08c846aa482..9282dc4d151 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -29,7 +29,8 @@
         PL011State,
         migrate_clock,
         unsafe { &qdev_prop_bool },
-        bool
+        bool,
+        default = true
     ),
 }
 
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 03/23] rust/pl011: add support for migration
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 01/23] rust: add definitions for vmstate Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 02/23] rust/pl011: fix default value for migrate-clock Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 04/23] rust/pl011: move CLK_NAME static to function scope Paolo Bonzini
                   ` (23 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Declare the vmstate description of the PL011 device.

Based on a patch by Manos Pitsidianakis.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lore.kernel.org/r/20241024-rust-round-2-v1-4-051e7a25b978@linaro.org
---
 rust/hw/char/pl011/src/device.rs       | 27 ++++++++++
 rust/hw/char/pl011/src/device_class.rs | 68 +++++++++++++++++++++++---
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index b3d8bc004e0..dd9145669dc 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -20,6 +20,12 @@
 
 static PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
 
+/// Integer Baud Rate Divider, `UARTIBRD`
+const IBRD_MASK: u32 = 0x3f;
+
+/// Fractional Baud Rate Divider, `UARTFBRD`
+const FBRD_MASK: u32 = 0xffff;
+
 const DATA_BREAK: u32 = 1 << 10;
 
 /// QEMU sourced constant.
@@ -492,6 +498,27 @@ pub fn update(&self) {
             unsafe { qemu_set_irq(*irq, i32::from(flags & i != 0)) };
         }
     }
+
+    pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
+        /* Sanity-check input state */
+        if self.read_pos >= self.read_fifo.len() || self.read_count > self.read_fifo.len() {
+            return Err(());
+        }
+
+        if !self.fifo_enabled() && self.read_count > 0 && self.read_pos > 0 {
+            // Older versions of PL011 didn't ensure that the single
+            // character in the FIFO in FIFO-disabled mode is in
+            // element 0 of the array; convert to follow the current
+            // code's assumptions.
+            self.read_fifo[0] = self.read_fifo[self.read_pos];
+            self.read_pos = 0;
+        }
+
+        self.ibrd &= IBRD_MASK;
+        self.fbrd &= FBRD_MASK;
+
+        Ok(())
+    }
 }
 
 /// Which bits in the interrupt status matter for each outbound IRQ line ?
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 9282dc4d151..27cabbd6651 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -2,16 +2,73 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use core::ptr::NonNull;
+use core::{
+    ffi::{c_int, c_void},
+    ptr::NonNull,
+};
 
-use qemu_api::{bindings::*, definitions::ObjectImpl, zeroable::Zeroable};
+use qemu_api::{
+    bindings::*, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections, vmstate_uint32,
+    vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
+};
 
-use crate::device::PL011State;
+use crate::device::{PL011State, PL011_FIFO_DEPTH};
+
+extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool {
+    unsafe {
+        debug_assert!(!opaque.is_null());
+        let state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+        state.as_ref().migrate_clock
+    }
+}
+
+/// Migration subsection for [`PL011State`] clock.
+pub static VMSTATE_PL011_CLOCK: VMStateDescription = VMStateDescription {
+    name: c"pl011/clock".as_ptr(),
+    version_id: 1,
+    minimum_version_id: 1,
+    needed: Some(pl011_clock_needed),
+    fields: vmstate_fields! {
+        vmstate_clock!(clock, PL011State),
+    },
+    ..Zeroable::ZERO
+};
+
+extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
+    unsafe {
+        debug_assert!(!opaque.is_null());
+        let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+        let result = state.as_mut().post_load(version_id as u32);
+        if result.is_err() { -1 } else { 0 }
+    }
+}
 
-#[used]
 pub static VMSTATE_PL011: VMStateDescription = VMStateDescription {
-    name: PL011State::TYPE_INFO.name,
-    unmigratable: true,
+    name: c"pl011".as_ptr(),
+    version_id: 2,
+    minimum_version_id: 2,
+    post_load: Some(pl011_post_load),
+    fields: vmstate_fields! {
+        vmstate_unused!(::core::mem::size_of::<u32>()),
+        vmstate_uint32!(flags, PL011State),
+        vmstate_uint32!(line_control, PL011State),
+        vmstate_uint32!(receive_status_error_clear, PL011State),
+        vmstate_uint32!(control, 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),
+    },
+    subsections: vmstate_subsections! {
+        VMSTATE_PL011_CLOCK
+    },
     ..Zeroable::ZERO
 };
 
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 04/23] rust/pl011: move CLK_NAME static to function scope
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (2 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 03/23] rust/pl011: add support for migration Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device Paolo Bonzini
                   ` (22 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

We do not need to have CLK_NAME public nor a static. No functional change.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lore.kernel.org/r/20241024-rust-round-2-v1-5-051e7a25b978@linaro.org
---
 rust/hw/char/pl011/src/device.rs | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index dd9145669dc..f91790ff185 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -102,9 +102,6 @@ impl qemu_api::definitions::Class for PL011Class {
     > = None;
 }
 
-#[used]
-pub static CLK_NAME: &CStr = c"clk";
-
 impl PL011State {
     /// Initializes a pre-allocated, unitialized instance of `PL011State`.
     ///
@@ -114,7 +111,9 @@ impl PL011State {
     /// `PL011State` type. It must not be called more than once on the same
     /// location/instance. All its fields are expected to hold unitialized
     /// values with the sole exception of `parent_obj`.
-    pub unsafe fn init(&mut self) {
+    unsafe fn init(&mut self) {
+        const CLK_NAME: &CStr = c"clk";
+
         let dev = addr_of_mut!(*self).cast::<DeviceState>();
         // SAFETY:
         //
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (3 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 04/23] rust/pl011: move CLK_NAME static to function scope Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-31 14:58   ` Zhao Liu
  2024-10-25 16:01 ` [PATCH 06/23] rust/pl011: remove commented out C code Paolo Bonzini
                   ` (21 subsequent siblings)
  26 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Add a device specialization for the Luminary UART device.

This commit adds a DeviceId enum that utilizes the Index trait to return
different bytes depending on what device id the UART has (Arm -default-
or Luminary)

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lore.kernel.org/r/20241024-rust-round-2-v1-6-051e7a25b978@linaro.org
---
 rust/hw/char/pl011/src/device.rs | 77 ++++++++++++++++++++++++++++++--
 rust/hw/char/pl011/src/lib.rs    |  1 +
 2 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index f91790ff185..051c59f39ae 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -18,8 +18,6 @@
     RegisterOffset,
 };
 
-static PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
-
 /// Integer Baud Rate Divider, `UARTIBRD`
 const IBRD_MASK: u32 = 0x3f;
 
@@ -31,6 +29,29 @@
 /// QEMU sourced constant.
 pub const PL011_FIFO_DEPTH: usize = 16_usize;
 
+#[derive(Clone, Copy, Debug)]
+enum DeviceId {
+    #[allow(dead_code)]
+    Arm = 0,
+    Luminary,
+}
+
+impl std::ops::Index<hwaddr> for DeviceId {
+    type Output = c_uchar;
+
+    fn index(&self, idx: hwaddr) -> &Self::Output {
+        match self {
+            Self::Arm => &Self::PL011_ID_ARM[idx as usize],
+            Self::Luminary => &Self::PL011_ID_LUMINARY[idx as usize],
+        }
+    }
+}
+
+impl DeviceId {
+    const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
+    const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1];
+}
+
 #[repr(C)]
 #[derive(Debug, qemu_api_macros::Object)]
 /// PL011 Device Model in QEMU
@@ -75,6 +96,8 @@ pub struct PL011State {
     pub clock: NonNull<Clock>,
     #[doc(alias = "migrate_clk")]
     pub migrate_clock: bool,
+    /// The byte string that identifies the device.
+    device_id: DeviceId,
 }
 
 impl ObjectImpl for PL011State {
@@ -162,7 +185,7 @@ pub fn read(
 
         std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
             Err(v) if (0x3f8..0x400).contains(&v) => {
-                u64::from(PL011_ID_ARM[((offset - 0xfe0) >> 2) as usize])
+                u64::from(self.device_id[(offset - 0xfe0) >> 2])
             }
             Err(_) => {
                 // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
@@ -619,3 +642,51 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
         state.as_mut().init();
     }
 }
+
+#[repr(C)]
+#[derive(Debug, qemu_api_macros::Object)]
+/// PL011 Luminary device model.
+pub struct PL011Luminary {
+    parent_obj: PL011State,
+}
+
+#[repr(C)]
+pub struct PL011LuminaryClass {
+    _inner: [u8; 0],
+}
+
+/// Initializes a pre-allocated, unitialized instance of `PL011Luminary`.
+///
+/// # Safety
+///
+/// We expect the FFI user of this function to pass a valid pointer, that has
+/// the same size as [`PL011Luminary`]. We also expect the device is
+/// readable/writeable from one thread at any time.
+pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) {
+    unsafe {
+        debug_assert!(!obj.is_null());
+        let mut state = NonNull::new_unchecked(obj.cast::<PL011Luminary>());
+        let state = state.as_mut();
+        state.parent_obj.device_id = DeviceId::Luminary;
+    }
+}
+
+impl qemu_api::definitions::Class for PL011LuminaryClass {
+    const CLASS_INIT: Option<
+        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
+    > = None;
+    const CLASS_BASE_INIT: Option<
+        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
+    > = None;
+}
+
+impl ObjectImpl for PL011Luminary {
+    type Class = PL011LuminaryClass;
+    const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self };
+    const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY;
+    const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011);
+    const ABSTRACT: bool = false;
+    const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_luminary_init);
+    const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
+    const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
+}
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 2939ee50c99..c6bb76a7926 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -46,6 +46,7 @@
 pub mod memory_ops;
 
 pub const TYPE_PL011: &::core::ffi::CStr = c"pl011";
+pub const TYPE_PL011_LUMINARY: &::core::ffi::CStr = c"pl011_luminary";
 
 /// Offset of each register from the base memory address of the device.
 ///
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 06/23] rust/pl011: remove commented out C code
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (4 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 07/23] rust/pl011: Use correct masks for IBRD and FBRD Paolo Bonzini
                   ` (20 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

This code juxtaposed what should be happening according to the C device
model but is not needed now that this has been reviewed (I hope) and its
validity checked against what the C device does (I hope, again).

No functional change.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lore.kernel.org/r/20241024-rust-round-2-v1-8-051e7a25b978@linaro.org
---
 rust/hw/char/pl011/src/device.rs | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 051c59f39ae..98357db04e8 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -192,7 +192,6 @@ pub fn read(
                 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 {
@@ -200,11 +199,9 @@ pub fn read(
                     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.
@@ -374,13 +371,6 @@ fn loopback_mdmctrl(&mut self) {
          * 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());
@@ -391,10 +381,6 @@ fn loopback_mdmctrl(&mut self) {
         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;
@@ -500,10 +486,8 @@ pub fn put_fifo(&mut self, value: c_uint) {
         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);
         }
 
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 07/23] rust/pl011: Use correct masks for IBRD and FBRD
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (5 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 06/23] rust/pl011: remove commented out C code Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 08/23] rust: patch bilge-impl to allow compilation with 1.63.0 Paolo Bonzini
                   ` (19 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Port fix from commit cd247eae16ab1b9ce97fd34c000c1b883feeda45
"hw/char/pl011: Use correct masks for IBRD and FBRD"

Related issue: <https://gitlab.com/qemu-project/qemu/-/issues/2610>

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lore.kernel.org/r/20241024-rust-round-2-v1-9-051e7a25b978@linaro.org
---
 rust/hw/char/pl011/src/device.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 98357db04e8..788b47203b1 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -19,10 +19,10 @@
 };
 
 /// Integer Baud Rate Divider, `UARTIBRD`
-const IBRD_MASK: u32 = 0x3f;
+const IBRD_MASK: u32 = 0xffff;
 
 /// Fractional Baud Rate Divider, `UARTFBRD`
-const FBRD_MASK: u32 = 0xffff;
+const FBRD_MASK: u32 = 0x3f;
 
 const DATA_BREAK: u32 = 1 << 10;
 
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 08/23] rust: patch bilge-impl to allow compilation with 1.63.0
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (6 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 07/23] rust/pl011: Use correct masks for IBRD and FBRD Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 09/23] rust: fix cfgs of proc-macro2 for 1.63.0 Paolo Bonzini
                   ` (18 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Apply a patch that removes "let ... else" constructs, replacing them with
"if let ... else" or "let ... = match ...".  "let ... else" was stabilized in
Rust 1.65.0.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .gitattributes                                |  2 +
 subprojects/bilge-impl-0.2-rs.wrap            |  1 +
 .../packagefiles/bilge-impl-1.63.0.patch      | 45 +++++++++++++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 subprojects/packagefiles/bilge-impl-1.63.0.patch

diff --git a/.gitattributes b/.gitattributes
index 6dc6383d3d1..9ce7a19581a 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -5,3 +5,5 @@
 *.rs            diff=rust
 *.rs.inc        diff=rust
 Cargo.lock      diff=toml merge=binary
+
+*.patch         -text -whitespace
diff --git a/subprojects/bilge-impl-0.2-rs.wrap b/subprojects/bilge-impl-0.2-rs.wrap
index eefb10c36c2..b24c34a9043 100644
--- a/subprojects/bilge-impl-0.2-rs.wrap
+++ b/subprojects/bilge-impl-0.2-rs.wrap
@@ -5,3 +5,4 @@ source_filename = bilge-impl-0.2.0.tar.gz
 source_hash = feb11e002038ad243af39c2068c8a72bcf147acf05025dcdb916fcc000adb2d8
 #method = cargo
 patch_directory = bilge-impl-0.2-rs
+diff_files = bilge-impl-1.63.0.patch
diff --git a/subprojects/packagefiles/bilge-impl-1.63.0.patch b/subprojects/packagefiles/bilge-impl-1.63.0.patch
new file mode 100644
index 00000000000..987428a6d65
--- /dev/null
+++ b/subprojects/packagefiles/bilge-impl-1.63.0.patch
@@ -0,0 +1,45 @@
+--- a/src/shared/discriminant_assigner.rs
++++ b/src/shared/discriminant_assigner.rs
+@@ -26,20 +26,20 @@
+         let discriminant_expr = &discriminant.1;
+         let variant_name = &variant.ident;
+ 
+-        let Expr::Lit(ExprLit { lit: Lit::Int(int), .. }) = discriminant_expr else {
++        if let Expr::Lit(ExprLit { lit: Lit::Int(int), .. }) = discriminant_expr {
++            let discriminant_value: u128 = int.base10_parse().unwrap_or_else(unreachable);
++            if discriminant_value > self.max_value() {
++                abort!(variant, "Value of variant exceeds the given number of bits")
++            }
++
++            Some(discriminant_value)
++        } else {
+             abort!(
+                 discriminant_expr,
+                 "variant `{}` is not a number", variant_name;
+                 help = "only literal integers currently supported"
+             )
+-        };
+-
+-        let discriminant_value: u128 = int.base10_parse().unwrap_or_else(unreachable);
+-        if discriminant_value > self.max_value() {
+-            abort!(variant, "Value of variant exceeds the given number of bits")
+         }
+-
+-        Some(discriminant_value)
+     }
+ 
+     fn assign(&mut self, variant: &Variant) -> u128 {
+--- a/src/shared/fallback.rs
++++ b/src/shared/fallback.rs
+@@ -22,8 +22,9 @@
+             }
+             Unnamed(fields) => {
+                 let variant_fields = fields.unnamed.iter();
+-                let Ok(fallback_value) = variant_fields.exactly_one() else {
+-                    abort!(variant, "fallback variant must have exactly one field"; help = "use only one field or change to a unit variant")
++                let fallback_value = match variant_fields.exactly_one() {
++                    Ok(ok) => ok,
++                    _ => abort!(variant, "fallback variant must have exactly one field"; help = "use only one field or change to a unit variant")
+                 };
+ 
+                 if !is_last_variant {
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 09/23] rust: fix cfgs of proc-macro2 for 1.63.0
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (7 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 08/23] rust: patch bilge-impl to allow compilation with 1.63.0 Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 10/23] rust: use std::os::raw instead of core::ffi Paolo Bonzini
                   ` (17 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Replay the configuration that would be computed by build.rs when compiling
on a 1.63.0 compiler.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 subprojects/packagefiles/proc-macro2-1-rs/meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/subprojects/packagefiles/proc-macro2-1-rs/meson.build b/subprojects/packagefiles/proc-macro2-1-rs/meson.build
index 818ec59336b..8e601b50ccc 100644
--- a/subprojects/packagefiles/proc-macro2-1-rs/meson.build
+++ b/subprojects/packagefiles/proc-macro2-1-rs/meson.build
@@ -15,7 +15,9 @@ _proc_macro2_rs = static_library(
   rust_abi: 'rust',
   rust_args: [
     '--cfg', 'feature="proc-macro"',
-    '--cfg', 'span_locations',
+    '--cfg', 'no_literal_byte_character',
+    '--cfg', 'no_literal_c_string',
+    '--cfg', 'no_source_text',
     '--cfg', 'wrap_proc_macro',
   ],
   dependencies: [
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 10/23] rust: use std::os::raw instead of core::ffi
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (8 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 09/23] rust: fix cfgs of proc-macro2 for 1.63.0 Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 11/23] rust: introduce a c_str macro Paolo Bonzini
                   ` (16 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

core::ffi::c_* types were introduced in Rust 1.64.0.  Use the older types
in std::os::raw, which are now aliases of the types in core::ffi.  There is
no need to compile QEMU as no_std, so this is acceptable as long as we support
a version of Debian with Rust 1.63.0.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                          |  3 +--
 rust/hw/char/pl011/src/device.rs     | 35 +++++++++++-----------------
 rust/hw/char/pl011/src/lib.rs        |  4 ++--
 rust/hw/char/pl011/src/memory_ops.rs | 14 +++--------
 rust/qemu-api/src/definitions.rs     |  2 +-
 rust/qemu-api/src/device_class.rs    |  6 ++---
 rust/qemu-api/src/lib.rs             | 11 +++++----
 rust/qemu-api/src/vmstate.rs         | 10 ++++----
 rust/qemu-api/tests/tests.rs         |  9 ++++---
 9 files changed, 39 insertions(+), 55 deletions(-)

diff --git a/meson.build b/meson.build
index 5e2b0fb82b2..e819148f36c 100644
--- a/meson.build
+++ b/meson.build
@@ -3933,14 +3933,13 @@ if have_rust and have_system
   bindgen_args = [
     '--disable-header-comment',
     '--raw-line', '// @generated',
-    '--ctypes-prefix', 'core::ffi',
+    '--ctypes-prefix', 'std::os::raw',
     '--formatter', 'rustfmt',
     '--generate-block',
     '--generate-cstr',
     '--impl-debug',
     '--merge-extern-blocks',
     '--no-doc-comments',
-    '--use-core',
     '--with-derive-default',
     '--no-layout-tests',
     '--no-prepend-enum-name',
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 788b47203b1..036757f7f3a 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -2,9 +2,10 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use core::{
-    ffi::{c_int, c_uchar, c_uint, c_void, CStr},
-    ptr::{addr_of, addr_of_mut, NonNull},
+use core::ptr::{addr_of, addr_of_mut, NonNull};
+use std::{
+    ffi::CStr,
+    os::raw::{c_int, c_uchar, c_uint, c_void},
 };
 
 use qemu_api::{
@@ -117,11 +118,10 @@ pub struct PL011Class {
 }
 
 impl qemu_api::definitions::Class for PL011Class {
-    const CLASS_INIT: Option<
-        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
-    > = Some(crate::device_class::pl011_class_init);
+    const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)> =
+        Some(crate::device_class::pl011_class_init);
     const CLASS_BASE_INIT: Option<
-        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
+        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void),
     > = None;
 }
 
@@ -176,11 +176,7 @@ unsafe fn init(&mut self) {
         }
     }
 
-    pub fn read(
-        &mut self,
-        offset: hwaddr,
-        _size: core::ffi::c_uint,
-    ) -> std::ops::ControlFlow<u64, u64> {
+    pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
         use RegisterOffset::*;
 
         std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
@@ -562,11 +558,7 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
 /// readable/writeable from one thread at any time.
 ///
 /// The buffer and size arguments must also be valid.
-pub unsafe extern "C" fn pl011_receive(
-    opaque: *mut core::ffi::c_void,
-    buf: *const u8,
-    size: core::ffi::c_int,
-) {
+pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const u8, size: c_int) {
     unsafe {
         debug_assert!(!opaque.is_null());
         let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
@@ -585,7 +577,7 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
 /// We expect the FFI user of this function to pass a valid pointer, that has
 /// the same size as [`PL011State`]. We also expect the device is
 /// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_event(opaque: *mut core::ffi::c_void, event: QEMUChrEvent) {
+pub unsafe extern "C" fn pl011_event(opaque: *mut c_void, event: QEMUChrEvent) {
     unsafe {
         debug_assert!(!opaque.is_null());
         let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
@@ -656,11 +648,10 @@ pub struct PL011LuminaryClass {
 }
 
 impl qemu_api::definitions::Class for PL011LuminaryClass {
-    const CLASS_INIT: Option<
-        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
-    > = None;
+    const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)> =
+        None;
     const CLASS_BASE_INIT: Option<
-        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
+        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void),
     > = None;
 }
 
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index c6bb76a7926..a539062aa30 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -45,8 +45,8 @@
 pub mod device_class;
 pub mod memory_ops;
 
-pub const TYPE_PL011: &::core::ffi::CStr = c"pl011";
-pub const TYPE_PL011_LUMINARY: &::core::ffi::CStr = c"pl011_luminary";
+pub const TYPE_PL011: &::std::ffi::CStr = c"pl011";
+pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c"pl011_luminary";
 
 /// Offset of each register from the base memory address of the device.
 ///
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
index fc69922fbf3..169d485a4d2 100644
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ b/rust/hw/char/pl011/src/memory_ops.rs
@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 use core::ptr::NonNull;
+use std::os::raw::{c_uint, c_void};
 
 use qemu_api::{bindings::*, zeroable::Zeroable};
 
@@ -22,11 +23,7 @@
     },
 };
 
-unsafe extern "C" fn pl011_read(
-    opaque: *mut core::ffi::c_void,
-    addr: hwaddr,
-    size: core::ffi::c_uint,
-) -> u64 {
+unsafe extern "C" fn pl011_read(opaque: *mut c_void, addr: hwaddr, size: c_uint) -> u64 {
     assert!(!opaque.is_null());
     let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
     let val = unsafe { state.as_mut().read(addr, size) };
@@ -43,12 +40,7 @@
     }
 }
 
-unsafe extern "C" fn pl011_write(
-    opaque: *mut core::ffi::c_void,
-    addr: hwaddr,
-    data: u64,
-    _size: core::ffi::c_uint,
-) {
+unsafe extern "C" fn pl011_write(opaque: *mut c_void, addr: hwaddr, data: u64, _size: c_uint) {
     unsafe {
         assert!(!opaque.is_null());
         let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index 064afe60549..26597934bbd 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -4,7 +4,7 @@
 
 //! Definitions required by QEMU when registering a device.
 
-use ::core::ffi::{c_void, CStr};
+use std::{ffi::CStr, os::raw::c_void};
 
 use crate::bindings::{Object, ObjectClass, TypeInfo};
 
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 3d40256f60f..cb4573ca6ef 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -7,7 +7,7 @@ macro_rules! device_class_init {
     ($func:ident, props => $props:ident, realize_fn => $realize_fn:expr, legacy_reset_fn => $legacy_reset_fn:expr, vmsd => $vmsd:ident$(,)*) => {
         pub unsafe extern "C" fn $func(
             klass: *mut $crate::bindings::ObjectClass,
-            _: *mut ::core::ffi::c_void,
+            _: *mut ::std::os::raw::c_void,
         ) {
             let mut dc =
                 ::core::ptr::NonNull::new(klass.cast::<$crate::bindings::DeviceClass>()).unwrap();
@@ -26,7 +26,7 @@ macro_rules! define_property {
     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
         $crate::bindings::Property {
             // use associated function syntax for type checking
-            name: ::core::ffi::CStr::as_ptr($name),
+            name: ::std::ffi::CStr::as_ptr($name),
             info: $prop,
             offset: ::core::mem::offset_of!($state, $field) as isize,
             set_default: true,
@@ -37,7 +37,7 @@ macro_rules! define_property {
     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
         $crate::bindings::Property {
             // use associated function syntax for type checking
-            name: ::core::ffi::CStr::as_ptr($name),
+            name: ::std::ffi::CStr::as_ptr($name),
             info: $prop,
             offset: ::core::mem::offset_of!($state, $field) as isize,
             set_default: false,
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 10ab3d7e639..ed840ee2f72 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -34,7 +34,10 @@ unsafe impl Sync for bindings::VMStateInfo {}
 pub mod vmstate;
 pub mod zeroable;
 
-use std::alloc::{GlobalAlloc, Layout};
+use std::{
+    alloc::{GlobalAlloc, Layout},
+    os::raw::c_void,
+};
 
 #[cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)]
 extern "C" {
@@ -48,8 +51,8 @@ fn g_aligned_alloc0(
 
 #[cfg(not(HAVE_GLIB_WITH_ALIGNED_ALLOC))]
 extern "C" {
-    fn qemu_memalign(alignment: usize, size: usize) -> *mut ::core::ffi::c_void;
-    fn qemu_vfree(ptr: *mut ::core::ffi::c_void);
+    fn qemu_memalign(alignment: usize, size: usize) -> *mut c_void;
+    fn qemu_vfree(ptr: *mut c_void);
 }
 
 extern "C" {
@@ -114,7 +117,7 @@ fn default() -> Self {
 }
 
 // Sanity check.
-const _: [(); 8] = [(); ::core::mem::size_of::<*mut ::core::ffi::c_void>()];
+const _: [(); 8] = [(); ::core::mem::size_of::<*mut c_void>()];
 
 unsafe impl GlobalAlloc for QemuAllocator {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 2a626137e25..5dd783a902a 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -55,7 +55,7 @@ macro_rules! vmstate_single_test {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), 0)
                 .as_bytes()
-                .as_ptr() as *const ::core::ffi::c_char,
+                .as_ptr() as *const ::std::os::raw::c_char,
             err_hint: ::core::ptr::null(),
             offset: ::core::mem::offset_of!($struct_name, $field_name),
             size: $size,
@@ -132,7 +132,7 @@ macro_rules! vmstate_array {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), 0)
                 .as_bytes()
-                .as_ptr() as *const ::core::ffi::c_char,
+                .as_ptr() as *const ::std::os::raw::c_char,
             err_hint: ::core::ptr::null(),
             offset: ::core::mem::offset_of!($struct_name, $field_name),
             size: $size,
@@ -180,7 +180,7 @@ macro_rules! vmstate_struct_pointer_v {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), 0)
                 .as_bytes()
-                .as_ptr() as *const ::core::ffi::c_char,
+                .as_ptr() as *const ::std::os::raw::c_char,
             err_hint: ::core::ptr::null(),
             offset: ::core::mem::offset_of!($struct_name, $field_name),
             size: ::core::mem::size_of::<*const $type>(),
@@ -205,7 +205,7 @@ macro_rules! vmstate_array_of_pointer {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), 0)
                 .as_bytes()
-                .as_ptr() as *const ::core::ffi::c_char,
+                .as_ptr() as *const ::std::os::raw::c_char,
             version_id: $version_id,
             num: $num as _,
             info: $info,
@@ -230,7 +230,7 @@ macro_rules! vmstate_array_of_pointer_to_struct {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), 0)
                 .as_bytes()
-                .as_ptr() as *const ::core::ffi::c_char,
+                .as_ptr() as *const ::std::os::raw::c_char,
             version_id: $version_id,
             num: $num as _,
             vmsd: $vmsd,
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 37c4dd44f81..c7089f0cf21 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -2,7 +2,7 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use core::ffi::CStr;
+use std::{ffi::CStr, os::raw::c_void};
 
 use qemu_api::{
     bindings::*,
@@ -64,11 +64,10 @@ impl ObjectImpl for DummyState {
     }
 
     impl Class for DummyClass {
-        const CLASS_INIT: Option<
-            unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
-        > = Some(dummy_class_init);
+        const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)> =
+            Some(dummy_class_init);
         const CLASS_BASE_INIT: Option<
-            unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
+            unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void),
         > = None;
     }
 
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 11/23] rust: introduce a c_str macro
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (9 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 10/23] rust: use std::os::raw instead of core::ffi Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-31 10:39   ` Zhao Liu
  2024-10-25 16:01 ` [PATCH 12/23] rust: silence unknown warnings for the sake of old compilers Paolo Bonzini
                   ` (15 subsequent siblings)
  26 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

This allows CStr constants to be defined easily on Rust 1.63.0, while
checking that there are no embedded NULs.  c"" literals were only
stabilized in Rust 1.77.0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs       |  5 ++-
 rust/hw/char/pl011/src/device_class.rs | 18 ++++-----
 rust/hw/char/pl011/src/lib.rs          |  6 ++-
 rust/qemu-api/meson.build              |  4 ++
 rust/qemu-api/src/c_str.rs             | 53 ++++++++++++++++++++++++++
 rust/qemu-api/src/lib.rs               |  1 +
 rust/qemu-api/src/vmstate.rs           |  2 +-
 rust/qemu-api/tests/tests.rs           |  8 ++--
 8 files changed, 78 insertions(+), 19 deletions(-)
 create mode 100644 rust/qemu-api/src/c_str.rs

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 036757f7f3a..2d225d544de 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -10,6 +10,7 @@
 
 use qemu_api::{
     bindings::{self, *},
+    c_str,
     definitions::ObjectImpl,
 };
 
@@ -135,7 +136,7 @@ impl PL011State {
     /// location/instance. All its fields are expected to hold unitialized
     /// values with the sole exception of `parent_obj`.
     unsafe fn init(&mut self) {
-        const CLK_NAME: &CStr = c"clk";
+        const CLK_NAME: &CStr = c_str!("clk");
 
         let dev = addr_of_mut!(*self).cast::<DeviceState>();
         // SAFETY:
@@ -598,7 +599,7 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
         let dev: *mut DeviceState = qdev_new(PL011State::TYPE_INFO.name);
         let sysbus: *mut SysBusDevice = dev.cast::<SysBusDevice>();
 
-        qdev_prop_set_chr(dev, c"chardev".as_ptr(), chr);
+        qdev_prop_set_chr(dev, c_str!("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);
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 27cabbd6651..8a2019e5345 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -2,14 +2,12 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use core::{
-    ffi::{c_int, c_void},
-    ptr::NonNull,
-};
+use core::ptr::NonNull;
+use std::os::raw::{c_int, c_void};
 
 use qemu_api::{
-    bindings::*, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections, vmstate_uint32,
-    vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
+    bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections,
+    vmstate_uint32, vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
 };
 
 use crate::device::{PL011State, PL011_FIFO_DEPTH};
@@ -24,7 +22,7 @@ extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool {
 
 /// Migration subsection for [`PL011State`] clock.
 pub static VMSTATE_PL011_CLOCK: VMStateDescription = VMStateDescription {
-    name: c"pl011/clock".as_ptr(),
+    name: c_str!("pl011/clock").as_ptr(),
     version_id: 1,
     minimum_version_id: 1,
     needed: Some(pl011_clock_needed),
@@ -43,7 +41,7 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
 }
 
 pub static VMSTATE_PL011: VMStateDescription = VMStateDescription {
-    name: c"pl011".as_ptr(),
+    name: c_str!("pl011").as_ptr(),
     version_id: 2,
     minimum_version_id: 2,
     post_load: Some(pl011_post_load),
@@ -74,14 +72,14 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
 qemu_api::declare_properties! {
     PL011_PROPERTIES,
     qemu_api::define_property!(
-        c"chardev",
+        c_str!("chardev"),
         PL011State,
         char_backend,
         unsafe { &qdev_prop_chr },
         CharBackend
     ),
     qemu_api::define_property!(
-        c"migrate-clk",
+        c_str!("migrate-clk"),
         PL011State,
         migrate_clock,
         unsafe { &qdev_prop_bool },
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index a539062aa30..a98d5cf9574 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -41,12 +41,14 @@
 extern crate bilge_impl;
 extern crate qemu_api;
 
+use qemu_api::c_str;
+
 pub mod device;
 pub mod device_class;
 pub mod memory_ops;
 
-pub const TYPE_PL011: &::std::ffi::CStr = c"pl011";
-pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c"pl011_luminary";
+pub const TYPE_PL011: &::std::ffi::CStr = c_str!("pl011");
+pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c_str!("pl011_luminary");
 
 /// Offset of each register from the base memory address of the device.
 ///
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 3b849f7c413..c950b008d59 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -3,6 +3,7 @@ _qemu_api_rs = static_library(
   structured_sources(
     [
       'src/lib.rs',
+      'src/c_str.rs',
       'src/definitions.rs',
       'src/device_class.rs',
       'src/vmstate.rs',
@@ -18,6 +19,9 @@ _qemu_api_rs = static_library(
   ],
 )
 
+rust.test('rust-qemu-api-tests', _qemu_api_rs,
+          suite: ['unit', 'rust'])
+
 qemu_api = declare_dependency(
   link_with: _qemu_api_rs,
   dependencies: qemu_api_macros,
diff --git a/rust/qemu-api/src/c_str.rs b/rust/qemu-api/src/c_str.rs
new file mode 100644
index 00000000000..cc6d8dc1211
--- /dev/null
+++ b/rust/qemu-api/src/c_str.rs
@@ -0,0 +1,53 @@
+// Copyright 2024 Red Hat, Inc.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#[macro_export]
+/// Given a string constant _without_ embedded or trailing NULs, return
+/// a CStr.
+///
+/// Needed for compatibility with Rust <1.77.
+macro_rules! c_str {
+    ($str:expr) => {{
+        const STRING: &str = concat!($str, "\0");
+        const BYTES: &[u8] = STRING.as_bytes();
+
+        // "for" is not allowed in const context... oh well,
+        // everybody loves some lisp.  This could be turned into
+        // a procedural macro if this is a problem; alternatively
+        // Rust 1.72 makes CStr::from_bytes_with_nul a const function.
+        const fn f(b: &[u8], i: usize) {
+            if i == b.len() - 1 {
+            } else if b[i] == 0 {
+                panic!("c_str argument contains NUL")
+            } else {
+                f(b, i + 1)
+            }
+        }
+        f(BYTES, 0);
+
+        // SAFETY: absence of NULs apart from the final byte was checked above
+        unsafe { std::ffi::CStr::from_bytes_with_nul_unchecked(BYTES) }
+    }};
+}
+
+#[cfg(test)]
+mod tests {
+    use std::ffi::CStr;
+
+    use crate::c_str;
+
+    #[test]
+    fn test_cstr_macro() {
+        let good = c_str!("🦀");
+        let good_bytes = b"\xf0\x9f\xa6\x80\0";
+        assert_eq!(good.to_bytes_with_nul(), good_bytes);
+    }
+
+    #[test]
+    fn test_cstr_macro_const() {
+        const GOOD: &CStr = c_str!("🦀");
+        const GOOD_BYTES: &[u8] = b"\xf0\x9f\xa6\x80\0";
+        assert_eq!(GOOD.to_bytes_with_nul(), GOOD_BYTES);
+    }
+}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index ed840ee2f72..e6bd953e10b 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -29,6 +29,7 @@ unsafe impl Sync for bindings::VMStateDescription {}
 unsafe impl Sync for bindings::VMStateField {}
 unsafe impl Sync for bindings::VMStateInfo {}
 
+pub mod c_str;
 pub mod definitions;
 pub mod device_class;
 pub mod vmstate;
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 5dd783a902a..0aaaef1c7c8 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -14,7 +14,7 @@
 macro_rules! vmstate_unused_buffer {
     ($field_exists_fn:expr, $version_id:expr, $size:expr) => {{
         $crate::bindings::VMStateField {
-            name: c"unused".as_ptr(),
+            name: c_str!("unused").as_ptr(),
             err_hint: ::core::ptr::null(),
             offset: 0,
             size: $size,
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index c7089f0cf21..381ac84657b 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -6,7 +6,7 @@
 
 use qemu_api::{
     bindings::*,
-    declare_properties, define_property,
+    c_str, declare_properties, define_property,
     definitions::{Class, ObjectImpl},
     device_class_init,
     zeroable::Zeroable,
@@ -16,7 +16,7 @@
 fn test_device_decl_macros() {
     // Test that macros can compile.
     pub static VMSTATE: VMStateDescription = VMStateDescription {
-        name: c"name".as_ptr(),
+        name: c_str!("name").as_ptr(),
         unmigratable: true,
         ..Zeroable::ZERO
     };
@@ -36,7 +36,7 @@ pub struct DummyClass {
     declare_properties! {
         DUMMY_PROPERTIES,
             define_property!(
-                c"migrate-clk",
+                c_str!("migrate-clk"),
                 DummyState,
                 migrate_clock,
                 unsafe { &qdev_prop_bool },
@@ -55,7 +55,7 @@ pub struct DummyClass {
     impl ObjectImpl for DummyState {
         type Class = DummyClass;
         const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self };
-        const TYPE_NAME: &'static CStr = c"dummy";
+        const TYPE_NAME: &'static CStr = c_str!("dummy");
         const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_DEVICE);
         const ABSTRACT: bool = false;
         const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 12/23] rust: silence unknown warnings for the sake of old compilers
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (10 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 11/23] rust: introduce a c_str macro Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-25 16:01 ` [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock Paolo Bonzini
                   ` (14 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Occasionally, we may need to silence warnings and clippy lints that
were only introduced in newer Rust compiler versions.  However, this
would fail when compiling with an older rustc:

error: unknown lint: `non_local_definitions`
   --> rust/qemu-api/rust-qemu-api-tests.p/structured/offset_of.rs:79:17

So by default we need to block the unknown_lints warning.  To avoid
misspelled lints or other similar issues, re-enable it in the CI job
that uses nightly rust.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                   | 8 ++++++++
 .gitlab-ci.d/buildtest.yml    | 2 +-
 meson_options.txt             | 2 ++
 scripts/meson-buildoptions.sh | 4 ++++
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index e819148f36c..69b8367eb3c 100644
--- a/meson.build
+++ b/meson.build
@@ -3327,6 +3327,14 @@ if have_rust and have_system
   # Prohibit code that is forbidden in Rust 2024
   rustc_args += ['-D', 'unsafe_op_in_unsafe_fn']
 
+  # Occasionally, we may need to silence warnings and clippy lints that
+  # were only introduced in newer Rust compiler versions.  Do not croak
+  # in that case; a CI job with rust_strict_lints == true ensures that
+  # we do not have misspelled allow() attributes.
+  if not get_option('strict_rust_lints')
+    rustc_args += ['-A', 'unknown_lints']
+  endif
+
   # Apart from procedural macros, our Rust executables will often link
   # with C code, so include all the libraries that C code needs.  This
   # is safe; https://github.com/rust-lang/rust/pull/54675 says that
diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 19ba5b9c818..aba65ff833a 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -128,7 +128,7 @@ build-system-fedora-rust-nightly:
     job: amd64-fedora-rust-nightly-container
   variables:
     IMAGE: fedora-rust-nightly
-    CONFIGURE_ARGS: --disable-docs --enable-rust
+    CONFIGURE_ARGS: --disable-docs --enable-rust --enable-strict-rust-lints
     TARGETS: aarch64-softmmu
     MAKE_CHECK_ARGS: check-build
   allow_failure: true
diff --git a/meson_options.txt b/meson_options.txt
index 0ee4d7bb86b..e46199a3232 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -376,3 +376,5 @@ option('x86_version', type : 'combo', choices : ['0', '1', '2', '3', '4'], value
 
 option('rust', type: 'feature', value: 'disabled',
        description: 'Rust support')
+option('strict_rust_lints', type: 'boolean', value: false,
+       description: 'Enable stricter set of Rust warnings')
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 6d08605b771..e898b20307d 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -47,6 +47,8 @@ meson_options_help() {
   printf "%s\n" '                           getrandom()'
   printf "%s\n" '  --enable-safe-stack      SafeStack Stack Smash Protection (requires'
   printf "%s\n" '                           clang/llvm and coroutine backend ucontext)'
+  printf "%s\n" '  --enable-strict-rust-lints'
+  printf "%s\n" '                           Enable stricter set of Rust warnings'
   printf "%s\n" '  --enable-strip           Strip targets on install'
   printf "%s\n" '  --enable-tcg-interpreter TCG with bytecode interpreter (slow)'
   printf "%s\n" '  --enable-trace-backends=CHOICES'
@@ -490,6 +492,8 @@ _meson_option_parse() {
     --disable-spice-protocol) printf "%s" -Dspice_protocol=disabled ;;
     --enable-stack-protector) printf "%s" -Dstack_protector=enabled ;;
     --disable-stack-protector) printf "%s" -Dstack_protector=disabled ;;
+    --enable-strict-rust-lints) printf "%s" -Dstrict_rust_lints=true ;;
+    --disable-strict-rust-lints) printf "%s" -Dstrict_rust_lints=false ;;
     --enable-strip) printf "%s" -Dstrip=true ;;
     --disable-strip) printf "%s" -Dstrip=false ;;
     --sysconfdir=*) quote_sh "-Dsysconfdir=$2" ;;
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (11 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 12/23] rust: silence unknown warnings for the sake of old compilers Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-31 11:31   ` Zhao Liu
  2024-11-01 10:14   ` Junjie Mao
  2024-10-25 16:01 ` [PATCH 14/23] rust: create a cargo workspace Paolo Bonzini
                   ` (13 subsequent siblings)
  26 siblings, 2 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

The next commit will introduce a new build.rs dependency for rust/qemu-api,
version_check.  Before adding it, ensure that all dependencies are
synchronized between the Meson- and cargo-based build systems.

Note that it's not clear whether in the long term we'll use Cargo for
anything; it seems that the three main uses (clippy, rustfmt, rustdoc)
can all be invoked manually---either via glue code in QEMU, or by
extending Meson to gain the relevant functionality.  However, for
the time being we're stuck with Cargo so it should at least look at
the same code as the rest of the build system.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/Cargo.lock   |  3 +++
 rust/qemu-api-macros/Cargo.lock |  9 ++++---
 rust/qemu-api/Cargo.lock        | 47 +++++++++++++++++++++++++++++++++
 rust/qemu-api/Cargo.toml        |  1 +
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/rust/hw/char/pl011/Cargo.lock b/rust/hw/char/pl011/Cargo.lock
index b58cebb186e..9f43b33e8b8 100644
--- a/rust/hw/char/pl011/Cargo.lock
+++ b/rust/hw/char/pl011/Cargo.lock
@@ -91,6 +91,9 @@ dependencies = [
 [[package]]
 name = "qemu_api"
 version = "0.1.0"
+dependencies = [
+ "qemu_api_macros",
+]
 
 [[package]]
 name = "qemu_api_macros"
diff --git a/rust/qemu-api-macros/Cargo.lock b/rust/qemu-api-macros/Cargo.lock
index fdc0fce116c..f989e25829f 100644
--- a/rust/qemu-api-macros/Cargo.lock
+++ b/rust/qemu-api-macros/Cargo.lock
@@ -4,9 +4,9 @@ version = 3
 
 [[package]]
 name = "proc-macro2"
-version = "1.0.86"
+version = "1.0.84"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5e719e8df665df0d1c8fbfd238015744736151d4445ec0836b8e628aae103b77"
+checksum = "ec96c6a92621310b51366f1e28d05ef11489516e93be030060e5fc12024a49d6"
 dependencies = [
  "unicode-ident",
 ]
@@ -18,6 +18,7 @@ dependencies = [
  "proc-macro2",
  "quote",
  "syn",
+ "unicode-ident",
 ]
 
 [[package]]
@@ -31,9 +32,9 @@ dependencies = [
 
 [[package]]
 name = "syn"
-version = "2.0.72"
+version = "2.0.66"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "dc4b9b9bf2add8093d3f2c0204471e951b2285580335de42f9d2534f3ae7a8af"
+checksum = "c42f3f41a2de00b01c0aaad383c5a45241efc8b2d1eda5661812fda5f3cdcff5"
 dependencies = [
  "proc-macro2",
  "quote",
diff --git a/rust/qemu-api/Cargo.lock b/rust/qemu-api/Cargo.lock
index e9c51a243a8..e407911cdd1 100644
--- a/rust/qemu-api/Cargo.lock
+++ b/rust/qemu-api/Cargo.lock
@@ -2,6 +2,53 @@
 # It is not intended for manual editing.
 version = 3
 
+[[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 = "qemu_api"
 version = "0.1.0"
+dependencies = [
+ "qemu_api_macros",
+]
+
+[[package]]
+name = "qemu_api_macros"
+version = "0.1.0"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn",
+]
+
+[[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"
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index 3677def3fe2..db594c64083 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -14,6 +14,7 @@ keywords = []
 categories = []
 
 [dependencies]
+qemu_api_macros = { path = "../qemu-api-macros" }
 
 [features]
 default = []
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 14/23] rust: create a cargo workspace
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (12 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock Paolo Bonzini
@ 2024-10-25 16:01 ` Paolo Bonzini
  2024-10-31 13:46   ` Zhao Liu
  2024-11-01 10:21   ` Junjie Mao
  2024-10-25 16:02 ` [PATCH 15/23] rust: introduce alternative implementation of offset_of! Paolo Bonzini
                   ` (12 subsequent siblings)
  26 siblings, 2 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Workspaces allows tracking dependencies for multiple crates at once,
by having a single Cargo.lock file at the top of the rust/ tree.
Because QEMU's Cargo.lock files have to be synchronized with the versions
of crates in subprojects/, using a workspace avoids the need to copy
over the Cargo.lock file when adding a new device (and thus a new crate)
under rust/hw/.

In addition, workspaces let cargo download and build dependencies just
once.  While right now we have one leaf crate (hw/char/pl011), this
will not be the case once more devices are added.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/{hw/char/pl011 => }/Cargo.lock |  0
 rust/Cargo.toml                     |  7 ++++
 rust/hw/char/pl011/Cargo.toml       |  3 --
 rust/qemu-api-macros/Cargo.lock     | 48 -------------------------
 rust/qemu-api-macros/Cargo.toml     |  3 --
 rust/qemu-api/Cargo.lock            | 54 -----------------------------
 rust/qemu-api/Cargo.toml            |  3 --
 7 files changed, 7 insertions(+), 111 deletions(-)
 rename rust/{hw/char/pl011 => }/Cargo.lock (100%)
 create mode 100644 rust/Cargo.toml
 delete mode 100644 rust/qemu-api-macros/Cargo.lock
 delete mode 100644 rust/qemu-api/Cargo.lock

diff --git a/rust/hw/char/pl011/Cargo.lock b/rust/Cargo.lock
similarity index 100%
rename from rust/hw/char/pl011/Cargo.lock
rename to rust/Cargo.lock
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
new file mode 100644
index 00000000000..0c94d5037da
--- /dev/null
+++ b/rust/Cargo.toml
@@ -0,0 +1,7 @@
+[workspace]
+resolver = "2"
+members = [
+    "qemu-api-macros",
+    "qemu-api",
+    "hw/char/pl011",
+]
diff --git a/rust/hw/char/pl011/Cargo.toml b/rust/hw/char/pl011/Cargo.toml
index b089e3dded6..a373906b9fb 100644
--- a/rust/hw/char/pl011/Cargo.toml
+++ b/rust/hw/char/pl011/Cargo.toml
@@ -21,6 +21,3 @@ bilge = { version = "0.2.0" }
 bilge-impl = { version = "0.2.0" }
 qemu_api = { path = "../../../qemu-api" }
 qemu_api_macros = { path = "../../../qemu-api-macros" }
-
-# Do not include in any global workspace
-[workspace]
diff --git a/rust/qemu-api-macros/Cargo.lock b/rust/qemu-api-macros/Cargo.lock
deleted file mode 100644
index f989e25829f..00000000000
--- a/rust/qemu-api-macros/Cargo.lock
+++ /dev/null
@@ -1,48 +0,0 @@
-# This file is automatically @generated by Cargo.
-# It is not intended for manual editing.
-version = 3
-
-[[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 = "qemu_api_macros"
-version = "0.1.0"
-dependencies = [
- "proc-macro2",
- "quote",
- "syn",
- "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"
diff --git a/rust/qemu-api-macros/Cargo.toml b/rust/qemu-api-macros/Cargo.toml
index 144cc3650fa..f8d6d03609f 100644
--- a/rust/qemu-api-macros/Cargo.toml
+++ b/rust/qemu-api-macros/Cargo.toml
@@ -20,6 +20,3 @@ proc-macro = true
 proc-macro2 = "1"
 quote = "1"
 syn = "2"
-
-# Do not include in any global workspace
-[workspace]
diff --git a/rust/qemu-api/Cargo.lock b/rust/qemu-api/Cargo.lock
deleted file mode 100644
index e407911cdd1..00000000000
--- a/rust/qemu-api/Cargo.lock
+++ /dev/null
@@ -1,54 +0,0 @@
-# This file is automatically @generated by Cargo.
-# It is not intended for manual editing.
-version = 3
-
-[[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 = "qemu_api"
-version = "0.1.0"
-dependencies = [
- "qemu_api_macros",
-]
-
-[[package]]
-name = "qemu_api_macros"
-version = "0.1.0"
-dependencies = [
- "proc-macro2",
- "quote",
- "syn",
-]
-
-[[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"
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index db594c64083..e092f61e8f3 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -20,8 +20,5 @@ qemu_api_macros = { path = "../qemu-api-macros" }
 default = []
 allocator = []
 
-# Do not include in any global workspace
-[workspace]
-
 [lints.rust]
 unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)'] }
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 15/23] rust: introduce alternative implementation of offset_of!
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (13 preceding siblings ...)
  2024-10-25 16:01 ` [PATCH 14/23] rust: create a cargo workspace Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-11-03  9:54   ` Junjie Mao
  2024-10-25 16:02 ` [PATCH 16/23] rust: do not use MaybeUninit::zeroed() Paolo Bonzini
                   ` (11 subsequent siblings)
  26 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

From: Junjie Mao <junjie.mao@hotmail.com>

offset_of! was stabilized in Rust 1.77.0.  Use an alternative implemenation
that was found on the Rust forums, and whose author agreed to license as
MIT for use in QEMU.

The alternative allows only one level of field access, but apart
from this can be used just by replacing core::mem::offset_of! with
qemu_api::offset_of!.

The actual implementation of offset_of! is done in a declarative macro,
but for simplicity and to avoid introducing an extra level of indentation,
the trigger is a procedural macro #[derive(offsets)].

The procedural macro is perhaps a bit overengineered, but it helps
introducing some idioms that will be useful in the future as well.

Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/Cargo.lock                               |   1 +
 rust/hw/char/pl011/src/device.rs              |   2 +-
 rust/qemu-api-macros/Cargo.toml               |   2 +-
 rust/qemu-api-macros/src/lib.rs               |  81 ++++++++-
 rust/qemu-api/Cargo.toml                      |   6 +-
 rust/qemu-api/build.rs                        |   9 +
 rust/qemu-api/meson.build                     |  12 +-
 rust/qemu-api/src/device_class.rs             |   8 +-
 rust/qemu-api/src/lib.rs                      |   4 +
 rust/qemu-api/src/offset_of.rs                | 161 ++++++++++++++++++
 rust/qemu-api/src/vmstate.rs                  |  10 +-
 rust/qemu-api/tests/tests.rs                  |   1 +
 subprojects/packagefiles/syn-2-rs/meson.build |   1 +
 13 files changed, 280 insertions(+), 18 deletions(-)
 create mode 100644 rust/qemu-api/src/offset_of.rs

diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 9f43b33e8b8..c0c6069247a 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -93,6 +93,7 @@ name = "qemu_api"
 version = "0.1.0"
 dependencies = [
  "qemu_api_macros",
+ "version_check",
 ]
 
 [[package]]
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2d225d544de..bca727e37f0 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -55,7 +55,7 @@ impl DeviceId {
 }
 
 #[repr(C)]
-#[derive(Debug, qemu_api_macros::Object)]
+#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
 /// PL011 Device Model in QEMU
 pub struct PL011State {
     pub parent_obj: SysBusDevice,
diff --git a/rust/qemu-api-macros/Cargo.toml b/rust/qemu-api-macros/Cargo.toml
index f8d6d03609f..a8f7377106b 100644
--- a/rust/qemu-api-macros/Cargo.toml
+++ b/rust/qemu-api-macros/Cargo.toml
@@ -19,4 +19,4 @@ proc-macro = true
 [dependencies]
 proc-macro2 = "1"
 quote = "1"
-syn = "2"
+syn = { version = "2", features = ["extra-traits"] }
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index a4bc5d01ee8..c2ea22101e4 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -3,8 +3,34 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 use proc_macro::TokenStream;
-use quote::quote;
-use syn::{parse_macro_input, DeriveInput};
+use proc_macro2::Span;
+use quote::{quote, quote_spanned};
+use syn::{
+    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
+    Fields, Ident, Type, Visibility,
+};
+
+struct CompileError(String, Span);
+
+impl From<CompileError> for proc_macro2::TokenStream {
+    fn from(err: CompileError) -> Self {
+        let CompileError(msg, span) = err;
+        quote_spanned! { span => compile_error!(#msg); }
+    }
+}
+
+fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), CompileError> {
+    let expected = parse_quote! { #[repr(C)] };
+
+    if input.attrs.iter().any(|attr| attr == &expected) {
+        Ok(())
+    } else {
+        Err(CompileError(
+            format!("#[repr(C)] required for {}", msg),
+            input.ident.span(),
+        ))
+    }
+}
 
 #[proc_macro_derive(Object)]
 pub fn derive_object(input: TokenStream) -> TokenStream {
@@ -21,3 +53,48 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
 
     TokenStream::from(expanded)
 }
+
+fn get_fields(input: &DeriveInput) -> Result<&Punctuated<Field, Comma>, CompileError> {
+    if let Data::Struct(s) = &input.data {
+        if let Fields::Named(fs) = &s.fields {
+            Ok(&fs.named)
+        } else {
+            Err(CompileError(
+                "Cannot generate offsets for unnamed fields.".to_string(),
+                input.ident.span(),
+            ))
+        }
+    } else {
+        Err(CompileError(
+            "Cannot generate offsets for union or enum.".to_string(),
+            input.ident.span(),
+        ))
+    }
+}
+
+#[rustfmt::skip::macros(quote)]
+fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, CompileError> {
+    is_c_repr(&input, "#[derive(offsets)]")?;
+
+    let name = &input.ident;
+    let fields = get_fields(&input)?;
+    let field_names: Vec<&Ident> = fields.iter().map(|f| f.ident.as_ref().unwrap()).collect();
+    let field_types: Vec<&Type> = fields.iter().map(|f| &f.ty).collect();
+    let field_vis: Vec<&Visibility> = fields.iter().map(|f| &f.vis).collect();
+
+    Ok(quote! {
+	::qemu_api::with_offsets! {
+	    struct #name {
+		#(#field_vis #field_names: #field_types,)*
+	    }
+	}
+    })
+}
+
+#[proc_macro_derive(offsets)]
+pub fn derive_offsets(input: TokenStream) -> TokenStream {
+    let input = parse_macro_input!(input as DeriveInput);
+    let expanded = derive_offsets_or_error(input).unwrap_or_else(Into::into);
+
+    TokenStream::from(expanded)
+}
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index e092f61e8f3..cc716d75d46 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -16,9 +16,13 @@ categories = []
 [dependencies]
 qemu_api_macros = { path = "../qemu-api-macros" }
 
+[build-dependencies]
+version_check = "~0.9"
+
 [features]
 default = []
 allocator = []
 
 [lints.rust]
-unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)'] }
+unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)',
+                                                 'cfg(has_offset_of)'] }
diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
index 419b154c2d2..20f8f718b90 100644
--- a/rust/qemu-api/build.rs
+++ b/rust/qemu-api/build.rs
@@ -4,6 +4,8 @@
 
 use std::path::Path;
 
+use version_check as rustc;
+
 fn main() {
     if !Path::new("src/bindings.rs").exists() {
         panic!(
@@ -11,4 +13,11 @@ fn main() {
              (`ninja bindings.rs`) and copy them to src/bindings.rs, or build through meson."
         );
     }
+
+    // Check for available rustc features
+    if rustc::is_min_version("1.77.0").unwrap_or(false) {
+        println!("cargo:rustc-cfg=has_offset_of");
+    }
+
+    println!("cargo:rerun-if-changed=build.rs");
 }
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index c950b008d59..6f637af7b1b 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -1,3 +1,9 @@
+_qemu_api_cfg = ['--cfg', 'MESON']
+# _qemu_api_cfg += ['--cfg', 'feature="allocator"']
+if rustc.version().version_compare('>=1.77.0')
+  _qemu_api_cfg += ['--cfg', 'has_offset_of']
+endif
+
 _qemu_api_rs = static_library(
   'qemu_api',
   structured_sources(
@@ -6,6 +12,7 @@ _qemu_api_rs = static_library(
       'src/c_str.rs',
       'src/definitions.rs',
       'src/device_class.rs',
+      'src/offset_of.rs',
       'src/vmstate.rs',
       'src/zeroable.rs',
     ],
@@ -13,10 +20,7 @@ _qemu_api_rs = static_library(
   ),
   override_options: ['rust_std=2021', 'build.rust_std=2021'],
   rust_abi: 'rust',
-  rust_args: [
-    '--cfg', 'MESON',
-    # '--cfg', 'feature="allocator"',
-  ],
+  rust_args: _qemu_api_cfg,
 )
 
 rust.test('rust-qemu-api-tests', _qemu_api_rs,
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index cb4573ca6ef..56608c7f7fc 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -23,23 +23,23 @@ macro_rules! device_class_init {
 
 #[macro_export]
 macro_rules! define_property {
-    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
         $crate::bindings::Property {
             // use associated function syntax for type checking
             name: ::std::ffi::CStr::as_ptr($name),
             info: $prop,
-            offset: ::core::mem::offset_of!($state, $field) as isize,
+            offset: $crate::offset_of!($state, $field) as isize,
             set_default: true,
             defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval as u64 },
             ..$crate::zeroable::Zeroable::ZERO
         }
     };
-    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
+    ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr$(,)*) => {
         $crate::bindings::Property {
             // use associated function syntax for type checking
             name: ::std::ffi::CStr::as_ptr($name),
             info: $prop,
-            offset: ::core::mem::offset_of!($state, $field) as isize,
+            offset: $crate::offset_of!($state, $field) as isize,
             set_default: false,
             ..$crate::zeroable::Zeroable::ZERO
         }
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index e6bd953e10b..aa8d16ec94b 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -32,6 +32,7 @@ unsafe impl Sync for bindings::VMStateInfo {}
 pub mod c_str;
 pub mod definitions;
 pub mod device_class;
+pub mod offset_of;
 pub mod vmstate;
 pub mod zeroable;
 
@@ -169,3 +170,6 @@ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
         }
     }
 }
+
+#[cfg(has_offset_of)]
+pub use core::mem::offset_of;
diff --git a/rust/qemu-api/src/offset_of.rs b/rust/qemu-api/src/offset_of.rs
new file mode 100644
index 00000000000..075e98f986b
--- /dev/null
+++ b/rust/qemu-api/src/offset_of.rs
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: MIT
+
+/// This macro provides the same functionality as `core::mem::offset_of`,
+/// except that only one level of field access is supported.  The declaration
+/// of the struct must be wrapped with `with_offsets! { }`.
+///
+/// It is needed because `offset_of!` was only stabilized in Rust 1.77.
+#[cfg(not(has_offset_of))]
+#[macro_export]
+macro_rules! offset_of {
+    ($Container:ty, $field:ident) => {
+        <$Container>::OFFSET_TO__.$field
+    };
+}
+
+/// A wrapper for struct declarations, that allows using `offset_of!` in
+/// versions of Rust prior to 1.77
+#[macro_export]
+macro_rules! with_offsets {
+    // This method to generate field offset constants comes from:
+    //
+    //     https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=10a22a9b8393abd7b541d8fc844bc0df
+    //
+    // used under MIT license with permission of Yandros aka Daniel Henry-Mantilla
+    (
+        $(#[$struct_meta:meta])*
+        $struct_vis:vis
+        struct $StructName:ident {
+            $(
+                $(#[$field_meta:meta])*
+                $field_vis:vis
+                $field_name:ident : $field_ty:ty
+            ),*
+            $(,)?
+        }
+    ) => (
+        #[cfg(not(has_offset_of))]
+        const _: () = {
+            struct StructOffsetsHelper<T>(std::marker::PhantomData<T>);
+            const END_OF_PREV_FIELD: usize = 0;
+
+            // populate StructOffsetsHelper<T> with associated consts,
+            // one for each field
+            $crate::with_offsets! {
+                @struct $StructName
+                @names [ $($field_name)* ]
+                @tys [ $($field_ty ,)*]
+            }
+
+            // now turn StructOffsetsHelper<T>'s consts into a single struct,
+            // applying field visibility.  This provides better error messages
+            // than if offset_of! used StructOffsetsHelper::<T> directly.
+            pub
+            struct StructOffsets {
+                $(
+                    $field_vis
+                    $field_name: usize,
+                )*
+            }
+            impl $StructName {
+                pub
+                const OFFSET_TO__: StructOffsets = StructOffsets {
+                    $(
+                        $field_name: StructOffsetsHelper::<$StructName>::$field_name,
+                    )*
+                };
+            }
+        };
+    );
+
+    (
+        @struct $StructName:ident
+        @names []
+        @tys []
+    ) => ();
+
+    (
+        @struct $StructName:ident
+        @names [$field_name:ident $($other_names:tt)*]
+        @tys [$field_ty:ty , $($other_tys:tt)*]
+    ) => (
+        #[allow(non_local_definitions)]
+        #[allow(clippy::modulo_one)]
+        impl StructOffsetsHelper<$StructName> {
+            #[allow(nonstandard_style)]
+            const $field_name: usize = {
+                const ALIGN: usize = std::mem::align_of::<$field_ty>();
+                const TRAIL: usize = END_OF_PREV_FIELD % ALIGN;
+                END_OF_PREV_FIELD + (if TRAIL == 0 { 0usize } else { ALIGN - TRAIL })
+            };
+        }
+        const _: () = {
+            const END_OF_PREV_FIELD: usize =
+                StructOffsetsHelper::<$StructName>::$field_name +
+                std::mem::size_of::<$field_ty>()
+            ;
+            $crate::with_offsets! {
+                @struct $StructName
+                @names [$($other_names)*]
+                @tys [$($other_tys)*]
+            }
+        };
+    );
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::offset_of;
+
+    #[repr(C)]
+    struct Foo {
+        a: u16,
+        b: u32,
+        c: u64,
+        d: u16,
+    }
+
+    #[repr(C)]
+    struct Bar {
+        pub a: u16,
+        pub b: u64,
+        c: Foo,
+        d: u64,
+    }
+
+    crate::with_offsets! {
+        #[repr(C)]
+        struct Bar {
+            pub a: u16,
+            pub b: u64,
+            c: Foo,
+            d: u64,
+        }
+    }
+
+    #[repr(C)]
+    pub struct Baz {
+        b: u32,
+        a: u8,
+    }
+    crate::with_offsets! {
+        #[repr(C)]
+        pub struct Baz {
+            b: u32,
+            a: u8,
+        }
+    }
+
+    #[test]
+    fn test_offset_of() {
+        const OFFSET_TO_C: usize = offset_of!(Bar, c);
+
+        assert_eq!(offset_of!(Bar, a), 0);
+        assert_eq!(offset_of!(Bar, b), 8);
+        assert_eq!(OFFSET_TO_C, 16);
+        assert_eq!(offset_of!(Bar, d), 40);
+
+        assert_eq!(offset_of!(Baz, b), 0);
+        assert_eq!(offset_of!(Baz, a), 4);
+    }
+}
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 0aaaef1c7c8..b452d01ae3e 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -57,7 +57,7 @@ macro_rules! vmstate_single_test {
                 .as_bytes()
                 .as_ptr() as *const ::std::os::raw::c_char,
             err_hint: ::core::ptr::null(),
-            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            offset: $crate::offset_of!($struct_name, $field_name),
             size: $size,
             start: 0,
             num: 0,
@@ -134,7 +134,7 @@ macro_rules! vmstate_array {
                 .as_bytes()
                 .as_ptr() as *const ::std::os::raw::c_char,
             err_hint: ::core::ptr::null(),
-            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            offset: $crate::offset_of!($struct_name, $field_name),
             size: $size,
             start: 0,
             num: $length as _,
@@ -182,7 +182,7 @@ macro_rules! vmstate_struct_pointer_v {
                 .as_bytes()
                 .as_ptr() as *const ::std::os::raw::c_char,
             err_hint: ::core::ptr::null(),
-            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            offset: $crate::offset_of!($struct_name, $field_name),
             size: ::core::mem::size_of::<*const $type>(),
             start: 0,
             num: 0,
@@ -211,7 +211,7 @@ macro_rules! vmstate_array_of_pointer {
             info: $info,
             size: ::core::mem::size_of::<*const $type>(),
             flags: VMStateFlags(VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0),
-            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            offset: $crate::offset_of!($struct_name, $field_name),
             err_hint: ::core::ptr::null(),
             start: 0,
             num_offset: 0,
@@ -240,7 +240,7 @@ macro_rules! vmstate_array_of_pointer_to_struct {
                     | VMStateFlags::VMS_STRUCT.0
                     | VMStateFlags::VMS_ARRAY_OF_POINTER.0,
             ),
-            offset: ::core::mem::offset_of!($struct_name, $field_name),
+            offset: $crate::offset_of!($struct_name, $field_name),
             err_hint: ::core::ptr::null(),
             start: 0,
             num_offset: 0,
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 381ac84657b..7442f695646 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -21,6 +21,7 @@ fn test_device_decl_macros() {
         ..Zeroable::ZERO
     };
 
+    #[derive(qemu_api_macros::offsets)]
     #[repr(C)]
     #[derive(qemu_api_macros::Object)]
     pub struct DummyState {
diff --git a/subprojects/packagefiles/syn-2-rs/meson.build b/subprojects/packagefiles/syn-2-rs/meson.build
index a53335f3092..9f56ce1c24d 100644
--- a/subprojects/packagefiles/syn-2-rs/meson.build
+++ b/subprojects/packagefiles/syn-2-rs/meson.build
@@ -24,6 +24,7 @@ _syn_rs = static_library(
     '--cfg', 'feature="printing"',
     '--cfg', 'feature="clone-impls"',
     '--cfg', 'feature="proc-macro"',
+    '--cfg', 'feature="extra-traits"',
   ],
   dependencies: [
     quote_dep,
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 16/23] rust: do not use MaybeUninit::zeroed()
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (14 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 15/23] rust: introduce alternative implementation of offset_of! Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 16:02 ` [PATCH 17/23] rust: clean up detection of the language Paolo Bonzini
                   ` (10 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

MaybeUninit::zeroed() is handy but is not available as a "const" function
until Rust 1.75.0.

Remove the default implementation of Zeroable::ZERO, and write by hand
the definitions for those types that need it.  It may be possible to
add automatic implementation of the trait, via a procedural macro and/or
a trick similar to offset_of!, but do it the easy way for now.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/zeroable.rs | 91 +++++++++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 14 deletions(-)

diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 45ec95c9f70..13cdb2ccba5 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -1,23 +1,86 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
+use std::ptr;
+
 /// Encapsulates the requirement that
-/// `MaybeUninit::<Self>::zeroed().assume_init()` does not cause
-/// undefined behavior.
+/// `MaybeUninit::<Self>::zeroed().assume_init()` does not cause undefined
+/// behavior.  This trait in principle could be implemented as just:
+///
+/// ```
+///     const ZERO: Self = unsafe {
+///         ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init()
+///     },
+/// ```
+///
+/// The need for a manual implementation is only because `zeroed()` cannot
+/// be used as a `const fn` prior to Rust 1.75.0. Once we can assume a new
+/// enough version of the compiler, we could provide a `#[derive(Zeroable)]`
+/// macro to check at compile-time that all struct fields are Zeroable, and
+/// use the above blanket implementation of the `ZERO` constant.
 ///
 /// # Safety
 ///
-/// Do not add this trait to a type unless all-zeroes is
-/// a valid value for the type.  In particular, remember that raw
-/// pointers can be zero, but references and `NonNull<T>` cannot
-/// unless wrapped with `Option<>`.
+/// Because the implementation of `ZERO` is manual, it does not make
+/// any assumption on the safety of `zeroed()`.  However, other users of the
+/// trait could use it that way.  Do not add this trait to a type unless
+/// all-zeroes is a valid value for the type.  In particular, remember that
+/// raw pointers can be zero, but references and `NonNull<T>` cannot
 pub unsafe trait Zeroable: Default {
-    /// SAFETY: If the trait was added to a type, then by definition
-    /// this is safe.
-    const ZERO: Self = unsafe { ::core::mem::MaybeUninit::<Self>::zeroed().assume_init() };
+    const ZERO: Self;
 }
 
-unsafe impl Zeroable for crate::bindings::Property__bindgen_ty_1 {}
-unsafe impl Zeroable for crate::bindings::Property {}
-unsafe impl Zeroable for crate::bindings::VMStateDescription {}
-unsafe impl Zeroable for crate::bindings::MemoryRegionOps__bindgen_ty_1 {}
-unsafe impl Zeroable for crate::bindings::MemoryRegionOps__bindgen_ty_2 {}
+unsafe impl Zeroable for crate::bindings::Property__bindgen_ty_1 {
+    const ZERO: Self = Self { i: 0 };
+}
+
+unsafe impl Zeroable for crate::bindings::Property {
+    const ZERO: Self = Self {
+        name: ptr::null(),
+        info: ptr::null(),
+        offset: 0,
+        bitnr: 0,
+        bitmask: 0,
+        set_default: false,
+        defval: Zeroable::ZERO,
+        arrayoffset: 0,
+        arrayinfo: ptr::null(),
+        arrayfieldsize: 0,
+        link_type: ptr::null(),
+    };
+}
+
+unsafe impl Zeroable for crate::bindings::VMStateDescription {
+    const ZERO: Self = Self {
+        name: ptr::null(),
+        unmigratable: false,
+        early_setup: false,
+        version_id: 0,
+        minimum_version_id: 0,
+        priority: crate::bindings::MigrationPriority::MIG_PRI_DEFAULT,
+        pre_load: None,
+        post_load: None,
+        pre_save: None,
+        post_save: None,
+        needed: None,
+        dev_unplug_pending: None,
+        fields: ptr::null(),
+        subsections: ptr::null(),
+    };
+}
+
+unsafe impl Zeroable for crate::bindings::MemoryRegionOps__bindgen_ty_1 {
+    const ZERO: Self = Self {
+        min_access_size: 0,
+        max_access_size: 0,
+        unaligned: false,
+        accepts: None,
+    };
+}
+
+unsafe impl Zeroable for crate::bindings::MemoryRegionOps__bindgen_ty_2 {
+    const ZERO: Self = Self {
+        min_access_size: 0,
+        max_access_size: 0,
+        unaligned: false,
+    };
+}
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 17/23] rust: clean up detection of the language
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (15 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 16/23] rust: do not use MaybeUninit::zeroed() Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 16:02 ` [PATCH 18/23] rust: allow version 1.63.0 of rustc Paolo Bonzini
                   ` (9 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Disable the detection code altogether if have_system == false.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/meson.build b/meson.build
index 69b8367eb3c..d199bcdd824 100644
--- a/meson.build
+++ b/meson.build
@@ -53,6 +53,17 @@ cpu = host_machine.cpu_family()
 
 target_dirs = config_host['TARGET_DIRS'].split()
 
+# type of binaries to build
+have_linux_user = false
+have_bsd_user = false
+have_system = false
+foreach target : target_dirs
+  have_linux_user = have_linux_user or target.endswith('linux-user')
+  have_bsd_user = have_bsd_user or target.endswith('bsd-user')
+  have_system = have_system or target.endswith('-softmmu')
+endforeach
+have_user = have_linux_user or have_bsd_user
+
 ############
 # Programs #
 ############
@@ -71,11 +82,13 @@ if host_os == 'darwin' and \
   all_languages += ['objc']
   objc = meson.get_compiler('objc')
 endif
-have_rust = false
-if not get_option('rust').disabled() and add_languages('rust', required: get_option('rust'), native: false) \
-    and add_languages('rust', required: get_option('rust'), native: true)
+
+have_rust = add_languages('rust', native: false,
+    required: get_option('rust').disable_auto_if(not have_system))
+have_rust = have_rust and add_languages('rust', native: true,
+    required: get_option('rust').disable_auto_if(not have_system))
+if have_rust
   rustc = meson.get_compiler('rust')
-  have_rust = true
   if rustc.version().version_compare('<1.80.0')
     if get_option('rust').enabled()
       error('rustc version ' + rustc.version() + ' is unsupported: Please upgrade to at least 1.80.0')
@@ -186,17 +199,6 @@ have_vhost_net_vdpa = have_vhost_vdpa and get_option('vhost_net').allowed()
 have_vhost_net_kernel = have_vhost_kernel and get_option('vhost_net').allowed()
 have_vhost_net = have_vhost_net_kernel or have_vhost_net_user or have_vhost_net_vdpa
 
-# type of binaries to build
-have_linux_user = false
-have_bsd_user = false
-have_system = false
-foreach target : target_dirs
-  have_linux_user = have_linux_user or target.endswith('linux-user')
-  have_bsd_user = have_bsd_user or target.endswith('bsd-user')
-  have_system = have_system or target.endswith('-softmmu')
-endforeach
-have_user = have_linux_user or have_bsd_user
-
 have_tools = get_option('tools') \
   .disable_auto_if(not have_system) \
   .allowed()
@@ -3317,7 +3319,7 @@ endif
 
 genh += configure_file(output: 'config-host.h', configuration: config_host_data)
 
-if have_rust and have_system
+if have_rust
   rustc_args = run_command(
     find_program('scripts/rust/rustc_args.py'),
     '--config-headers', meson.project_build_root() / 'config-host.h',
@@ -3937,7 +3939,7 @@ common_all = static_library('common',
                             implicit_include_directories: false,
                             dependencies: common_ss.all_dependencies())
 
-if have_rust and have_system
+if have_rust
   bindgen_args = [
     '--disable-header-comment',
     '--raw-line', '// @generated',
@@ -4091,7 +4093,7 @@ foreach target : target_dirs
   arch_srcs += target_specific.sources()
   arch_deps += target_specific.dependencies()
 
-  if have_rust and have_system
+  if have_rust and target_type == 'system'
     target_rust = rust_devices_ss.apply(config_target, strict: false)
     crates = []
     foreach dep : target_rust.dependencies()
@@ -4453,9 +4455,9 @@ else
 endif
 summary_info += {'Rust support':      have_rust}
 if have_rust
-  summary_info += {'rustc version':   rustc.version()}
-  summary_info += {'rustc':           ' '.join(rustc.cmd_array())}
   summary_info += {'Rust target':     config_host['RUST_TARGET_TRIPLE']}
+  summary_info += {'rustc':           ' '.join(rustc.cmd_array())}
+  summary_info += {'rustc version':   rustc.version()}
 endif
 option_cflags = (get_option('debug') ? ['-g'] : [])
 if get_option('optimization') != 'plain'
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 18/23] rust: allow version 1.63.0 of rustc
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (16 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 17/23] rust: clean up detection of the language Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 16:02 ` [PATCH 19/23] rust: do not use --generate-cstr Paolo Bonzini
                   ` (8 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

All constructs introduced by newer versions of Rust have been removed.

Apart from Debian 12, all other supported Linux distributions have
rustc 1.75.0 or newer.  This means that they only lack c"" literals
and stable offset_of!.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index d199bcdd824..ab0613f353c 100644
--- a/meson.build
+++ b/meson.build
@@ -89,11 +89,12 @@ have_rust = have_rust and add_languages('rust', native: true,
     required: get_option('rust').disable_auto_if(not have_system))
 if have_rust
   rustc = meson.get_compiler('rust')
-  if rustc.version().version_compare('<1.80.0')
+  if rustc.version().version_compare('<1.63.0')
     if get_option('rust').enabled()
-      error('rustc version ' + rustc.version() + ' is unsupported: Please upgrade to at least 1.80.0')
+      error('rustc version ' + rustc.version() + ' is unsupported. Please upgrade to at least 1.63.0')
     else
-      warning('rustc version ' + rustc.version() + ' is unsupported: Disabling Rust compilation. Please upgrade to at least 1.80.0 to use Rust.')
+      warning('rustc version ' + rustc.version() + ' is unsupported, disabling Rust compilation.')
+      message('Please upgrade to at least 1.63.0 to use Rust.')
       have_rust = false
     endif
   endif
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 19/23] rust: do not use --generate-cstr
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (17 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 18/23] rust: allow version 1.63.0 of rustc Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 20:03   ` Michael Tokarev
  2024-10-25 16:02 ` [PATCH 20/23] rust: allow older version of bindgen Paolo Bonzini
                   ` (7 subsequent siblings)
  26 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

--generate-cstr is a good idea and generally the right thing to do,
but it is not available in Debian 12 and Ubuntu 22.04.  Work around
the absence.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                       |  4 +++-
 rust/hw/char/pl011/src/device.rs  |  1 +
 rust/qemu-api/src/device_class.rs | 10 ++++++++++
 rust/qemu-api/tests/tests.rs      |  4 ++--
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index ab0613f353c..bfd04294850 100644
--- a/meson.build
+++ b/meson.build
@@ -3941,13 +3941,15 @@ common_all = static_library('common',
                             dependencies: common_ss.all_dependencies())
 
 if have_rust
+  # We would like to use --generate-cstr, but it is only available
+  # starting with bindgen 0.66.0.  The oldest supported versions
+  # are in Ubuntu 22.04 (0.59.1) and Debian 12 (0.60.1).
   bindgen_args = [
     '--disable-header-comment',
     '--raw-line', '// @generated',
     '--ctypes-prefix', 'std::os::raw',
     '--formatter', 'rustfmt',
     '--generate-block',
-    '--generate-cstr',
     '--impl-debug',
     '--merge-extern-blocks',
     '--no-doc-comments',
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index bca727e37f0..2a85960b81f 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -12,6 +12,7 @@
     bindings::{self, *},
     c_str,
     definitions::ObjectImpl,
+    device_class::TYPE_SYS_BUS_DEVICE,
 };
 
 use crate::{
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 56608c7f7fc..0ba798d3e3c 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -2,6 +2,10 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
+use std::ffi::CStr;
+
+use crate::bindings;
+
 #[macro_export]
 macro_rules! device_class_init {
     ($func:ident, props => $props:ident, realize_fn => $realize_fn:expr, legacy_reset_fn => $legacy_reset_fn:expr, vmsd => $vmsd:ident$(,)*) => {
@@ -62,3 +66,9 @@ macro_rules! declare_properties {
         ];
     };
 }
+
+// workaround until we can use --generate-cstr in bindgen.
+pub const TYPE_DEVICE: &CStr =
+    unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
+pub const TYPE_SYS_BUS_DEVICE: &CStr =
+    unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_SYS_BUS_DEVICE) };
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 7442f695646..43a4827de12 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -8,7 +8,7 @@
     bindings::*,
     c_str, declare_properties, define_property,
     definitions::{Class, ObjectImpl},
-    device_class_init,
+    device_class, device_class_init,
     zeroable::Zeroable,
 };
 
@@ -57,7 +57,7 @@ impl ObjectImpl for DummyState {
         type Class = DummyClass;
         const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self };
         const TYPE_NAME: &'static CStr = c_str!("dummy");
-        const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_DEVICE);
+        const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE);
         const ABSTRACT: bool = false;
         const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
         const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 20/23] rust: allow older version of bindgen
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (18 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 19/23] rust: do not use --generate-cstr Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 16:02 ` [PATCH 21/23] rust: make rustfmt optional Paolo Bonzini
                   ` (6 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Cope with the old version that is provided in Debian 12.

--size_t-is-usize is needed on bindgen <0.61.0, and it was removed in
bindgen 0.65.0, so check for it in meson.build.

--merge-extern-blocks was added in 0.61.0.

--formatter rustfmt was added in 0.65.0 and is the default, so remove it.

Apart from Debian 12 and Ubuntu 22.04, all other supported distros have
version 0.66.x of bindgen or newer (or do not have bindgen at all).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/about/build-platforms.rst | 12 ++++++++++++
 meson.build                    | 29 +++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/docs/about/build-platforms.rst b/docs/about/build-platforms.rst
index 8fd7da140a3..ff56091078e 100644
--- a/docs/about/build-platforms.rst
+++ b/docs/about/build-platforms.rst
@@ -107,6 +107,18 @@ Python build dependencies
   required, it may be necessary to fetch python modules from the Python
   Package Index (PyPI) via ``pip``, in order to build QEMU.
 
+Rust build dependencies
+  QEMU is generally conservative in adding new Rust dependencies, and all
+  of them are included in the distributed tarballs.  One exception is the
+  bindgen tool, which is too big to package and distribute.  The minimum
+  supported version of bindgen is 0.60.x.  For distributions that do not
+  include bindgen or have an older version, it is recommended to install
+  a newer version using ``cargo install bindgen-cli``.
+
+  Developers may want to use Cargo-based tools in the QEMU source tree;
+  this requires Cargo 1.74.0.  Note that Cargo is not required in order
+  to build QEMU.
+
 Optional build dependencies
   Build components whose absence does not affect the ability to build
   QEMU may not be available in distros, or may be too old for QEMU's
diff --git a/meson.build b/meson.build
index bfd04294850..fb0295b312c 100644
--- a/meson.build
+++ b/meson.build
@@ -100,6 +100,21 @@ if have_rust
   endif
 endif
 
+bindgen = find_program('bindgen', required: get_option('rust').disable_auto_if(not have_rust))
+if not bindgen.found() or bindgen.version().version_compare('<0.60.0')
+  if get_option('rust').enabled()
+    error('bindgen version ' + bindgen.version() + ' is unsupported. You can install a new version with "cargo install bindgen-cli"')
+  else
+    if bindgen.found()
+      warning('bindgen version ' + bindgen.version() + ' is unsupported, disabling Rust compilation.')
+    else
+      warning('bindgen not found, disabling Rust compilation.')
+    endif
+    message('To use Rust you can install a new version with "cargo install bindgen-cli"')
+    have_rust = false
+  endif
+endif
+
 dtrace = not_found
 stap = not_found
 if 'dtrace' in get_option('trace_backends')
@@ -3943,15 +3958,13 @@ common_all = static_library('common',
 if have_rust
   # We would like to use --generate-cstr, but it is only available
   # starting with bindgen 0.66.0.  The oldest supported versions
-  # are in Ubuntu 22.04 (0.59.1) and Debian 12 (0.60.1).
+  # is 0.60.x (Debian 12 has 0.60.1) which introduces --allowlist-file.
   bindgen_args = [
     '--disable-header-comment',
     '--raw-line', '// @generated',
     '--ctypes-prefix', 'std::os::raw',
-    '--formatter', 'rustfmt',
     '--generate-block',
     '--impl-debug',
-    '--merge-extern-blocks',
     '--no-doc-comments',
     '--with-derive-default',
     '--no-layout-tests',
@@ -3960,6 +3973,12 @@ if have_rust
     '--allowlist-file', meson.project_source_root() + '/.*',
     '--allowlist-file', meson.project_build_root() + '/.*'
     ]
+  if bindgen.version().version_compare('<0.61.0')
+    # default in 0.61+
+    bindgen_args += ['--size_t-is-usize']
+  else
+    bindgen_args += ['--merge-extern-blocks']
+  endif
   c_enums = [
     'DeviceCategory',
     'GpioPolarity',
@@ -3995,7 +4014,7 @@ if have_rust
     dependencies: common_ss.all_dependencies(),
     output: 'bindings.rs',
     include_directories: include_directories('.', 'include'),
-    bindgen_version: ['>=0.69.4'],
+    bindgen_version: ['>=0.60.0'],
     args: bindgen_args,
     )
   subdir('rust')
@@ -4461,6 +4480,8 @@ if have_rust
   summary_info += {'Rust target':     config_host['RUST_TARGET_TRIPLE']}
   summary_info += {'rustc':           ' '.join(rustc.cmd_array())}
   summary_info += {'rustc version':   rustc.version()}
+  summary_info += {'bindgen':         bindgen.full_path()}
+  summary_info += {'bindgen version': bindgen.version()}
 endif
 option_cflags = (get_option('debug') ? ['-g'] : [])
 if get_option('optimization') != 'plain'
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 21/23] rust: make rustfmt optional
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (19 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 20/23] rust: allow older version of bindgen Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 16:02 ` [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04 Paolo Bonzini
                   ` (5 subsequent siblings)
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/meson.build b/meson.build
index fb0295b312c..3db4dcc8a24 100644
--- a/meson.build
+++ b/meson.build
@@ -115,6 +115,10 @@ if not bindgen.found() or bindgen.version().version_compare('<0.60.0')
   endif
 endif
 
+if have_rust
+  rustfmt = find_program('rustfmt', required: false)
+endif
+
 dtrace = not_found
 stap = not_found
 if 'dtrace' in get_option('trace_backends')
@@ -3973,6 +3977,13 @@ if have_rust
     '--allowlist-file', meson.project_source_root() + '/.*',
     '--allowlist-file', meson.project_build_root() + '/.*'
     ]
+  if not rustfmt.found()
+    if bindgen.version().version_compare('<0.65.0')
+      bindgen_args += ['--no-rustfmt-bindings']
+    else
+      bindgen_args += ['--formatter', 'none']
+    endif
+  endif
   if bindgen.version().version_compare('<0.61.0')
     # default in 0.61+
     bindgen_args += ['--size_t-is-usize']
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (20 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 21/23] rust: make rustfmt optional Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 18:51   ` Pierrick Bouvier
  2024-10-25 20:08   ` Pierrick Bouvier
  2024-10-25 16:02 ` [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job Paolo Bonzini
                   ` (4 subsequent siblings)
  26 siblings, 2 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Because Ubuntu 22.04 has a very old version of bindgen, that
does not have the important option --allowlist-file, it will
not be able to use --enable-rust out of the box.  Instead,
install the latest version of bindgen-cli via "cargo install"
in the container, following QEMU's own documentation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/docker/dockerfiles/ubuntu2204.docker |  5 +++++
 tests/lcitool/mappings.yml                 |  4 ++++
 tests/lcitool/refresh                      | 11 ++++++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/ubuntu2204.docker b/tests/docker/dockerfiles/ubuntu2204.docker
index ce3aa39d4f3..245ac879622 100644
--- a/tests/docker/dockerfiles/ubuntu2204.docker
+++ b/tests/docker/dockerfiles/ubuntu2204.docker
@@ -149,6 +149,11 @@ ENV LANG "en_US.UTF-8"
 ENV MAKE "/usr/bin/make"
 ENV NINJA "/usr/bin/ninja"
 ENV PYTHON "/usr/bin/python3"
+ENV CARGO_HOME=/usr/local/cargo
+ENV PATH=$CARGO_HOME/bin:$PATH
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+  apt install -y --no-install-recommends cargo
+RUN cargo install bindgen-cli
 # As a final step configure the user (if env is defined)
 ARG USER
 ARG UID
diff --git a/tests/lcitool/mappings.yml b/tests/lcitool/mappings.yml
index 9c5ac87c1c2..c90b23a00f1 100644
--- a/tests/lcitool/mappings.yml
+++ b/tests/lcitool/mappings.yml
@@ -1,4 +1,8 @@
 mappings:
+  # Too old on Ubuntu 22.04; we install it from cargo instead
+  bindgen:
+    Ubuntu2204:
+
   flake8:
     OpenSUSELeap15:
 
diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
index 0f16f4d525c..a46cbbdca41 100755
--- a/tests/lcitool/refresh
+++ b/tests/lcitool/refresh
@@ -137,6 +137,14 @@ fedora_rustup_nightly_extras = [
     'RUN /usr/local/cargo/bin/rustup run nightly cargo install bindgen-cli\n',
 ]
 
+ubuntu2204_bindgen_extras = [
+    "ENV CARGO_HOME=/usr/local/cargo\n",
+    'ENV PATH=$CARGO_HOME/bin:$PATH\n',
+    "RUN DEBIAN_FRONTEND=noninteractive eatmydata \\\n",
+    "  apt install -y --no-install-recommends cargo\n",
+    'RUN cargo install bindgen-cli\n',
+]
+
 def cross_build(prefix, targets):
     conf = "ENV QEMU_CONFIGURE_OPTS --cross-prefix=%s\n" % (prefix)
     targets = "ENV DEF_TARGET_LIST %s\n" % (targets)
@@ -157,7 +165,8 @@ try:
                         trailer="".join(debian12_extras))
     generate_dockerfile("fedora", "fedora-40")
     generate_dockerfile("opensuse-leap", "opensuse-leap-15")
-    generate_dockerfile("ubuntu2204", "ubuntu-2204")
+    generate_dockerfile("ubuntu2204", "ubuntu-2204",
+                        trailer="".join(ubuntu2204_bindgen_extras))
 
     #
     # Non-fatal Rust-enabled build
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (21 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04 Paolo Bonzini
@ 2024-10-25 16:02 ` Paolo Bonzini
  2024-10-25 18:55   ` Pierrick Bouvier
  2024-10-25 20:08   ` Pierrick Bouvier
  2024-10-25 16:23 ` [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Manos Pitsidianakis
                   ` (3 subsequent siblings)
  26 siblings, 2 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 16:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

We have fixed all incompatibilities with older versions of rustc
and bindgen.  Enable Rust on Debian to check that the minimum
supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .gitlab-ci.d/buildtest.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index aba65ff833a..8deaf9627cb 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -40,7 +40,7 @@ build-system-ubuntu:
     job: amd64-ubuntu2204-container
   variables:
     IMAGE: ubuntu2204
-    CONFIGURE_ARGS: --enable-docs
+    CONFIGURE_ARGS: --enable-docs --enable-rust
     TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu
     MAKE_CHECK_ARGS: check-build
 
@@ -71,7 +71,7 @@ build-system-debian:
     job: amd64-debian-container
   variables:
     IMAGE: debian
-    CONFIGURE_ARGS: --with-coroutine=sigaltstack
+    CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust
     TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4-softmmu
       sparc-softmmu xtensa-softmmu
     MAKE_CHECK_ARGS: check-build
-- 
2.47.0



^ permalink raw reply related	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (22 preceding siblings ...)
  2024-10-25 16:02 ` [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job Paolo Bonzini
@ 2024-10-25 16:23 ` Manos Pitsidianakis
  2024-10-31 16:28   ` Paolo Bonzini
  2024-10-27  7:01 ` Michael Tokarev
                   ` (2 subsequent siblings)
  26 siblings, 1 reply; 67+ messages in thread
From: Manos Pitsidianakis @ 2024-10-25 16:23 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, zhao1.liu, junjie.mao, berrange

Paolo, you picked up my patches without us first talking about it.
This is not how things should work.


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 16:02 ` [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04 Paolo Bonzini
@ 2024-10-25 18:51   ` Pierrick Bouvier
  2024-10-25 19:35     ` Paolo Bonzini
  2024-10-25 20:08   ` Pierrick Bouvier
  1 sibling, 1 reply; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 18:51 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Hi Paolo,

On 10/25/24 09:02, Paolo Bonzini wrote:
> Because Ubuntu 22.04 has a very old version of bindgen, that
> does not have the important option --allowlist-file, it will
> not be able to use --enable-rust out of the box.  Instead,
> install the latest version of bindgen-cli via "cargo install"
> in the container, following QEMU's own documentation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/docker/dockerfiles/ubuntu2204.docker |  5 +++++
>   tests/lcitool/mappings.yml                 |  4 ++++
>   tests/lcitool/refresh                      | 11 ++++++++++-
>   3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/dockerfiles/ubuntu2204.docker b/tests/docker/dockerfiles/ubuntu2204.docker
> index ce3aa39d4f3..245ac879622 100644
> --- a/tests/docker/dockerfiles/ubuntu2204.docker
> +++ b/tests/docker/dockerfiles/ubuntu2204.docker
> @@ -149,6 +149,11 @@ ENV LANG "en_US.UTF-8"
>   ENV MAKE "/usr/bin/make"
>   ENV NINJA "/usr/bin/ninja"
>   ENV PYTHON "/usr/bin/python3"
> +ENV CARGO_HOME=/usr/local/cargo
> +ENV PATH=$CARGO_HOME/bin:$PATH
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +  apt install -y --no-install-recommends cargo
> +RUN cargo install bindgen-cli
>   # As a final step configure the user (if env is defined)
>   ARG USER
>   ARG UID
> diff --git a/tests/lcitool/mappings.yml b/tests/lcitool/mappings.yml
> index 9c5ac87c1c2..c90b23a00f1 100644
> --- a/tests/lcitool/mappings.yml
> +++ b/tests/lcitool/mappings.yml
> @@ -1,4 +1,8 @@
>   mappings:
> +  # Too old on Ubuntu 22.04; we install it from cargo instead
> +  bindgen:
> +    Ubuntu2204:
> +
>     flake8:
>       OpenSUSELeap15:
>   
> diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
> index 0f16f4d525c..a46cbbdca41 100755
> --- a/tests/lcitool/refresh
> +++ b/tests/lcitool/refresh
> @@ -137,6 +137,14 @@ fedora_rustup_nightly_extras = [
>       'RUN /usr/local/cargo/bin/rustup run nightly cargo install bindgen-cli\n',
>   ]
>   
> +ubuntu2204_bindgen_extras = [
> +    "ENV CARGO_HOME=/usr/local/cargo\n",
> +    'ENV PATH=$CARGO_HOME/bin:$PATH\n',
> +    "RUN DEBIAN_FRONTEND=noninteractive eatmydata \\\n",
> +    "  apt install -y --no-install-recommends cargo\n",
> +    'RUN cargo install bindgen-cli\n',
> +]
> +
>   def cross_build(prefix, targets):
>       conf = "ENV QEMU_CONFIGURE_OPTS --cross-prefix=%s\n" % (prefix)
>       targets = "ENV DEF_TARGET_LIST %s\n" % (targets)
> @@ -157,7 +165,8 @@ try:
>                           trailer="".join(debian12_extras))
>       generate_dockerfile("fedora", "fedora-40")
>       generate_dockerfile("opensuse-leap", "opensuse-leap-15")
> -    generate_dockerfile("ubuntu2204", "ubuntu-2204")
> +    generate_dockerfile("ubuntu2204", "ubuntu-2204",
> +                        trailer="".join(ubuntu2204_bindgen_extras))
>   
>       #
>       # Non-fatal Rust-enabled build

Should we install the same version as the minimal one we expect (0.60, 
in debian 12)?

All the rest of series is focused on having fixed minimal version, and 
this patch seems to escape this rule.
Note: we can still install it using cargo, but just having a fixed 
version would be better.


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job
  2024-10-25 16:02 ` [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job Paolo Bonzini
@ 2024-10-25 18:55   ` Pierrick Bouvier
  2024-10-25 18:58     ` Pierrick Bouvier
  2024-10-25 19:27     ` Paolo Bonzini
  2024-10-25 20:08   ` Pierrick Bouvier
  1 sibling, 2 replies; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 18:55 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On 10/25/24 09:02, Paolo Bonzini wrote:
> We have fixed all incompatibilities with older versions of rustc
> and bindgen.  Enable Rust on Debian to check that the minimum
> supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   .gitlab-ci.d/buildtest.yml | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index aba65ff833a..8deaf9627cb 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -40,7 +40,7 @@ build-system-ubuntu:
>       job: amd64-ubuntu2204-container
>     variables:
>       IMAGE: ubuntu2204
> -    CONFIGURE_ARGS: --enable-docs
> +    CONFIGURE_ARGS: --enable-docs --enable-rust
>       TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu
>       MAKE_CHECK_ARGS: check-build
>   
> @@ -71,7 +71,7 @@ build-system-debian:
>       job: amd64-debian-container
>     variables:
>       IMAGE: debian
> -    CONFIGURE_ARGS: --with-coroutine=sigaltstack
> +    CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust
>       TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4-softmmu
>         sparc-softmmu xtensa-softmmu
>       MAKE_CHECK_ARGS: check-build

Do you think it could be valuable to have a third job for Rust with:
- ubuntu2204 or debian with latest rustc/cargo/bindgen, so we may detect 
regressions when those are updated.

This way, we would test (2204 + min, debian + min, latest), which should 
ensure Rust code will build correctly on older and newer systems.

Pierrick


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job
  2024-10-25 18:55   ` Pierrick Bouvier
@ 2024-10-25 18:58     ` Pierrick Bouvier
  2024-10-25 19:27     ` Paolo Bonzini
  1 sibling, 0 replies; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 18:58 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On 10/25/24 11:55, Pierrick Bouvier wrote:
> On 10/25/24 09:02, Paolo Bonzini wrote:
>> We have fixed all incompatibilities with older versions of rustc
>> and bindgen.  Enable Rust on Debian to check that the minimum
>> supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>    .gitlab-ci.d/buildtest.yml | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
>> index aba65ff833a..8deaf9627cb 100644
>> --- a/.gitlab-ci.d/buildtest.yml
>> +++ b/.gitlab-ci.d/buildtest.yml
>> @@ -40,7 +40,7 @@ build-system-ubuntu:
>>        job: amd64-ubuntu2204-container
>>      variables:
>>        IMAGE: ubuntu2204
>> -    CONFIGURE_ARGS: --enable-docs
>> +    CONFIGURE_ARGS: --enable-docs --enable-rust
>>        TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu
>>        MAKE_CHECK_ARGS: check-build
>>    
>> @@ -71,7 +71,7 @@ build-system-debian:
>>        job: amd64-debian-container
>>      variables:
>>        IMAGE: debian
>> -    CONFIGURE_ARGS: --with-coroutine=sigaltstack
>> +    CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust
>>        TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4-softmmu
>>          sparc-softmmu xtensa-softmmu
>>        MAKE_CHECK_ARGS: check-build
> 
> Do you think it could be valuable to have a third job for Rust with:
> - ubuntu2204 or debian with latest rustc/cargo/bindgen, so we may detect
> regressions when those are updated.
> 
> This way, we would test (2204 + min, debian + min, latest), which should
> ensure Rust code will build correctly on older and newer systems.
> 
> Pierrick

The rust latest install can be made as part of the job itself, instead 
of having another container.


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job
  2024-10-25 18:55   ` Pierrick Bouvier
  2024-10-25 18:58     ` Pierrick Bouvier
@ 2024-10-25 19:27     ` Paolo Bonzini
  2024-10-25 19:33       ` Pierrick Bouvier
  1 sibling, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 19:27 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

Il ven 25 ott 2024, 20:55 Pierrick Bouvier <pierrick.bouvier@linaro.org> ha
scritto:

> On 10/25/24 09:02, Paolo Bonzini wrote:
> > We have fixed all incompatibilities with older versions of rustc
> > and bindgen.  Enable Rust on Debian to check that the minimum
> > supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   .gitlab-ci.d/buildtest.yml | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> > index aba65ff833a..8deaf9627cb 100644
> > --- a/.gitlab-ci.d/buildtest.yml
> > +++ b/.gitlab-ci.d/buildtest.yml
> > @@ -40,7 +40,7 @@ build-system-ubuntu:
> >       job: amd64-ubuntu2204-container
> >     variables:
> >       IMAGE: ubuntu2204
> > -    CONFIGURE_ARGS: --enable-docs
> > +    CONFIGURE_ARGS: --enable-docs --enable-rust
> >       TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu
> >       MAKE_CHECK_ARGS: check-build
> >
> > @@ -71,7 +71,7 @@ build-system-debian:
> >       job: amd64-debian-container
> >     variables:
> >       IMAGE: debian
> > -    CONFIGURE_ARGS: --with-coroutine=sigaltstack
> > +    CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust
> >       TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4-softmmu
> >         sparc-softmmu xtensa-softmmu
> >       MAKE_CHECK_ARGS: check-build
>
> Do you think it could be valuable to have a third job for Rust with:
> - ubuntu2204 or debian with latest rustc/cargo/bindgen, so we may detect
> regressions when those are updated.
>

Note that apart from these two jobs we have Fedora with rustup-installed
nightly (in master) and Fedora with distro Rust tool chain (patches
posted). Would that provide the same (or similar enough) scenario?

Paolo


> This way, we would test (2204 + min, debian + min, latest), which should
> ensure Rust code will build correctly on older and newer systems.
>
> Pierrick
>
>

[-- Attachment #2: Type: text/html, Size: 2967 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job
  2024-10-25 19:27     ` Paolo Bonzini
@ 2024-10-25 19:33       ` Pierrick Bouvier
  0 siblings, 0 replies; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 19:33 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

On 10/25/24 12:27, Paolo Bonzini wrote:
> 
> 
> Il ven 25 ott 2024, 20:55 Pierrick Bouvier <pierrick.bouvier@linaro.org 
> <mailto:pierrick.bouvier@linaro.org>> ha scritto:
> 
>     On 10/25/24 09:02, Paolo Bonzini wrote:
>      > We have fixed all incompatibilities with older versions of rustc
>      > and bindgen.  Enable Rust on Debian to check that the minimum
>      > supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen.
>      >
>      > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>>
>      > ---
>      >   .gitlab-ci.d/buildtest.yml | 4 ++--
>      >   1 file changed, 2 insertions(+), 2 deletions(-)
>      >
>      > diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
>      > index aba65ff833a..8deaf9627cb 100644
>      > --- a/.gitlab-ci.d/buildtest.yml
>      > +++ b/.gitlab-ci.d/buildtest.yml
>      > @@ -40,7 +40,7 @@ build-system-ubuntu:
>      >       job: amd64-ubuntu2204-container
>      >     variables:
>      >       IMAGE: ubuntu2204
>      > -    CONFIGURE_ARGS: --enable-docs
>      > +    CONFIGURE_ARGS: --enable-docs --enable-rust
>      >       TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu
>      >       MAKE_CHECK_ARGS: check-build
>      >
>      > @@ -71,7 +71,7 @@ build-system-debian:
>      >       job: amd64-debian-container
>      >     variables:
>      >       IMAGE: debian
>      > -    CONFIGURE_ARGS: --with-coroutine=sigaltstack
>      > +    CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust
>      >       TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4-softmmu
>      >         sparc-softmmu xtensa-softmmu
>      >       MAKE_CHECK_ARGS: check-build
> 
>     Do you think it could be valuable to have a third job for Rust with:
>     - ubuntu2204 or debian with latest rustc/cargo/bindgen, so we may
>     detect
>     regressions when those are updated.
> 
> 
> Note that apart from these two jobs we have Fedora with rustup-installed 
> nightly (in master) and Fedora with distro Rust tool chain (patches 
> posted). Would that provide the same (or similar enough) scenario?
> 

It covers the need yes.
Since we don't need any nightly feature to build the code, maybe we 
could target last stable instead.
For fedora vs ubuntu/debian, I don't have any strong opinion.

> Paolo
> 
> 
>     This way, we would test (2204 + min, debian + min, latest), which
>     should
>     ensure Rust code will build correctly on older and newer systems.
> 
>     Pierrick
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 18:51   ` Pierrick Bouvier
@ 2024-10-25 19:35     ` Paolo Bonzini
  2024-10-25 19:47       ` Pierrick Bouvier
  0 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 19:35 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]

Il ven 25 ott 2024, 20:51 Pierrick Bouvier <pierrick.bouvier@linaro.org> ha
scritto:

> Hi Paolo,
>
> On 10/25/24 09:02, Paolo Bonzini wrote:
> > Because Ubuntu 22.04 has a very old version of bindgen, that
> > does not have the important option --allowlist-file, it will
> > not be able to use --enable-rust out of the box.  Instead,
> > install the latest version of bindgen-cli via "cargo install"
> > in the container, following QEMU's own documentation.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   tests/docker/dockerfiles/ubuntu2204.docker |  5 +++++
> >   tests/lcitool/mappings.yml                 |  4 ++++
> >   tests/lcitool/refresh                      | 11 ++++++++++-
> >   3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/docker/dockerfiles/ubuntu2204.docker
> b/tests/docker/dockerfiles/ubuntu2204.docker
> > index ce3aa39d4f3..245ac879622 100644
> > --- a/tests/docker/dockerfiles/ubuntu2204.docker
> > +++ b/tests/docker/dockerfiles/ubuntu2204.docker
> > @@ -149,6 +149,11 @@ ENV LANG "en_US.UTF-8"
> >   ENV MAKE "/usr/bin/make"
> >   ENV NINJA "/usr/bin/ninja"
> >   ENV PYTHON "/usr/bin/python3"
> > +ENV CARGO_HOME=/usr/local/cargo
> > +ENV PATH=$CARGO_HOME/bin:$PATH
> > +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> > +  apt install -y --no-install-recommends cargo
> > +RUN cargo install bindgen-cli
> >   # As a final step configure the user (if env is defined)
> >   ARG USER
> >   ARG UID
> > diff --git a/tests/lcitool/mappings.yml b/tests/lcitool/mappings.yml
> > index 9c5ac87c1c2..c90b23a00f1 100644
> > --- a/tests/lcitool/mappings.yml
> > +++ b/tests/lcitool/mappings.yml
> > @@ -1,4 +1,8 @@
> >   mappings:
> > +  # Too old on Ubuntu 22.04; we install it from cargo instead
> > +  bindgen:
> > +    Ubuntu2204:
> > +
> >     flake8:
> >       OpenSUSELeap15:
> >
> > diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
> > index 0f16f4d525c..a46cbbdca41 100755
> > --- a/tests/lcitool/refresh
> > +++ b/tests/lcitool/refresh
> > @@ -137,6 +137,14 @@ fedora_rustup_nightly_extras = [
> >       'RUN /usr/local/cargo/bin/rustup run nightly cargo install
> bindgen-cli\n',
> >   ]
> >
> > +ubuntu2204_bindgen_extras = [
> > +    "ENV CARGO_HOME=/usr/local/cargo\n",
> > +    'ENV PATH=$CARGO_HOME/bin:$PATH\n',
> > +    "RUN DEBIAN_FRONTEND=noninteractive eatmydata \\\n",
> > +    "  apt install -y --no-install-recommends cargo\n",
> > +    'RUN cargo install bindgen-cli\n',
> > +]
> > +
> >   def cross_build(prefix, targets):
> >       conf = "ENV QEMU_CONFIGURE_OPTS --cross-prefix=%s\n" % (prefix)
> >       targets = "ENV DEF_TARGET_LIST %s\n" % (targets)
> > @@ -157,7 +165,8 @@ try:
> >                           trailer="".join(debian12_extras))
> >       generate_dockerfile("fedora", "fedora-40")
> >       generate_dockerfile("opensuse-leap", "opensuse-leap-15")
> > -    generate_dockerfile("ubuntu2204", "ubuntu-2204")
> > +    generate_dockerfile("ubuntu2204", "ubuntu-2204",
> > +                        trailer="".join(ubuntu2204_bindgen_extras))
> >
> >       #
> >       # Non-fatal Rust-enabled build
>
> Should we install the same version as the minimal one we expect (0.60,
> in debian 12)? All the rest of series is focused on having fixed minimal
> version, and

this patch seems to escape this rule.
>

But in the end the operation of bindgen is quite deterministic, so if the
coverage is improved we can indeed install 0.60.x. For example, if we think
that user on Debian 12 might use distro bindgen together with a recent
rustc (in their case, rustup-installed), then installing bindgen 0.60.x on
Ubuntu would provide a similar effect.

On the other hand I expect that users will just do "cargo install
bindgen-cli", and Ubuntu is a pretty common distro, so that's what I went
for here.

Paolo

Note: we can still install it using cargo, but just having a fixed
> version would be better.
>
>

[-- Attachment #2: Type: text/html, Size: 5682 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 19:35     ` Paolo Bonzini
@ 2024-10-25 19:47       ` Pierrick Bouvier
  2024-10-25 20:08         ` Paolo Bonzini
  0 siblings, 1 reply; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 19:47 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

On 10/25/24 12:35, Paolo Bonzini wrote:
> 
> 
> Il ven 25 ott 2024, 20:51 Pierrick Bouvier <pierrick.bouvier@linaro.org 
> <mailto:pierrick.bouvier@linaro.org>> ha scritto:
> 
>     Hi Paolo,
> 
>     On 10/25/24 09:02, Paolo Bonzini wrote:
>      > Because Ubuntu 22.04 has a very old version of bindgen, that
>      > does not have the important option --allowlist-file, it will
>      > not be able to use --enable-rust out of the box.  Instead,
>      > install the latest version of bindgen-cli via "cargo install"
>      > in the container, following QEMU's own documentation.
>      >
>      > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>>
>      > ---
>      >   tests/docker/dockerfiles/ubuntu2204.docker |  5 +++++
>      >   tests/lcitool/mappings.yml                 |  4 ++++
>      >   tests/lcitool/refresh                      | 11 ++++++++++-
>      >   3 files changed, 19 insertions(+), 1 deletion(-)
>      >
>      > diff --git a/tests/docker/dockerfiles/ubuntu2204.docker
>     b/tests/docker/dockerfiles/ubuntu2204.docker
>      > index ce3aa39d4f3..245ac879622 100644
>      > --- a/tests/docker/dockerfiles/ubuntu2204.docker
>      > +++ b/tests/docker/dockerfiles/ubuntu2204.docker
>      > @@ -149,6 +149,11 @@ ENV LANG "en_US.UTF-8"
>      >   ENV MAKE "/usr/bin/make"
>      >   ENV NINJA "/usr/bin/ninja"
>      >   ENV PYTHON "/usr/bin/python3"
>      > +ENV CARGO_HOME=/usr/local/cargo
>      > +ENV PATH=$CARGO_HOME/bin:$PATH
>      > +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
>      > +  apt install -y --no-install-recommends cargo
>      > +RUN cargo install bindgen-cli
>      >   # As a final step configure the user (if env is defined)
>      >   ARG USER
>      >   ARG UID
>      > diff --git a/tests/lcitool/mappings.yml b/tests/lcitool/mappings.yml
>      > index 9c5ac87c1c2..c90b23a00f1 100644
>      > --- a/tests/lcitool/mappings.yml
>      > +++ b/tests/lcitool/mappings.yml
>      > @@ -1,4 +1,8 @@
>      >   mappings:
>      > +  # Too old on Ubuntu 22.04; we install it from cargo instead
>      > +  bindgen:
>      > +    Ubuntu2204:
>      > +
>      >     flake8:
>      >       OpenSUSELeap15:
>      >
>      > diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
>      > index 0f16f4d525c..a46cbbdca41 100755
>      > --- a/tests/lcitool/refresh
>      > +++ b/tests/lcitool/refresh
>      > @@ -137,6 +137,14 @@ fedora_rustup_nightly_extras = [
>      >       'RUN /usr/local/cargo/bin/rustup run nightly cargo install
>     bindgen-cli\n',
>      >   ]
>      >
>      > +ubuntu2204_bindgen_extras = [
>      > +    "ENV CARGO_HOME=/usr/local/cargo\n",
>      > +    'ENV PATH=$CARGO_HOME/bin:$PATH\n',
>      > +    "RUN DEBIAN_FRONTEND=noninteractive eatmydata \\\n",
>      > +    "  apt install -y --no-install-recommends cargo\n",
>      > +    'RUN cargo install bindgen-cli\n',
>      > +]
>      > +
>      >   def cross_build(prefix, targets):
>      >       conf = "ENV QEMU_CONFIGURE_OPTS --cross-prefix=%s\n" % (prefix)
>      >       targets = "ENV DEF_TARGET_LIST %s\n" % (targets)
>      > @@ -157,7 +165,8 @@ try:
>      >                           trailer="".join(debian12_extras))
>      >       generate_dockerfile("fedora", "fedora-40")
>      >       generate_dockerfile("opensuse-leap", "opensuse-leap-15")
>      > -    generate_dockerfile("ubuntu2204", "ubuntu-2204")
>      > +    generate_dockerfile("ubuntu2204", "ubuntu-2204",
>      > +                        trailer="".join(ubuntu2204_bindgen_extras))
>      >
>      >       #
>      >       # Non-fatal Rust-enabled build
> 
>     Should we install the same version as the minimal one we expect (0.60,
>     in debian 12)? All the rest of series is focused on having fixed
>     minimal version, and
> 
>     this patch seems to escape this rule.
> 
> 
> But in the end the operation of bindgen is quite deterministic, so if 
> the coverage is improved we can indeed install 0.60.x. For example, if 
> we think that user on Debian 12 might use distro bindgen together with a 
> recent rustc (in their case, rustup-installed), then installing bindgen 
> 0.60.x on Ubuntu would provide a similar effect.
> 

I missed that the debian job covers this use case. So indeed, we can use 
a recent one on ubuntu.

Where is the change for the debian container?

> On the other hand I expect that users will just do "cargo install 
> bindgen-cli", and Ubuntu is a pretty common distro, so that's what I 
> went for here.
>

It's a reasonable expectation indeed.

> Paolo
> 
>     Note: we can still install it using cargo, but just having a fixed
>     version would be better.
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 19/23] rust: do not use --generate-cstr
  2024-10-25 16:02 ` [PATCH 19/23] rust: do not use --generate-cstr Paolo Bonzini
@ 2024-10-25 20:03   ` Michael Tokarev
  2024-10-25 20:06     ` Pierrick Bouvier
  2024-10-25 20:11     ` Paolo Bonzini
  0 siblings, 2 replies; 67+ messages in thread
From: Michael Tokarev @ 2024-10-25 20:03 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

25.10.2024 19:02, Paolo Bonzini wrote:
> --generate-cstr is a good idea and generally the right thing to do,
> but it is not available in Debian 12 and Ubuntu 22.04.  Work around
> the absence.

Can't we just install a more recent bindgen and use all the current
features of it, like it's done in patch 22 for ubuntu?

/mjt


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 19/23] rust: do not use --generate-cstr
  2024-10-25 20:03   ` Michael Tokarev
@ 2024-10-25 20:06     ` Pierrick Bouvier
  2024-10-25 20:10       ` Michael Tokarev
  2024-10-25 20:11     ` Paolo Bonzini
  1 sibling, 1 reply; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 20:06 UTC (permalink / raw)
  To: Michael Tokarev, Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On 10/25/24 13:03, Michael Tokarev wrote:
> 25.10.2024 19:02, Paolo Bonzini wrote:
>> --generate-cstr is a good idea and generally the right thing to do,
>> but it is not available in Debian 12 and Ubuntu 22.04.  Work around
>> the absence.
> 
> Can't we just install a more recent bindgen and use all the current
> features of it, like it's done in patch 22 for ubuntu?
> 

Users yes, but distros expect to be able to use their packaged version.

> /mjt
> 


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 19:47       ` Pierrick Bouvier
@ 2024-10-25 20:08         ` Paolo Bonzini
  2024-10-25 20:14           ` Pierrick Bouvier
  0 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 20:08 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

Il ven 25 ott 2024, 21:47 Pierrick Bouvier <pierrick.bouvier@linaro.org> ha
scritto:

> >  in the end the operation of bindgen is quite deterministic, so if
> > the coverage is improved we can indeed install 0.60.x. For example, if
> > we think that user on Debian 12 might use distro bindgen together with a
> > recent rustc (in their case, rustup-installed), then installing bindgen
> > 0.60.x on Ubuntu would provide a similar effect.
> >
>
> I missed that the debian job covers this use case. So indeed, we can use
> a recent one on ubuntu.
>
> Where is the change for the debian container?
>

Here:
https://lore.kernel.org/qemu-devel/20241015133925.311587-2-berrange@redhat.com/

Without this series, that patch was already installing bindgen and rustc to
test that --enable-rust was not enabled implicitly.

Paolo

> On the other hand I expect that users will just do "cargo install
> > bindgen-cli", and Ubuntu is a pretty common distro, so that's what I
> > went for here.
>
> It's a reasonable expectation indeed.
>

[-- Attachment #2: Type: text/html, Size: 1878 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 16:02 ` [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04 Paolo Bonzini
  2024-10-25 18:51   ` Pierrick Bouvier
@ 2024-10-25 20:08   ` Pierrick Bouvier
  1 sibling, 0 replies; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 20:08 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On 10/25/24 09:02, Paolo Bonzini wrote:
> Because Ubuntu 22.04 has a very old version of bindgen, that
> does not have the important option --allowlist-file, it will
> not be able to use --enable-rust out of the box.  Instead,
> install the latest version of bindgen-cli via "cargo install"
> in the container, following QEMU's own documentation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/docker/dockerfiles/ubuntu2204.docker |  5 +++++
>   tests/lcitool/mappings.yml                 |  4 ++++
>   tests/lcitool/refresh                      | 11 ++++++++++-
>   3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/dockerfiles/ubuntu2204.docker b/tests/docker/dockerfiles/ubuntu2204.docker
> index ce3aa39d4f3..245ac879622 100644
> --- a/tests/docker/dockerfiles/ubuntu2204.docker
> +++ b/tests/docker/dockerfiles/ubuntu2204.docker
> @@ -149,6 +149,11 @@ ENV LANG "en_US.UTF-8"
>   ENV MAKE "/usr/bin/make"
>   ENV NINJA "/usr/bin/ninja"
>   ENV PYTHON "/usr/bin/python3"
> +ENV CARGO_HOME=/usr/local/cargo
> +ENV PATH=$CARGO_HOME/bin:$PATH
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +  apt install -y --no-install-recommends cargo
> +RUN cargo install bindgen-cli
>   # As a final step configure the user (if env is defined)
>   ARG USER
>   ARG UID
> diff --git a/tests/lcitool/mappings.yml b/tests/lcitool/mappings.yml
> index 9c5ac87c1c2..c90b23a00f1 100644
> --- a/tests/lcitool/mappings.yml
> +++ b/tests/lcitool/mappings.yml
> @@ -1,4 +1,8 @@
>   mappings:
> +  # Too old on Ubuntu 22.04; we install it from cargo instead
> +  bindgen:
> +    Ubuntu2204:
> +
>     flake8:
>       OpenSUSELeap15:
>   
> diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
> index 0f16f4d525c..a46cbbdca41 100755
> --- a/tests/lcitool/refresh
> +++ b/tests/lcitool/refresh
> @@ -137,6 +137,14 @@ fedora_rustup_nightly_extras = [
>       'RUN /usr/local/cargo/bin/rustup run nightly cargo install bindgen-cli\n',
>   ]
>   
> +ubuntu2204_bindgen_extras = [
> +    "ENV CARGO_HOME=/usr/local/cargo\n",
> +    'ENV PATH=$CARGO_HOME/bin:$PATH\n',
> +    "RUN DEBIAN_FRONTEND=noninteractive eatmydata \\\n",
> +    "  apt install -y --no-install-recommends cargo\n",
> +    'RUN cargo install bindgen-cli\n',
> +]
> +
>   def cross_build(prefix, targets):
>       conf = "ENV QEMU_CONFIGURE_OPTS --cross-prefix=%s\n" % (prefix)
>       targets = "ENV DEF_TARGET_LIST %s\n" % (targets)
> @@ -157,7 +165,8 @@ try:
>                           trailer="".join(debian12_extras))
>       generate_dockerfile("fedora", "fedora-40")
>       generate_dockerfile("opensuse-leap", "opensuse-leap-15")
> -    generate_dockerfile("ubuntu2204", "ubuntu-2204")
> +    generate_dockerfile("ubuntu2204", "ubuntu-2204",
> +                        trailer="".join(ubuntu2204_bindgen_extras))
>   
>       #
>       # Non-fatal Rust-enabled build

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job
  2024-10-25 16:02 ` [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job Paolo Bonzini
  2024-10-25 18:55   ` Pierrick Bouvier
@ 2024-10-25 20:08   ` Pierrick Bouvier
  1 sibling, 0 replies; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 20:08 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On 10/25/24 09:02, Paolo Bonzini wrote:
> We have fixed all incompatibilities with older versions of rustc
> and bindgen.  Enable Rust on Debian to check that the minimum
> supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   .gitlab-ci.d/buildtest.yml | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index aba65ff833a..8deaf9627cb 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -40,7 +40,7 @@ build-system-ubuntu:
>       job: amd64-ubuntu2204-container
>     variables:
>       IMAGE: ubuntu2204
> -    CONFIGURE_ARGS: --enable-docs
> +    CONFIGURE_ARGS: --enable-docs --enable-rust
>       TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu
>       MAKE_CHECK_ARGS: check-build
>   
> @@ -71,7 +71,7 @@ build-system-debian:
>       job: amd64-debian-container
>     variables:
>       IMAGE: debian
> -    CONFIGURE_ARGS: --with-coroutine=sigaltstack
> +    CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust
>       TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4-softmmu
>         sparc-softmmu xtensa-softmmu
>       MAKE_CHECK_ARGS: check-build

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 19/23] rust: do not use --generate-cstr
  2024-10-25 20:06     ` Pierrick Bouvier
@ 2024-10-25 20:10       ` Michael Tokarev
  2024-10-25 20:12         ` Pierrick Bouvier
  0 siblings, 1 reply; 67+ messages in thread
From: Michael Tokarev @ 2024-10-25 20:10 UTC (permalink / raw)
  To: Pierrick Bouvier, Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

25.10.2024 23:06, Pierrick Bouvier wrote:
> On 10/25/24 13:03, Michael Tokarev wrote:
>> 25.10.2024 19:02, Paolo Bonzini wrote:
>>> --generate-cstr is a good idea and generally the right thing to do,
>>> but it is not available in Debian 12 and Ubuntu 22.04.  Work around
>>> the absence.
>>
>> Can't we just install a more recent bindgen and use all the current
>> features of it, like it's done in patch 22 for ubuntu?
> 
> Users yes, but distros expect to be able to use their packaged version.

Pretty please do not target rust in qemu for *currently* supported
distros.  For debian bookworm it is already way too late, - for
bookworm as a distro, qemu with rust is hopeless, it is possible
only with trixie and up.

Users wishing to experiment can install more recent packages, for
qemu ci it is the way to go too using the way in patch 22, and that's
it.  There's no need to sacrifice qemu rust code for current debian
stable.  rust is already too volatile by its own, and targeting
that wide range of versions is insane.

/mjt



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 19/23] rust: do not use --generate-cstr
  2024-10-25 20:03   ` Michael Tokarev
  2024-10-25 20:06     ` Pierrick Bouvier
@ 2024-10-25 20:11     ` Paolo Bonzini
  1 sibling, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 20:11 UTC (permalink / raw)
  To: Michael Tokarev
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

Il ven 25 ott 2024, 22:03 Michael Tokarev <mjt@tls.msk.ru> ha scritto:

> 25.10.2024 19:02, Paolo Bonzini wrote:
> > --generate-cstr is a good idea and generally the right thing to do,
> > but it is not available in Debian 12 and Ubuntu 22.04.  Work around
> > the absence.
>
> Can't we just install a more recent bindgen and use all the current
> features of it, like it's done in patch 22 for ubuntu?
>

The idea is that Ubuntu will get the memo and add an updated bindgen, since
they did update rustc to 1.75.0... so hopefully even that change in patch
22 is temporary.

This patch is only a minor nuisance and we'll only need it for about a year.

Paolo

/mjt
>
>

[-- Attachment #2: Type: text/html, Size: 1359 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 19/23] rust: do not use --generate-cstr
  2024-10-25 20:10       ` Michael Tokarev
@ 2024-10-25 20:12         ` Pierrick Bouvier
  0 siblings, 0 replies; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 20:12 UTC (permalink / raw)
  To: Michael Tokarev, Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On 10/25/24 13:10, Michael Tokarev wrote:
> 25.10.2024 23:06, Pierrick Bouvier wrote:
>> On 10/25/24 13:03, Michael Tokarev wrote:
>>> 25.10.2024 19:02, Paolo Bonzini wrote:
>>>> --generate-cstr is a good idea and generally the right thing to do,
>>>> but it is not available in Debian 12 and Ubuntu 22.04.  Work around
>>>> the absence.
>>>
>>> Can't we just install a more recent bindgen and use all the current
>>> features of it, like it's done in patch 22 for ubuntu?
>>
>> Users yes, but distros expect to be able to use their packaged version.
> 
> Pretty please do not target rust in qemu for *currently* supported
> distros.  For debian bookworm it is already way too late, - for
> bookworm as a distro, qemu with rust is hopeless, it is possible
> only with trixie and up.
> 
> Users wishing to experiment can install more recent packages, for
> qemu ci it is the way to go too using the way in patch 22, and that's
> it.  There's no need to sacrifice qemu rust code for current debian
> stable.  rust is already too volatile by its own, and targeting
> that wide range of versions is insane.
> 

It's the goal of this series, and with changes included, it can be built 
on debian stable (with packaged rustc/bindgen).

> /mjt
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 20:08         ` Paolo Bonzini
@ 2024-10-25 20:14           ` Pierrick Bouvier
  2024-10-25 20:21             ` Paolo Bonzini
  0 siblings, 1 reply; 67+ messages in thread
From: Pierrick Bouvier @ 2024-10-25 20:14 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

On 10/25/24 13:08, Paolo Bonzini wrote:
> 
> 
> Il ven 25 ott 2024, 21:47 Pierrick Bouvier <pierrick.bouvier@linaro.org 
> <mailto:pierrick.bouvier@linaro.org>> ha scritto:
> 
>      >  in the end the operation of bindgen is quite deterministic, so if
>      > the coverage is improved we can indeed install 0.60.x. For
>     example, if
>      > we think that user on Debian 12 might use distro bindgen together
>     with a
>      > recent rustc (in their case, rustup-installed), then installing
>     bindgen
>      > 0.60.x on Ubuntu would provide a similar effect.
>      >
> 
>     I missed that the debian job covers this use case. So indeed, we can
>     use
>     a recent one on ubuntu.
> 
>     Where is the change for the debian container?
> 
> 
> Here: 
> https://lore.kernel.org/qemu-devel/20241015133925.311587-2-berrange@redhat.com/ <https://lore.kernel.org/qemu-devel/20241015133925.311587-2-berrange@redhat.com/>
> 

Good. As there is no based-on tag in the cover letter, I expected this 
to apply on current master.

> Without this series, that patch was already installing bindgen and rustc 
> to test that --enable-rust was not enabled implicitly.
> 
> Paolo
> 
>      > On the other hand I expect that users will just do "cargo install
>      > bindgen-cli", and Ubuntu is a pretty common distro, so that's what I
>      > went for here.
> 
>     It's a reasonable expectation indeed.
> 

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04
  2024-10-25 20:14           ` Pierrick Bouvier
@ 2024-10-25 20:21             ` Paolo Bonzini
  0 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-25 20:21 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu, Junjie Mao,
	P. Berrange, Daniel

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]

Il ven 25 ott 2024, 22:14 Pierrick Bouvier <pierrick.bouvier@linaro.org> ha
scritto:

> > Here:
> >
> https://lore.kernel.org/qemu-devel/20241015133925.311587-2-berrange@redhat.com/
>
> Good. As there is no based-on tag in the cover letter, I expected this
> to apply on current master.
>

Yes, it's based on my next pull request but I didn't have time to post it
yet (see cover letter).

Paolo


> > Without this series, that patch was already installing bindgen and rustc
> > to test that --enable-rust was not enabled implicitly.
> >
> > Paolo
> >
> >      > On the other hand I expect that users will just do "cargo install
> >      > bindgen-cli", and Ubuntu is a pretty common distro, so that's
> what I
> >      > went for here.
> >
> >     It's a reasonable expectation indeed.
> >
>

[-- Attachment #2: Type: text/html, Size: 1678 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (23 preceding siblings ...)
  2024-10-25 16:23 ` [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Manos Pitsidianakis
@ 2024-10-27  7:01 ` Michael Tokarev
  2024-10-27  8:00   ` Paolo Bonzini
  2024-10-28  9:21   ` Daniel P. Berrangé
  2024-10-30 16:52 ` Paolo Bonzini
  2024-10-31 16:41 ` Zhao Liu
  26 siblings, 2 replies; 67+ messages in thread
From: Michael Tokarev @ 2024-10-27  7:01 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel
  Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

I think this is the wrong direction (ie, backwards).

Sacrificing current code to be compatible with old stuff feels wrong.
Especially for really old, like rustc in debian bookworm.

bookworm has rustc-web (and a few related packages) which is regular
rustc version 1.78, just renamed.  It is regular bookworm, not backports.
It has some packages disabled (compared to regular rust) and is a hack,
but it exists and can be used for now (dunno if it is sufficient for
qemu though).

Also debian has backports mechanism, which also can be used for qemu -
I can try back-porting regular rust (and llvm) to bookworm.

I think this is a better way (at least a way forward) than trying to
move backwards.

But generally, what is the reason to support debian stable?  I understand
the CI thing, - we need a way to test stuff.  For this, I'd say a better
alternative would be to target debian testing (currently trixie), not
debian stable.

Thanks,

/mjt


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-27  7:01 ` Michael Tokarev
@ 2024-10-27  8:00   ` Paolo Bonzini
  2024-10-27  9:38     ` Michael Tokarev
  2024-10-28  9:21   ` Daniel P. Berrangé
  1 sibling, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-27  8:00 UTC (permalink / raw)
  To: Michael Tokarev
  Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

On Sun, Oct 27, 2024 at 8:02 AM Michael Tokarev <mjt@tls.msk.ru> wrote:
>i
> I think this is the wrong direction (ie, backwards).
>
> Sacrificing current code to be compatible with old stuff feels wrong.
> Especially for really old, like rustc in debian bookworm.
>
> bookworm has rustc-web (and a few related packages) which is regular
> rustc version 1.78, just renamed.  It is regular bookworm, not backports.
> It has some packages disabled (compared to regular rust) and is a hack,
> but it exists and can be used for now (dunno if it is sufficient for
> qemu though).

Thanks for pointing it out! It is indeed better, however it does not
support mipsel.

> Also debian has backports mechanism, which also can be used for qemu -
> I can try back-porting regular rust (and llvm) to bookworm.
> I think this is a better way (at least a way forward) than trying to
> move backwards.
>
> But generally, what is the reason to support debian stable?  I understand
> the CI thing, - we need a way to test stuff.  For this, I'd say a better
> alternative would be to target debian testing (currently trixie), not
> debian stable.

Basically: it is not too hard and it can be reverted without much
hassle once we stop supporting Debian 12 in general. Also, the
next-lowest version (in Ubuntu 22.04, which has 1.75.0) would still
have some relatively invasive changes, for example patch 11. We'll
always need some workarounds until all supported distros have rustc
1.77.0.

Paolo



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-27  8:00   ` Paolo Bonzini
@ 2024-10-27  9:38     ` Michael Tokarev
  2024-10-27  9:42       ` Michael Tokarev
  0 siblings, 1 reply; 67+ messages in thread
From: Michael Tokarev @ 2024-10-27  9:38 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

27.10.2024 11:00, Paolo Bonzini wrpte:

[rustc-web]

> Thanks for pointing it out! It is indeed better, however it does not
> support mipsel.

mipsel?  do you mean mips64el?

/mjt


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-27  9:38     ` Michael Tokarev
@ 2024-10-27  9:42       ` Michael Tokarev
  2024-10-27  9:57         ` Michael Tokarev
  2024-10-27 12:39         ` Paolo Bonzini
  0 siblings, 2 replies; 67+ messages in thread
From: Michael Tokarev @ 2024-10-27  9:42 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

27.10.2024 12:38, Michael Tokarev wrote:
> 27.10.2024 11:00, Paolo Bonzini wrpte:
> 
> [rustc-web]
> 
>> Thanks for pointing it out! It is indeed better, however it does not
>> support mipsel.
> 
> mipsel?  do you mean mips64el?

Ah. I see what you mean.
https://buildd.debian.org/status/package.php?p=rustc-web&suite=bookworm

FWIW, mipsel has been removed for the next debian, it isn't supported
anymore in sid or testing (trixie).

/mjt


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-27  9:42       ` Michael Tokarev
@ 2024-10-27  9:57         ` Michael Tokarev
  2024-10-27 12:39         ` Paolo Bonzini
  1 sibling, 0 replies; 67+ messages in thread
From: Michael Tokarev @ 2024-10-27  9:57 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

27.10.2024 12:42, Michael Tokarev пишет:
> 27.10.2024 12:38, Michael Tokarev wrote:
>> 27.10.2024 11:00, Paolo Bonzini wrpte:
>>
>> [rustc-web]
>>
>>> Thanks for pointing it out! It is indeed better, however it does not
>>> support mipsel.
>>
>> mipsel?  do you mean mips64el?

Please note upstream rust does not provide rustup binaries for mipsel.
So there's really no way to bootstrap mipsel rust currently.

And mipsel is gone in debian.

It looks like we should not expect rust to work on mipsel.

/mjt


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-27  9:42       ` Michael Tokarev
  2024-10-27  9:57         ` Michael Tokarev
@ 2024-10-27 12:39         ` Paolo Bonzini
  1 sibling, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-27 12:39 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

Il dom 27 ott 2024, 10:43 Michael Tokarev <mjt@tls.msk.ru> ha scritto:

> 27.10.2024 12:38, Michael Tokarev wrote:
> > 27.10.2024 11:00, Paolo Bonzini wrpte:
> >
> > [rustc-web]
> >
> >> Thanks for pointing it out! It is indeed better, however it does not
> >> support mipsel.
> >
> > mipsel?  do you mean mips64el?
>
> Ah. I see what you mean.
> https://buildd.debian.org/status/package.php?p=rustc-web&suite=bookworm
>
> FWIW, mipsel has been removed for the next debian, it isn't supported
> anymore in sid or testing (trixie).
>

Oh, then we need to deprecate it! But for now we're a bit stuck with it.

Paolo


> /mjt
>
>

[-- Attachment #2: Type: text/html, Size: 1458 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-27  7:01 ` Michael Tokarev
  2024-10-27  8:00   ` Paolo Bonzini
@ 2024-10-28  9:21   ` Daniel P. Berrangé
  2024-10-28 12:26     ` Alex Bennée
  1 sibling, 1 reply; 67+ messages in thread
From: Daniel P. Berrangé @ 2024-10-28  9:21 UTC (permalink / raw)
  To: Michael Tokarev
  Cc: Paolo Bonzini, qemu-devel, manos.pitsidianakis, zhao1.liu,
	junjie.mao

On Sun, Oct 27, 2024 at 10:01:26AM +0300, Michael Tokarev wrote:
> I think this is the wrong direction (ie, backwards).
> 
> Sacrificing current code to be compatible with old stuff feels wrong.
> Especially for really old, like rustc in debian bookworm.
> 
> bookworm has rustc-web (and a few related packages) which is regular
> rustc version 1.78, just renamed.  It is regular bookworm, not backports.
> It has some packages disabled (compared to regular rust) and is a hack,
> but it exists and can be used for now (dunno if it is sufficient for
> qemu though).
> 
> Also debian has backports mechanism, which also can be used for qemu -
> I can try back-porting regular rust (and llvm) to bookworm.
> 
> I think this is a better way (at least a way forward) than trying to
> move backwards.
> 
> But generally, what is the reason to support debian stable?  I understand
> the CI thing, - we need a way to test stuff.  For this, I'd say a better
> alternative would be to target debian testing (currently trixie), not
> debian stable.

The stable distros are what our community of contributors are usually
using, as few people want non-released bleeding edge distros as their
primary development platform.

Custom installing latest upstream pieces is not a user friendly position
to take. Occassionally it is unavoidable, but it is something to be
avoided wherever practical.

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 :|



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-28  9:21   ` Daniel P. Berrangé
@ 2024-10-28 12:26     ` Alex Bennée
  2024-10-28 12:41       ` Paolo Bonzini
  0 siblings, 1 reply; 67+ messages in thread
From: Alex Bennée @ 2024-10-28 12:26 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Michael Tokarev, Paolo Bonzini, qemu-devel, manos.pitsidianakis,
	zhao1.liu, junjie.mao

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Sun, Oct 27, 2024 at 10:01:26AM +0300, Michael Tokarev wrote:
>> I think this is the wrong direction (ie, backwards).
>> 
>> Sacrificing current code to be compatible with old stuff feels wrong.
>> Especially for really old, like rustc in debian bookworm.
>> 
>> bookworm has rustc-web (and a few related packages) which is regular
>> rustc version 1.78, just renamed.  It is regular bookworm, not backports.
>> It has some packages disabled (compared to regular rust) and is a hack,
>> but it exists and can be used for now (dunno if it is sufficient for
>> qemu though).
>> 
>> Also debian has backports mechanism, which also can be used for qemu -
>> I can try back-porting regular rust (and llvm) to bookworm.
>> 
>> I think this is a better way (at least a way forward) than trying to
>> move backwards.
>> 
>> But generally, what is the reason to support debian stable?  I understand
>> the CI thing, - we need a way to test stuff.  For this, I'd say a better
>> alternative would be to target debian testing (currently trixie), not
>> debian stable.
>
> The stable distros are what our community of contributors are usually
> using, as few people want non-released bleeding edge distros as their
> primary development platform.
>
> Custom installing latest upstream pieces is not a user friendly position
> to take. Occassionally it is unavoidable, but it is something to be
> avoided wherever practical.

At least rustup makes this reasonably easy for the rust bits. We do rely
on the excellent Debian backports for getting QEMU quickly into testing
images but I was assuming we would have trixie before --enable-rust
became mandatory so I'm not too worried if bookworm is the outlier for
old versions.

>
> With regards,
> Daniel

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-28 12:26     ` Alex Bennée
@ 2024-10-28 12:41       ` Paolo Bonzini
  0 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-28 12:41 UTC (permalink / raw)
  To: Alex Bennée, Daniel P. Berrangé
  Cc: Michael Tokarev, qemu-devel, manos.pitsidianakis, zhao1.liu,
	junjie.mao

On 10/28/24 13:26, Alex Bennée wrote:
> At least rustup makes this reasonably easy for the rust bits. We do rely
> on the excellent Debian backports for getting QEMU quickly into testing
> images but I was assuming we would have trixie before --enable-rust
> became mandatory so I'm not too worried if bookworm is the outlier for
> old versions.

I agree that we can delay making Rust mandatory only after Bookworm has 
gone away.  However, if we can make --enable-rust default as soon as 
possible, even in 10.0, then it would be easier if the same code (which 
is the Rust one) runs on all supported platforms.

All in all, the point of this series is to show that the workarounds for 
old rustc and bindgen are self-contained and easy to revert later when 
we can.  If we agree about that, it seems worse to me if a couple 
releases single out Bookworm as the only non-Rust supported platform.

Paolo



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (24 preceding siblings ...)
  2024-10-27  7:01 ` Michael Tokarev
@ 2024-10-30 16:52 ` Paolo Bonzini
  2024-10-31 16:41 ` Zhao Liu
  26 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-30 16:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: manos.pitsidianakis, zhao1.liu, junjie.mao, berrange

Ping for missing reviews; particularly patches 11, 13 and 14.

    https://lore.kernel.org/qemu-devel/20241025160209.194307-12-pbonzini@redhat.com/
    https://lore.kernel.org/qemu-devel/20241025160209.194307-14-pbonzini@redhat.com/
    https://lore.kernel.org/qemu-devel/20241025160209.194307-15-pbonzini@redhat.com/

As to patch 19, which is the ugly --generate-cstr workaround for
old bindgen, I've played a bit with refactoring the QOM bindings and
adding a new trait even for types defined by C code.  Such approach
would make the workaround considerably less ugly, since you need
some kind of

pub impl ObjectType for bindings::DeviceState {
    type Class = bindings::DeviceClass;
    const TYPE_NAME: &CStr = bindings::TYPE_DEVICE;
}

anyway; then the only difference introduced by old bindgen would be

 pub impl ObjectType for bindings::DeviceState {
     type Class = bindings::DeviceClass;
-    const TYPE_NAME: &CStr = bindings::TYPE_DEVICE;
+    const TYPE_NAME: &CStr =
+        unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
 }

which is considerably better than having a random TYPE_DEVICE global
somewhere.

Paolo

On Fri, Oct 25, 2024 at 6:02 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Since Manos helpfully posted his vmstate patches, this series is all that
> is needed in order to enable Rust for at least the Debian, Fedora and
> Ubuntu jobs.  I took his patches and isolated them from the procedural
> macro experiment.
>
> There are quite a few changes from the previous posting:
>
> - new patches to bring pl011 mostly up to date with the C code (1-7)
>
> - remove unnecessary .gitattributes file (8)
>
> - apply rustfmt throughout
>
> - add "rust: create a cargo workspace" to ensure a single version of the
>   dependencies is used for all cargo commands (14, based on a suggestion by
>   Junjie)
>
> - add Junjie's syntax checks to the offset_of! macro.  I added a small
>   struct with a From<> implementation, to make compile errors even easier
>   to report (15).
>
> - add final patch to enable rust in the Debian and Ubuntu system build
>   jobs (23)
>
> Note that this requires "meson subprojects update --reset" in order to do
> an incremental build if you have already downloaded the Rust subprojects.
> While I have a solution for that (modeled after scripts/git-submodule.sh),
> I first need to check with the Meson folks whether my script is using only
> stable interfaces.
>
> This series can be found at branch rust-next of my git repository
> (https://gitlab.com/bonzini/qemu.git), which also helps with the
> problems in applying patch 8.  Everything up to commit f6a46d2a4eb
> ("rust: do not use TYPE_CHARDEV unnecessarily", 2024-10-25) will be
> my next pull request, which I will send early next week (to give
> people some more days to complain).
>
> Paolo
>
> Supersedes: <20241022100956.196657-1-pbonzini@redhat.com>
>
> CI: https://gitlab.com/bonzini/qemu/-/pipelines/1512732399
>
>
> Manos Pitsidianakis (6):
>   rust: add definitions for vmstate
>   rust/pl011: add support for migration
>   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
>
> Paolo Bonzini (17):
>   rust/pl011: fix default value for migrate-clock
>   rust: patch bilge-impl to allow compilation with 1.63.0
>   rust: fix cfgs of proc-macro2 for 1.63.0
>   rust: use std::os::raw instead of core::ffi
>   rust: introduce a c_str macro
>   rust: silence unknown warnings for the sake of old compilers
>   rust: synchronize dependencies between subprojects and Cargo.lock
>   rust: create a cargo workspace
>   rust: introduce alternative implementation of offset_of!
>   rust: do not use MaybeUninit::zeroed()
>   rust: clean up detection of the language
>   rust: allow version 1.63.0 of rustc
>   rust: do not use --generate-cstr
>   rust: allow older version of bindgen
>   rust: make rustfmt optional
>   dockerfiles: install bindgen from cargo on Ubuntu 22.04
>   ci: enable rust in the Debian and Ubuntu system build job
>
>  docs/about/build-platforms.rst                |  12 +
>  meson.build                                   | 102 +++--
>  .gitattributes                                |   2 +
>  .gitlab-ci.d/buildtest.yml                    |   6 +-
>  meson_options.txt                             |   2 +
>  rust/{hw/char/pl011 => }/Cargo.lock           |   4 +
>  rust/Cargo.toml                               |   7 +
>  rust/hw/char/pl011/Cargo.toml                 |   3 -
>  rust/hw/char/pl011/src/device.rs              | 158 ++++++--
>  rust/hw/char/pl011/src/device_class.rs        |  71 +++-
>  rust/hw/char/pl011/src/lib.rs                 |   5 +-
>  rust/hw/char/pl011/src/memory_ops.rs          |  14 +-
>  rust/qemu-api-macros/Cargo.lock               |  47 ---
>  rust/qemu-api-macros/Cargo.toml               |   5 +-
>  rust/qemu-api-macros/src/lib.rs               |  81 +++-
>  rust/qemu-api/Cargo.lock                      |   7 -
>  rust/qemu-api/Cargo.toml                      |  10 +-
>  rust/qemu-api/build.rs                        |   9 +
>  rust/qemu-api/meson.build                     |  17 +-
>  rust/qemu-api/src/c_str.rs                    |  53 +++
>  rust/qemu-api/src/definitions.rs              |   2 +-
>  rust/qemu-api/src/device_class.rs             |  43 +--
>  rust/qemu-api/src/lib.rs                      |  19 +-
>  rust/qemu-api/src/offset_of.rs                | 161 ++++++++
>  rust/qemu-api/src/vmstate.rs                  | 358 ++++++++++++++++++
>  rust/qemu-api/src/zeroable.rs                 |  91 ++++-
>  rust/qemu-api/tests/tests.rs                  |  29 +-
>  scripts/meson-buildoptions.sh                 |   4 +
>  subprojects/bilge-impl-0.2-rs.wrap            |   1 +
>  .../packagefiles/bilge-impl-1.63.0.patch      |  45 +++
>  .../packagefiles/proc-macro2-1-rs/meson.build |   4 +-
>  subprojects/packagefiles/syn-2-rs/meson.build |   1 +
>  tests/docker/dockerfiles/ubuntu2204.docker    |   5 +
>  tests/lcitool/mappings.yml                    |   4 +
>  tests/lcitool/refresh                         |  11 +-
>  35 files changed, 1166 insertions(+), 227 deletions(-)
>  rename rust/{hw/char/pl011 => }/Cargo.lock (98%)
>  create mode 100644 rust/Cargo.toml
>  delete mode 100644 rust/qemu-api-macros/Cargo.lock
>  delete mode 100644 rust/qemu-api/Cargo.lock
>  create mode 100644 rust/qemu-api/src/c_str.rs
>  create mode 100644 rust/qemu-api/src/offset_of.rs
>  create mode 100644 rust/qemu-api/src/vmstate.rs
>  create mode 100644 subprojects/packagefiles/bilge-impl-1.63.0.patch
>
> --
> 2.47.0



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 11/23] rust: introduce a c_str macro
  2024-10-25 16:01 ` [PATCH 11/23] rust: introduce a c_str macro Paolo Bonzini
@ 2024-10-31 10:39   ` Zhao Liu
  0 siblings, 0 replies; 67+ messages in thread
From: Zhao Liu @ 2024-10-31 10:39 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, junjie.mao, berrange

> +#[macro_export]
> +/// Given a string constant _without_ embedded or trailing NULs, return
> +/// a CStr.
> +///
> +/// Needed for compatibility with Rust <1.77.
> +macro_rules! c_str {
> +    ($str:expr) => {{
> +        const STRING: &str = concat!($str, "\0");
> +        const BYTES: &[u8] = STRING.as_bytes();
> +
> +        // "for" is not allowed in const context... oh well,
> +        // everybody loves some lisp.  This could be turned into
> +        // a procedural macro if this is a problem; alternatively
> +        // Rust 1.72 makes CStr::from_bytes_with_nul a const function.
> +        const fn f(b: &[u8], i: usize) {
> +            if i == b.len() - 1 {
> +            } else if b[i] == 0 {
> +                panic!("c_str argument contains NUL")
> +            } else {
> +                f(b, i + 1)
> +            }
> +        }
> +        f(BYTES, 0);
> +
> +        // SAFETY: absence of NULs apart from the final byte was checked above
> +        unsafe { std::ffi::CStr::from_bytes_with_nul_unchecked(BYTES) }
> +    }};
> +}
> +

It's not worth introducing the const_for crate for the loop, and I can't
come up with a better solution. The current implementation is simple and
effective, so for now, it's the best option for me.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock
  2024-10-25 16:01 ` [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock Paolo Bonzini
@ 2024-10-31 11:31   ` Zhao Liu
  2024-11-01 10:14   ` Junjie Mao
  1 sibling, 0 replies; 67+ messages in thread
From: Zhao Liu @ 2024-10-31 11:31 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, junjie.mao, berrange

On Fri, Oct 25, 2024 at 06:01:58PM +0200, Paolo Bonzini wrote:
> Date: Fri, 25 Oct 2024 18:01:58 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 13/23] rust: synchronize dependencies between subprojects
>  and Cargo.lock
> X-Mailer: git-send-email 2.47.0
> 
> The next commit will introduce a new build.rs dependency for rust/qemu-api,
> version_check.  Before adding it, ensure that all dependencies are
> synchronized between the Meson- and cargo-based build systems.
> 
> Note that it's not clear whether in the long term we'll use Cargo for
> anything; it seems that the three main uses (clippy, rustfmt, rustdoc)

not sure whether cargo update could help to know if the dependenies can
be updated or not...

> can all be invoked manually---either via glue code in QEMU, or by
> extending Meson to gain the relevant functionality.  However, for
> the time being we're stuck with Cargo so it should at least look at
> the same code as the rest of the build system.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/Cargo.lock   |  3 +++
>  rust/qemu-api-macros/Cargo.lock |  9 ++++---
>  rust/qemu-api/Cargo.lock        | 47 +++++++++++++++++++++++++++++++++
>  rust/qemu-api/Cargo.toml        |  1 +
>  4 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/hw/char/pl011/Cargo.lock b/rust/hw/char/pl011/Cargo.lock
> index b58cebb186e..9f43b33e8b8 100644
> --- a/rust/hw/char/pl011/Cargo.lock
> +++ b/rust/hw/char/pl011/Cargo.lock
> @@ -91,6 +91,9 @@ dependencies = [
>  [[package]]
>  name = "qemu_api"
>  version = "0.1.0"
> +dependencies = [
> + "qemu_api_macros",
> +]
>  
>  [[package]]
>  name = "qemu_api_macros"
> diff --git a/rust/qemu-api-macros/Cargo.lock b/rust/qemu-api-macros/Cargo.lock
> index fdc0fce116c..f989e25829f 100644
> --- a/rust/qemu-api-macros/Cargo.lock
> +++ b/rust/qemu-api-macros/Cargo.lock
> @@ -4,9 +4,9 @@ version = 3
>  
>  [[package]]
>  name = "proc-macro2"
> -version = "1.0.86"
> +version = "1.0.84"
>  source = "registry+https://github.com/rust-lang/crates.io-index"
> -checksum = "5e719e8df665df0d1c8fbfd238015744736151d4445ec0836b8e628aae103b77"
> +checksum = "ec96c6a92621310b51366f1e28d05ef11489516e93be030060e5fc12024a49d6"
>  dependencies = [
>   "unicode-ident",
>  ]
> @@ -18,6 +18,7 @@ dependencies = [
>   "proc-macro2",
>   "quote",
>   "syn",
> + "unicode-ident",
>  ]

With cargo build, it seems this dependency doesn't need to be added here.

I compared the versions and checksums of the wrap files, and I also
built it using cargo build based on this commit. The only change by
Cargo is the one mentioned above; everything else looks good.

With the nit fixed or otherwise,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 14/23] rust: create a cargo workspace
  2024-10-25 16:01 ` [PATCH 14/23] rust: create a cargo workspace Paolo Bonzini
@ 2024-10-31 13:46   ` Zhao Liu
  2024-11-01 10:21   ` Junjie Mao
  1 sibling, 0 replies; 67+ messages in thread
From: Zhao Liu @ 2024-10-31 13:46 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, junjie.mao, berrange

On Fri, Oct 25, 2024 at 06:01:59PM +0200, Paolo Bonzini wrote:
> Date: Fri, 25 Oct 2024 18:01:59 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 14/23] rust: create a cargo workspace
> X-Mailer: git-send-email 2.47.0
> 
> Workspaces allows tracking dependencies for multiple crates at once,
> by having a single Cargo.lock file at the top of the rust/ tree.
> Because QEMU's Cargo.lock files have to be synchronized with the versions
> of crates in subprojects/, using a workspace avoids the need to copy
> over the Cargo.lock file when adding a new device (and thus a new crate)
> under rust/hw/.
> 
> In addition, workspaces let cargo download and build dependencies just
> once.  While right now we have one leaf crate (hw/char/pl011), this
> will not be the case once more devices are added.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/{hw/char/pl011 => }/Cargo.lock |  0
>  rust/Cargo.toml                     |  7 ++++
>  rust/hw/char/pl011/Cargo.toml       |  3 --
>  rust/qemu-api-macros/Cargo.lock     | 48 -------------------------
>  rust/qemu-api-macros/Cargo.toml     |  3 --
>  rust/qemu-api/Cargo.lock            | 54 -----------------------------
>  rust/qemu-api/Cargo.toml            |  3 --
>  7 files changed, 7 insertions(+), 111 deletions(-)
>  rename rust/{hw/char/pl011 => }/Cargo.lock (100%)
>  create mode 100644 rust/Cargo.toml
>  delete mode 100644 rust/qemu-api-macros/Cargo.lock
>  delete mode 100644 rust/qemu-api/Cargo.lock
 
Workspace is a good idea and "cargo build" works fine on my side.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device
  2024-10-25 16:01 ` [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device Paolo Bonzini
@ 2024-10-31 14:58   ` Zhao Liu
  0 siblings, 0 replies; 67+ messages in thread
From: Zhao Liu @ 2024-10-31 14:58 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, junjie.mao, berrange

On Fri, Oct 25, 2024 at 06:01:50PM +0200, Paolo Bonzini wrote:
> Date: Fri, 25 Oct 2024 18:01:50 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device
> X-Mailer: git-send-email 2.47.0
> 
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> 
> Add a device specialization for the Luminary UART device.
> 
> This commit adds a DeviceId enum that utilizes the Index trait to return
> different bytes depending on what device id the UART has (Arm -default-
> or Luminary)
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Link: https://lore.kernel.org/r/20241024-rust-round-2-v1-6-051e7a25b978@linaro.org
> ---
>  rust/hw/char/pl011/src/device.rs | 77 ++++++++++++++++++++++++++++++--
>  rust/hw/char/pl011/src/lib.rs    |  1 +
>  2 files changed, 75 insertions(+), 3 deletions(-)

Great! All tests passed on my side (x86 platform).

Tested-by: Zhao Liu <zhao1.liu@intel.com>



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-25 16:23 ` [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Manos Pitsidianakis
@ 2024-10-31 16:28   ` Paolo Bonzini
  0 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-10-31 16:28 UTC (permalink / raw)
  To: Manos Pitsidianakis; +Cc: qemu-devel, zhao1.liu, junjie.mao, berrange

Manos,

I apologize for extracting these parts of your code without making my
intention fully clear with you first. My intention was to move the CI
integration forward while you focused on the procedural macros,
because right now the partial CI is limiting other potential
contributors.

In order to fix this in the 9.2 release, I would like to include the
extracted patches in a pull request before soft freeze. They implement
the core functionality required by the tests, and they have been
tested already.

During the freeze we can evaluate how to best structure the remaining
changes, including improved QOM bindings and your procedural macro
work.

Thanks,

Paolo


On Fri, Oct 25, 2024 at 6:24 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> Paolo, you picked up my patches without us first talking about it.
> This is not how things should work.
>



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen
  2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
                   ` (25 preceding siblings ...)
  2024-10-30 16:52 ` Paolo Bonzini
@ 2024-10-31 16:41 ` Zhao Liu
  26 siblings, 0 replies; 67+ messages in thread
From: Zhao Liu @ 2024-10-31 16:41 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, junjie.mao, berrange

> This series can be found at branch rust-next of my git repository
> (https://gitlab.com/bonzini/qemu.git), which also helps with the
> problems in applying patch 8.  Everything up to commit f6a46d2a4eb
> ("rust: do not use TYPE_CHARDEV unnecessarily", 2024-10-25) will be
> my next pull request, which I will send early next week (to give
> people some more days to complain).
> 
> Paolo
> 
> Supersedes: <20241022100956.196657-1-pbonzini@redhat.com>
> 
> CI: https://gitlab.com/bonzini/qemu/-/pipelines/1512732399

Compiled with v1.63 and ran "make check" on my x86 platform.

Everything looks good!

Tested-by: Zhao Liu <zhao1.liu@intel.com>



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock
  2024-10-25 16:01 ` [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock Paolo Bonzini
  2024-10-31 11:31   ` Zhao Liu
@ 2024-11-01 10:14   ` Junjie Mao
  2024-11-01 15:30     ` Paolo Bonzini
  1 sibling, 1 reply; 67+ messages in thread
From: Junjie Mao @ 2024-11-01 10:14 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange


Paolo Bonzini <pbonzini@redhat.com> writes:

> The next commit will introduce a new build.rs dependency for rust/qemu-api,
> version_check.  Before adding it, ensure that all dependencies are
> synchronized between the Meson- and cargo-based build systems.
>
> Note that it's not clear whether in the long term we'll use Cargo for
> anything; it seems that the three main uses (clippy, rustfmt, rustdoc)
> can all be invoked manually---either via glue code in QEMU, or by
> extending Meson to gain the relevant functionality.  However, for
> the time being we're stuck with Cargo so it should at least look at
> the same code as the rest of the build system.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/Cargo.lock   |  3 +++
>  rust/qemu-api-macros/Cargo.lock |  9 ++++---
>  rust/qemu-api/Cargo.lock        | 47 +++++++++++++++++++++++++++++++++
>  rust/qemu-api/Cargo.toml        |  1 +
>  4 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/rust/hw/char/pl011/Cargo.lock b/rust/hw/char/pl011/Cargo.lock
> index b58cebb186e..9f43b33e8b8 100644
> --- a/rust/hw/char/pl011/Cargo.lock
> +++ b/rust/hw/char/pl011/Cargo.lock
> @@ -91,6 +91,9 @@ dependencies = [
>  [[package]]
>  name = "qemu_api"
>  version = "0.1.0"
> +dependencies = [
> + "qemu_api_macros",
> +]
>
>  [[package]]
>  name = "qemu_api_macros"
> diff --git a/rust/qemu-api-macros/Cargo.lock b/rust/qemu-api-macros/Cargo.lock
> index fdc0fce116c..f989e25829f 100644
> --- a/rust/qemu-api-macros/Cargo.lock
> +++ b/rust/qemu-api-macros/Cargo.lock
> @@ -4,9 +4,9 @@ version = 3
>
>  [[package]]
>  name = "proc-macro2"
> -version = "1.0.86"
> +version = "1.0.84"

How about specifying also the exact version in Cargo.toml, e.g.:

--- a/rust/qemu-api-macros/Cargo.toml
+++ b/rust/qemu-api-macros/Cargo.toml
@@ -17,9 +17,9 @@ categories = []
 proc-macro = true

 [dependencies]
-proc-macro2 = "1"
-quote = "1"
-syn = { version = "2", features = ["extra-traits"] }
+proc-macro2 = "=1.0.84"
+quote = "=1.0.36"
+syn = { version = "=2.0.66", features = ["extra-traits"] }

 [lints]
 workspace = true

With that the versions of direct dependencies will be unchanged even
after a cargo generate-lockfile.

Unfortunately, versions of nested dependencies, such as either and
unicode-ident, may still have newer patch versions after a lockfile
regeneration. That can be worked around by turning nested dependencies
to direct ones with fixed version constraints, but looks quite ugly.

--
Best Regards
Junjie Mao


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 14/23] rust: create a cargo workspace
  2024-10-25 16:01 ` [PATCH 14/23] rust: create a cargo workspace Paolo Bonzini
  2024-10-31 13:46   ` Zhao Liu
@ 2024-11-01 10:21   ` Junjie Mao
  1 sibling, 0 replies; 67+ messages in thread
From: Junjie Mao @ 2024-11-01 10:21 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange


Paolo Bonzini <pbonzini@redhat.com> writes:

> Workspaces allows tracking dependencies for multiple crates at once,
> by having a single Cargo.lock file at the top of the rust/ tree.
> Because QEMU's Cargo.lock files have to be synchronized with the versions
> of crates in subprojects/, using a workspace avoids the need to copy
> over the Cargo.lock file when adding a new device (and thus a new crate)
> under rust/hw/.
>
> In addition, workspaces let cargo download and build dependencies just
> once.  While right now we have one leaf crate (hw/char/pl011), this
> will not be the case once more devices are added.

Cargo workspace fits our use case very well!

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock
  2024-11-01 10:14   ` Junjie Mao
@ 2024-11-01 15:30     ` Paolo Bonzini
  2024-11-02  2:13       ` Junjie Mao
  0 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-11-01 15:30 UTC (permalink / raw)
  To: Junjie Mao
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu,
	P. Berrange, Daniel

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

Il ven 1 nov 2024, 11:21 Junjie Mao <junjie.mao@hotmail.com> ha scritto:

> How about specifying also the exact version in Cargo.toml, e.g.:
>
>  [dependencies]
> -proc-macro2 = "1"
> -quote = "1"
> -syn = { version = "2", features = ["extra-traits"] }
> +proc-macro2 = "=1.0.84"
> +quote = "=1.0.36"
> +syn = { version = "=2.0.66", features = ["extra-traits"] }
>
>
Unfortunately, versions of nested dependencies, such as either and
> unicode-ident, may still have newer patch versions after a lockfile
> regeneration. That can be worked around by turning nested dependencies
> to direct ones with fixed version constraints, but looks quite ugly.
>

Yeah, that's the reason why I didn't do it... Since we don't have any
security-sensitive dependencies, changes to the lock files are going to be
rare and it's easier to just look at them more closely.

Paolo

--
> Best Regards
> Junjie Mao
>
>

[-- Attachment #2: Type: text/html, Size: 1834 bytes --]

^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock
  2024-11-01 15:30     ` Paolo Bonzini
@ 2024-11-02  2:13       ` Junjie Mao
  0 siblings, 0 replies; 67+ messages in thread
From: Junjie Mao @ 2024-11-02  2:13 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: qemu-devel, Emmanouil Pitsidianakis, Zhao Liu,
	P. Berrange, Daniel


Paolo Bonzini <pbonzini@redhat.com> writes:

> Il ven 1 nov 2024, 11:21 Junjie Mao <junjie.mao@hotmail.com> ha scritto:
>
>  How about specifying also the exact version in Cargo.toml, e.g.:
>
>   [dependencies]
>  -proc-macro2 = "1"
>  -quote = "1"
>  -syn = { version = "2", features = ["extra-traits"] }
>  +proc-macro2 = "=1.0.84"
>  +quote = "=1.0.36"
>  +syn = { version = "=2.0.66", features = ["extra-traits"] }
>
>  Unfortunately, versions of nested dependencies, such as either and
>  unicode-ident, may still have newer patch versions after a lockfile
>  regeneration. That can be worked around by turning nested dependencies
>  to direct ones with fixed version constraints, but looks quite ugly.
>
> Yeah, that's the reason why I didn't do it... Since we don't have any security-sensitive dependencies, changes to the lock files are going to be rare and it's easier to just look at them more closely.

Got your point. Thanks for the clarification!

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>

--
Best Regards
Junjie Mao


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 15/23] rust: introduce alternative implementation of offset_of!
  2024-10-25 16:02 ` [PATCH 15/23] rust: introduce alternative implementation of offset_of! Paolo Bonzini
@ 2024-11-03  9:54   ` Junjie Mao
  2024-11-04 16:02     ` Paolo Bonzini
                       ` (2 more replies)
  0 siblings, 3 replies; 67+ messages in thread
From: Junjie Mao @ 2024-11-03  9:54 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange


Paolo Bonzini <pbonzini@redhat.com> writes:

> From: Junjie Mao <junjie.mao@hotmail.com>
>
> offset_of! was stabilized in Rust 1.77.0.  Use an alternative implemenation
> that was found on the Rust forums, and whose author agreed to license as
> MIT for use in QEMU.
>
> The alternative allows only one level of field access, but apart
> from this can be used just by replacing core::mem::offset_of! with
> qemu_api::offset_of!.
>
> The actual implementation of offset_of! is done in a declarative macro,
> but for simplicity and to avoid introducing an extra level of indentation,
> the trigger is a procedural macro #[derive(offsets)].
>
> The procedural macro is perhaps a bit overengineered, but it helps
> introducing some idioms that will be useful in the future as well.
>
> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Generally looks good to me. Thanks for integrating this!

It seems Rust does not have builtin support for unit tests expecting
compilation failures. There is a crate (named trybuild [1]) for that
purpose but it requires introducing a dozen of new dependencies (see
below). Do you think it worth the effort? If so, I can take a closer
look and cook something for initial review (probably post 9.2).

    trybuild v1.0.101
    ├── glob v0.3.1
    ├── serde v1.0.210
    ├── serde_derive v1.0.210 (proc-macro)
    │   ├── proc-macro2 v1.0.84 (*)
    │   ├── quote v1.0.36 (*)
    │   └── syn v2.0.66 (*)
    ├── serde_json v1.0.132
    │   ├── itoa v1.0.11
    │   ├── memchr v2.7.4
    │   ├── ryu v1.0.18
    │   └── serde v1.0.210
    ├── target-triple v0.1.3
    ├── termcolor v1.4.1
    └── toml v0.8.19
        ├── serde v1.0.210
        ├── serde_spanned v0.6.8
        │   └── serde v1.0.210
        ├── toml_datetime v0.6.8
        │   └── serde v1.0.210
        └── toml_edit v0.22.22
            ├── indexmap v2.6.0
            │   ├── equivalent v1.0.1
            │   └── hashbrown v0.15.0
            ├── serde v1.0.210
            ├── serde_spanned v0.6.8 (*)
            ├── toml_datetime v0.6.8 (*)
            └── winnow v0.6.20

[1] https://docs.rs/trybuild/latest/trybuild/

[snip]
> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
> index a4bc5d01ee8..c2ea22101e4 100644
> --- a/rust/qemu-api-macros/src/lib.rs
> +++ b/rust/qemu-api-macros/src/lib.rs
> @@ -3,8 +3,34 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>
>  use proc_macro::TokenStream;
> -use quote::quote;
> -use syn::{parse_macro_input, DeriveInput};
> +use proc_macro2::Span;
> +use quote::{quote, quote_spanned};
> +use syn::{
> +    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
> +    Fields, Ident, Type, Visibility,
> +};
> +
> +struct CompileError(String, Span);
> +
> +impl From<CompileError> for proc_macro2::TokenStream {
> +    fn from(err: CompileError) -> Self {
> +        let CompileError(msg, span) = err;
> +        quote_spanned! { span => compile_error!(#msg); }

The documentation [2] says "there should be no space before the =>
token" and that is by intention to tell that `span` is "evaluated in the
context of proc macro" while those after the arm "in the generated
code". Should we follow that convention (even though the extra white
space does not impact building)?

[2] https://docs.rs/quote/latest/quote/macro.quote_spanned.html

--
Best Regards
Junjie Mao


^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 15/23] rust: introduce alternative implementation of offset_of!
  2024-11-03  9:54   ` Junjie Mao
@ 2024-11-04 16:02     ` Paolo Bonzini
  2024-11-04 16:03     ` Paolo Bonzini
  2024-11-04 16:03     ` Paolo Bonzini
  2 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-11-04 16:02 UTC (permalink / raw)
  To: Junjie Mao; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange

On 11/3/24 10:54, Junjie Mao wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> offset_of! was stabilized in Rust 1.77.0.  Use an alternative implemenation
>> that was found on the Rust forums, and whose author agreed to license as
>> MIT for use in QEMU.
>>
>> The alternative allows only one level of field access, but apart
>> from this can be used just by replacing core::mem::offset_of! with
>> qemu_api::offset_of!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Generally looks good to me. Thanks for integrating this!
> 
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
> 
>      trybuild v1.0.101
>      ├── glob v0.3.1
>      ├── serde v1.0.210
>      ├── serde_derive v1.0.210 (proc-macro)
>      │   ├── proc-macro2 v1.0.84 (*)
>      │   ├── quote v1.0.36 (*)
>      │   └── syn v2.0.66 (*)
>      ├── serde_json v1.0.132
>      │   ├── itoa v1.0.11
>      │   ├── memchr v2.7.4
>      │   ├── ryu v1.0.18
>      │   └── serde v1.0.210
>      ├── target-triple v0.1.3
>      ├── termcolor v1.4.1
>      └── toml v0.8.19
>          ├── serde v1.0.210
>          ├── serde_spanned v0.6.8
>          │   └── serde v1.0.210
>          ├── toml_datetime v0.6.8
>          │   └── serde v1.0.210
>          └── toml_edit v0.22.22
>              ├── indexmap v2.6.0
>              │   ├── equivalent v1.0.1
>              │   └── hashbrown v0.15.0
>              ├── serde v1.0.210
>              ├── serde_spanned v0.6.8 (*)
>              ├── toml_datetime v0.6.8 (*)
>              └── winnow v0.6.20
> 
> [1] https://docs.rs/trybuild/latest/trybuild/
> 
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>
>>   use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> +    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> +    Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> +    fn from(err: CompileError) -> Self {
>> +        let CompileError(msg, span) = err;
>> +        quote_spanned! { span => compile_error!(#msg); }
> 
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?

I think it'd be a bit too much to add 18 crates (of which 2 for both 
native and non-native).

Assuming you can make it work at all (because it's a lot of crates, 
because there might be dependencies on Cargo, and because the toml crate 
changes MSRV very often), it would increase build time a lot.  Plus, the 
only time I worked with a crate that used trybuild, it tended to break a 
lot due to changes in compiler errors.

We're only adding macros for our own convenience, it's not like we're a 
general purpose crate, so we can live with the limitation.  But thanks 
for getting such a clear dep tree!

Paolo



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 15/23] rust: introduce alternative implementation of offset_of!
  2024-11-03  9:54   ` Junjie Mao
  2024-11-04 16:02     ` Paolo Bonzini
@ 2024-11-04 16:03     ` Paolo Bonzini
  2024-11-04 16:03     ` Paolo Bonzini
  2 siblings, 0 replies; 67+ messages in thread
From: Paolo Bonzini @ 2024-11-04 16:03 UTC (permalink / raw)
  To: Junjie Mao; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange

On 11/3/24 10:54, Junjie Mao wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> offset_of! was stabilized in Rust 1.77.0.  Use an alternative implemenation
>> that was found on the Rust forums, and whose author agreed to license as
>> MIT for use in QEMU.
>>
>> The alternative allows only one level of field access, but apart
>> from this can be used just by replacing core::mem::offset_of! with
>> qemu_api::offset_of!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Generally looks good to me. Thanks for integrating this!
> 
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
> 
>      trybuild v1.0.101
>      ├── glob v0.3.1
>      ├── serde v1.0.210
>      ├── serde_derive v1.0.210 (proc-macro)
>      │   ├── proc-macro2 v1.0.84 (*)
>      │   ├── quote v1.0.36 (*)
>      │   └── syn v2.0.66 (*)
>      ├── serde_json v1.0.132
>      │   ├── itoa v1.0.11
>      │   ├── memchr v2.7.4
>      │   ├── ryu v1.0.18
>      │   └── serde v1.0.210
>      ├── target-triple v0.1.3
>      ├── termcolor v1.4.1
>      └── toml v0.8.19
>          ├── serde v1.0.210
>          ├── serde_spanned v0.6.8
>          │   └── serde v1.0.210
>          ├── toml_datetime v0.6.8
>          │   └── serde v1.0.210
>          └── toml_edit v0.22.22
>              ├── indexmap v2.6.0
>              │   ├── equivalent v1.0.1
>              │   └── hashbrown v0.15.0
>              ├── serde v1.0.210
>              ├── serde_spanned v0.6.8 (*)
>              ├── toml_datetime v0.6.8 (*)
>              └── winnow v0.6.20
> 
> [1] https://docs.rs/trybuild/latest/trybuild/
> 
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>
>>   use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> +    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> +    Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> +    fn from(err: CompileError) -> Self {
>> +        let CompileError(msg, span) = err;
>> +        quote_spanned! { span => compile_error!(#msg); }
> 
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?

Ah, forgot to reply about this.  Personally I think it's clear enough 
with the "=>", but if there's agreement on removing the space I don't 
oppose it.

Paolo



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 15/23] rust: introduce alternative implementation of offset_of!
  2024-11-03  9:54   ` Junjie Mao
  2024-11-04 16:02     ` Paolo Bonzini
  2024-11-04 16:03     ` Paolo Bonzini
@ 2024-11-04 16:03     ` Paolo Bonzini
  2024-11-05  2:07       ` Junjie Mao
  2 siblings, 1 reply; 67+ messages in thread
From: Paolo Bonzini @ 2024-11-04 16:03 UTC (permalink / raw)
  To: Junjie Mao; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange

On 11/3/24 10:54, Junjie Mao wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> From: Junjie Mao <junjie.mao@hotmail.com>
>>
>> offset_of! was stabilized in Rust 1.77.0.  Use an alternative implemenation
>> that was found on the Rust forums, and whose author agreed to license as
>> MIT for use in QEMU.
>>
>> The alternative allows only one level of field access, but apart
>> from this can be used just by replacing core::mem::offset_of! with
>> qemu_api::offset_of!.
>>
>> The actual implementation of offset_of! is done in a declarative macro,
>> but for simplicity and to avoid introducing an extra level of indentation,
>> the trigger is a procedural macro #[derive(offsets)].
>>
>> The procedural macro is perhaps a bit overengineered, but it helps
>> introducing some idioms that will be useful in the future as well.
>>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Generally looks good to me. Thanks for integrating this!
> 
> It seems Rust does not have builtin support for unit tests expecting
> compilation failures. There is a crate (named trybuild [1]) for that
> purpose but it requires introducing a dozen of new dependencies (see
> below). Do you think it worth the effort? If so, I can take a closer
> look and cook something for initial review (probably post 9.2).
> 
>      trybuild v1.0.101
>      ├── glob v0.3.1
>      ├── serde v1.0.210
>      ├── serde_derive v1.0.210 (proc-macro)
>      │   ├── proc-macro2 v1.0.84 (*)
>      │   ├── quote v1.0.36 (*)
>      │   └── syn v2.0.66 (*)
>      ├── serde_json v1.0.132
>      │   ├── itoa v1.0.11
>      │   ├── memchr v2.7.4
>      │   ├── ryu v1.0.18
>      │   └── serde v1.0.210
>      ├── target-triple v0.1.3
>      ├── termcolor v1.4.1
>      └── toml v0.8.19
>          ├── serde v1.0.210
>          ├── serde_spanned v0.6.8
>          │   └── serde v1.0.210
>          ├── toml_datetime v0.6.8
>          │   └── serde v1.0.210
>          └── toml_edit v0.22.22
>              ├── indexmap v2.6.0
>              │   ├── equivalent v1.0.1
>              │   └── hashbrown v0.15.0
>              ├── serde v1.0.210
>              ├── serde_spanned v0.6.8 (*)
>              ├── toml_datetime v0.6.8 (*)
>              └── winnow v0.6.20
> 
> [1] https://docs.rs/trybuild/latest/trybuild/
> 
> [snip]
>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>> index a4bc5d01ee8..c2ea22101e4 100644
>> --- a/rust/qemu-api-macros/src/lib.rs
>> +++ b/rust/qemu-api-macros/src/lib.rs
>> @@ -3,8 +3,34 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>
>>   use proc_macro::TokenStream;
>> -use quote::quote;
>> -use syn::{parse_macro_input, DeriveInput};
>> +use proc_macro2::Span;
>> +use quote::{quote, quote_spanned};
>> +use syn::{
>> +    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>> +    Fields, Ident, Type, Visibility,
>> +};
>> +
>> +struct CompileError(String, Span);
>> +
>> +impl From<CompileError> for proc_macro2::TokenStream {
>> +    fn from(err: CompileError) -> Self {
>> +        let CompileError(msg, span) = err;
>> +        quote_spanned! { span => compile_error!(#msg); }
> 
> The documentation [2] says "there should be no space before the =>
> token" and that is by intention to tell that `span` is "evaluated in the
> context of proc macro" while those after the arm "in the generated
> code". Should we follow that convention (even though the extra white
> space does not impact building)?

Ah, forgot to reply about this.  Personally I think it's clear enough 
with the space around both sides of "=>", but if there's agreement on 
removing the space I don't oppose it.

Paolo



^ permalink raw reply	[flat|nested] 67+ messages in thread

* Re: [PATCH 15/23] rust: introduce alternative implementation of offset_of!
  2024-11-04 16:03     ` Paolo Bonzini
@ 2024-11-05  2:07       ` Junjie Mao
  0 siblings, 0 replies; 67+ messages in thread
From: Junjie Mao @ 2024-11-05  2:07 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, manos.pitsidianakis, zhao1.liu, berrange


Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/3/24 10:54, Junjie Mao wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
>>> index a4bc5d01ee8..c2ea22101e4 100644
>>> --- a/rust/qemu-api-macros/src/lib.rs
>>> +++ b/rust/qemu-api-macros/src/lib.rs
>>> @@ -3,8 +3,34 @@
>>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>>
>>>   use proc_macro::TokenStream;
>>> -use quote::quote;
>>> -use syn::{parse_macro_input, DeriveInput};
>>> +use proc_macro2::Span;
>>> +use quote::{quote, quote_spanned};
>>> +use syn::{
>>> +    parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
>>> +    Fields, Ident, Type, Visibility,
>>> +};
>>> +
>>> +struct CompileError(String, Span);
>>> +
>>> +impl From<CompileError> for proc_macro2::TokenStream {
>>> +    fn from(err: CompileError) -> Self {
>>> +        let CompileError(msg, span) = err;
>>> +        quote_spanned! { span => compile_error!(#msg); }
>> The documentation [2] says "there should be no space before the =>
>> token" and that is by intention to tell that `span` is "evaluated in the
>> context of proc macro" while those after the arm "in the generated
>> code". Should we follow that convention (even though the extra white
>> space does not impact building)?
>
> Ah, forgot to reply about this.  Personally I think it's clear enough with the
> space around both sides of "=>", but if there's agreement on removing the space
> I don't oppose it.
>
> Paolo

I don't have any preference, either. I think we can keep it as is and
make sure future calls to quote_spanned! have consistent style. Thanks.

--
Best Regards
Junjie Mao


^ permalink raw reply	[flat|nested] 67+ messages in thread

end of thread, other threads:[~2024-11-05  2:16 UTC | newest]

Thread overview: 67+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-25 16:01 [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Paolo Bonzini
2024-10-25 16:01 ` [PATCH 01/23] rust: add definitions for vmstate Paolo Bonzini
2024-10-25 16:01 ` [PATCH 02/23] rust/pl011: fix default value for migrate-clock Paolo Bonzini
2024-10-25 16:01 ` [PATCH 03/23] rust/pl011: add support for migration Paolo Bonzini
2024-10-25 16:01 ` [PATCH 04/23] rust/pl011: move CLK_NAME static to function scope Paolo Bonzini
2024-10-25 16:01 ` [PATCH 05/23] rust/pl011: add TYPE_PL011_LUMINARY device Paolo Bonzini
2024-10-31 14:58   ` Zhao Liu
2024-10-25 16:01 ` [PATCH 06/23] rust/pl011: remove commented out C code Paolo Bonzini
2024-10-25 16:01 ` [PATCH 07/23] rust/pl011: Use correct masks for IBRD and FBRD Paolo Bonzini
2024-10-25 16:01 ` [PATCH 08/23] rust: patch bilge-impl to allow compilation with 1.63.0 Paolo Bonzini
2024-10-25 16:01 ` [PATCH 09/23] rust: fix cfgs of proc-macro2 for 1.63.0 Paolo Bonzini
2024-10-25 16:01 ` [PATCH 10/23] rust: use std::os::raw instead of core::ffi Paolo Bonzini
2024-10-25 16:01 ` [PATCH 11/23] rust: introduce a c_str macro Paolo Bonzini
2024-10-31 10:39   ` Zhao Liu
2024-10-25 16:01 ` [PATCH 12/23] rust: silence unknown warnings for the sake of old compilers Paolo Bonzini
2024-10-25 16:01 ` [PATCH 13/23] rust: synchronize dependencies between subprojects and Cargo.lock Paolo Bonzini
2024-10-31 11:31   ` Zhao Liu
2024-11-01 10:14   ` Junjie Mao
2024-11-01 15:30     ` Paolo Bonzini
2024-11-02  2:13       ` Junjie Mao
2024-10-25 16:01 ` [PATCH 14/23] rust: create a cargo workspace Paolo Bonzini
2024-10-31 13:46   ` Zhao Liu
2024-11-01 10:21   ` Junjie Mao
2024-10-25 16:02 ` [PATCH 15/23] rust: introduce alternative implementation of offset_of! Paolo Bonzini
2024-11-03  9:54   ` Junjie Mao
2024-11-04 16:02     ` Paolo Bonzini
2024-11-04 16:03     ` Paolo Bonzini
2024-11-04 16:03     ` Paolo Bonzini
2024-11-05  2:07       ` Junjie Mao
2024-10-25 16:02 ` [PATCH 16/23] rust: do not use MaybeUninit::zeroed() Paolo Bonzini
2024-10-25 16:02 ` [PATCH 17/23] rust: clean up detection of the language Paolo Bonzini
2024-10-25 16:02 ` [PATCH 18/23] rust: allow version 1.63.0 of rustc Paolo Bonzini
2024-10-25 16:02 ` [PATCH 19/23] rust: do not use --generate-cstr Paolo Bonzini
2024-10-25 20:03   ` Michael Tokarev
2024-10-25 20:06     ` Pierrick Bouvier
2024-10-25 20:10       ` Michael Tokarev
2024-10-25 20:12         ` Pierrick Bouvier
2024-10-25 20:11     ` Paolo Bonzini
2024-10-25 16:02 ` [PATCH 20/23] rust: allow older version of bindgen Paolo Bonzini
2024-10-25 16:02 ` [PATCH 21/23] rust: make rustfmt optional Paolo Bonzini
2024-10-25 16:02 ` [PATCH 22/23] dockerfiles: install bindgen from cargo on Ubuntu 22.04 Paolo Bonzini
2024-10-25 18:51   ` Pierrick Bouvier
2024-10-25 19:35     ` Paolo Bonzini
2024-10-25 19:47       ` Pierrick Bouvier
2024-10-25 20:08         ` Paolo Bonzini
2024-10-25 20:14           ` Pierrick Bouvier
2024-10-25 20:21             ` Paolo Bonzini
2024-10-25 20:08   ` Pierrick Bouvier
2024-10-25 16:02 ` [PATCH 23/23] ci: enable rust in the Debian and Ubuntu system build job Paolo Bonzini
2024-10-25 18:55   ` Pierrick Bouvier
2024-10-25 18:58     ` Pierrick Bouvier
2024-10-25 19:27     ` Paolo Bonzini
2024-10-25 19:33       ` Pierrick Bouvier
2024-10-25 20:08   ` Pierrick Bouvier
2024-10-25 16:23 ` [PATCH v3 00/23] rust: fix CI + allow older versions of rustc and bindgen Manos Pitsidianakis
2024-10-31 16:28   ` Paolo Bonzini
2024-10-27  7:01 ` Michael Tokarev
2024-10-27  8:00   ` Paolo Bonzini
2024-10-27  9:38     ` Michael Tokarev
2024-10-27  9:42       ` Michael Tokarev
2024-10-27  9:57         ` Michael Tokarev
2024-10-27 12:39         ` Paolo Bonzini
2024-10-28  9:21   ` Daniel P. Berrangé
2024-10-28 12:26     ` Alex Bennée
2024-10-28 12:41       ` Paolo Bonzini
2024-10-30 16:52 ` Paolo Bonzini
2024-10-31 16:41 ` Zhao Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).