[PATCH v2 00/13] rust: miscellaneous cleanups + QOM integration tests

[PATCH v2 00/13] rust: miscellaneous cleanups + QOM integration tests

[Paolo Bonzini]

This series integrates some of the observations from the MSRV patches at
https://lore.kernel.org/qemu-devel/20241015131735.518771-1-pbonzini@redhat.com/.

The main changes here are two: first, build an integration test that
actually tries to create a QOM object that is defined by Rust code;
second, make the properties array immutable so that declare_properties!
is enforced to use only const-friendly constructs.  These are patches
6-11; the others consist of small cleanups.

Hidden in here is actually a C patch (#10) which makes the
bindgen-generated prototypes use "*const" instead of "*mut".

Tested with Rust nightly and (together with more patches from the
RFC), with Rust 1.63.0.

Unlike the MSRV patches, this should be ready for inclusion; the
changes should be mostly uncontroversial.

Paolo

Based-on: <20241021163449.136804-1-pbonzini@redhat.com>


v1->v2:
- add comment justifying -D unsafe_op_in_unsafe_fn
- use module_init! macro from #[derive(Object)]
- use test() instead of rust.test() for integration test
- pass rustfmt and clippy


Paolo Bonzini (13):
  meson: import rust module into a global variable
  meson: remove repeated search for rust_root_crate.sh
  meson: pass rustc_args when building all crates
  rust: do not use --no-size_t-is-usize
  rust: remove uses of #[no_mangle]
  rust: modernize link_section usage for ELF platforms
  rust: build integration test for the qemu_api crate
  rust: cleanup module_init!, use it from #[derive(Object)]
  rust: clean up define_property macro
  qdev: make properties array "const"
  rust: make properties array immutable
  rust: provide safe wrapper for MaybeUninit::zeroed()
  rust: do not use TYPE_CHARDEV unnecessarily

 meson.build                            | 33 +++++++---
 include/hw/qdev-core.h                 |  4 +-
 include/hw/qdev-properties.h           |  4 +-
 hw/core/qdev-properties.c              | 26 ++++----
 system/qdev-monitor.c                  |  2 +-
 rust/hw/char/pl011/src/device.rs       |  7 +--
 rust/hw/char/pl011/src/device_class.rs | 10 ++--
 rust/hw/char/pl011/src/memory_ops.rs   | 13 ++--
 rust/qemu-api-macros/meson.build       |  2 +-
 rust/qemu-api-macros/src/lib.rs        | 30 ++--------
 rust/qemu-api/meson.build              | 29 +++++++--
 rust/qemu-api/src/definitions.rs       | 67 ++++++++++-----------
 rust/qemu-api/src/device_class.rs      | 83 +++++++-------------------
 rust/qemu-api/src/lib.rs               |  4 +-
 rust/qemu-api/src/tests.rs             | 49 ---------------
 rust/qemu-api/src/zeroable.rs          | 23 +++++++
 rust/qemu-api/tests/tests.rs           | 78 ++++++++++++++++++++++++
 17 files changed, 240 insertions(+), 224 deletions(-)
 delete mode 100644 rust/qemu-api/src/tests.rs
 create mode 100644 rust/qemu-api/src/zeroable.rs
 create mode 100644 rust/qemu-api/tests/tests.rs

-- 
2.46.2

[PATCH v2 01/13] meson: import rust module into a global variable

[Paolo Bonzini]

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                      | 1 +
 rust/qemu-api-macros/meson.build | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index d26690ce204..ffd78b6cbb3 100644
--- a/meson.build
+++ b/meson.build
@@ -15,6 +15,7 @@ meson.add_postconf_script(find_program('scripts/symlink-install-tree.py'))
 
 not_found = dependency('', required: false)
 keyval = import('keyval')
+rust = import('rust')
 ss = import('sourceset')
 fs = import('fs')
 
diff --git a/rust/qemu-api-macros/meson.build b/rust/qemu-api-macros/meson.build
index 517b9a4d2d5..24325dea5c2 100644
--- a/rust/qemu-api-macros/meson.build
+++ b/rust/qemu-api-macros/meson.build
@@ -2,7 +2,7 @@ quote_dep = dependency('quote-1-rs', native: true)
 syn_dep = dependency('syn-2-rs', native: true)
 proc_macro2_dep = dependency('proc-macro2-1-rs', native: true)
 
-_qemu_api_macros_rs = import('rust').proc_macro(
+_qemu_api_macros_rs = rust.proc_macro(
   'qemu_api_macros',
   files('src/lib.rs'),
   override_options: ['rust_std=2021', 'build.rust_std=2021'],
-- 
2.46.2

Re: [PATCH v2 01/13] meson: import rust module into a global variable

[Manos Pitsidianakis]

On Mon, 21 Oct 2024 19:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> meson.build                      | 1 +
> rust/qemu-api-macros/meson.build | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/meson.build b/meson.build
>index d26690ce204..ffd78b6cbb3 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -15,6 +15,7 @@ meson.add_postconf_script(find_program('scripts/symlink-install-tree.py'))
> 
> not_found = dependency('', required: false)
> keyval = import('keyval')
>+rust = import('rust')
> ss = import('sourceset')
> fs = import('fs')
> 
>diff --git a/rust/qemu-api-macros/meson.build b/rust/qemu-api-macros/meson.build
>index 517b9a4d2d5..24325dea5c2 100644
>--- a/rust/qemu-api-macros/meson.build
>+++ b/rust/qemu-api-macros/meson.build
>@@ -2,7 +2,7 @@ quote_dep = dependency('quote-1-rs', native: true)
> syn_dep = dependency('syn-2-rs', native: true)
> proc_macro2_dep = dependency('proc-macro2-1-rs', native: true)
> 
>-_qemu_api_macros_rs = import('rust').proc_macro(
>+_qemu_api_macros_rs = rust.proc_macro(
>   'qemu_api_macros',
>   files('src/lib.rs'),
>   override_options: ['rust_std=2021', 'build.rust_std=2021'],
>-- 
>2.46.2
>

You could also change the bindgen target line:

  bindings_rs = import('rust').bindgen(

Either way:

Tested-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

[PATCH v2 02/13] meson: remove repeated search for rust_root_crate.sh

[Paolo Bonzini]

Avoid repeated lines of the form

Program scripts/rust/rust_root_crate.sh found: YES (/home/pbonzini/work/upstream/qemu/scripts/rust/rust_root_crate.sh)

in the meson logs.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index ffd78b6cbb3..f85bc22fbdd 100644
--- a/meson.build
+++ b/meson.build
@@ -3977,6 +3977,7 @@ endif
 
 
 feature_to_c = find_program('scripts/feature_to_c.py')
+rust_root_crate = find_program('scripts/rust/rust_root_crate.sh')
 
 if host_os == 'darwin'
   entitlement = find_program('scripts/entitlement.sh')
@@ -4078,7 +4079,7 @@ foreach target : target_dirs
     if crates.length() > 0
       rlib_rs = custom_target('rust_' + target.underscorify() + '.rs',
                               output: 'rust_' + target.underscorify() + '.rs',
-                              command: [find_program('scripts/rust/rust_root_crate.sh')] + crates,
+                              command: [rust_root_crate, crates],
                               capture: true,
                               build_by_default: true,
                               build_always_stale: true)
-- 
2.46.2

[PATCH v2 03/13] meson: pass rustc_args when building all crates

[Paolo Bonzini]

rustc_args is needed to smooth the difference in warnings between the various
versions of rustc.  Always include those arguments.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                       | 20 +++++++++++++-------
 rust/qemu-api/meson.build         |  2 +-
 rust/qemu-api/src/device_class.rs | 10 ++++++----
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/meson.build b/meson.build
index f85bc22fbdd..63c07a5b660 100644
--- a/meson.build
+++ b/meson.build
@@ -3317,6 +3317,19 @@ endif
 
 genh += configure_file(output: 'config-host.h', configuration: config_host_data)
 
+if have_rust and have_system
+  rustc_args = run_command(
+    find_program('scripts/rust/rustc_args.py'),
+    '--config-headers', meson.project_build_root() / 'config-host.h',
+    capture : true,
+    check: true).stdout().strip().split()
+
+  # Prohibit code that is forbidden in Rust 2024
+  rustc_args += ['-D', 'unsafe_op_in_unsafe_fn']
+  add_project_arguments(rustc_args, native: false, language: 'rust')
+  add_project_arguments(rustc_args, native: true, language: 'rust')
+endif
+
 hxtool = find_program('scripts/hxtool')
 shaderinclude = find_program('scripts/shaderinclude.py')
 qapi_gen = find_program('scripts/qapi-gen.py')
@@ -3909,12 +3922,6 @@ common_all = static_library('common',
                             dependencies: common_ss.all_dependencies())
 
 if have_rust and have_system
-  rustc_args = run_command(
-    find_program('scripts/rust/rustc_args.py'),
-    '--config-headers', meson.project_build_root() / 'config-host.h',
-    capture : true,
-    check: true).stdout().strip().split()
-  rustc_args += ['-D', 'unsafe_op_in_unsafe_fn']
   bindgen_args = [
     '--disable-header-comment',
     '--raw-line', '// @generated',
@@ -4087,7 +4094,6 @@ foreach target : target_dirs
                             rlib_rs,
                             dependencies: target_rust.dependencies(),
                             override_options: ['rust_std=2021', 'build.rust_std=2021'],
-                            rust_args: rustc_args,
                             rust_abi: 'c')
       arch_deps += declare_dependency(link_whole: [rlib])
     endif
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index c72d34b607d..42ea815fa5a 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -10,7 +10,7 @@ _qemu_api_rs = static_library(
   ),
   override_options: ['rust_std=2021', 'build.rust_std=2021'],
   rust_abi: 'rust',
-  rust_args: rustc_args + [
+  rust_args: [
     '--cfg', 'MESON',
     # '--cfg', 'feature="allocator"',
   ],
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 1ea95beb78d..b6b68cf9ce2 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -16,10 +16,12 @@ macro_rules! device_class_init {
         ) {
             let mut dc =
                 ::core::ptr::NonNull::new(klass.cast::<$crate::bindings::DeviceClass>()).unwrap();
-            dc.as_mut().realize = $realize_fn;
-            dc.as_mut().vmsd = &$vmsd;
-            $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
-            $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
+            unsafe {
+                dc.as_mut().realize = $realize_fn;
+                dc.as_mut().vmsd = &$vmsd;
+                $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
+                $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
+            }
         }
     };
 }
-- 
2.46.2

Re: [PATCH v2 03/13] meson: pass rustc_args when building all crates

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> rustc_args is needed to smooth the difference in warnings between the various
> versions of rustc.  Always include those arguments.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

--
Best Regards
Junjie Mao

Re: [PATCH v2 03/13] meson: pass rustc_args when building all crates

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:28PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:28 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 03/13] meson: pass rustc_args when building all crates
> X-Mailer: git-send-email 2.46.2
> 
> rustc_args is needed to smooth the difference in warnings between the various
> versions of rustc.  Always include those arguments.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                       | 20 +++++++++++++-------
>  rust/qemu-api/meson.build         |  2 +-
>  rust/qemu-api/src/device_class.rs | 10 ++++++----
>  3 files changed, 20 insertions(+), 12 deletions(-)

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

[PATCH v2 04/13] rust: do not use --no-size_t-is-usize

[Paolo Bonzini]

This not necessary and makes it harder to write code that
is portable between 32- and 64-bit systems: it adds extra casts even
though size_of, align_of or offset_of already return the right type.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                      | 1 -
 rust/qemu-api/src/definitions.rs | 6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index 63c07a5b660..6739165908e 100644
--- a/meson.build
+++ b/meson.build
@@ -3934,7 +3934,6 @@ if have_rust and have_system
     '--no-doc-comments',
     '--use-core',
     '--with-derive-default',
-    '--no-size_t-is-usize',
     '--no-layout-tests',
     '--no-prepend-enum-name',
     '--allowlist-file', meson.project_source_root() + '/include/.*',
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index 60bd3f8aaa6..0b681c593f2 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -81,13 +81,13 @@ macro_rules! type_info {
             } else {
                 ::core::ptr::null_mut()
             },
-            instance_size: ::core::mem::size_of::<$t>() as $crate::bindings::size_t,
-            instance_align: ::core::mem::align_of::<$t>() as $crate::bindings::size_t,
+            instance_size: ::core::mem::size_of::<$t>(),
+            instance_align: ::core::mem::align_of::<$t>(),
             instance_init: <$t as $crate::definitions::ObjectImpl>::INSTANCE_INIT,
             instance_post_init: <$t as $crate::definitions::ObjectImpl>::INSTANCE_POST_INIT,
             instance_finalize: <$t as $crate::definitions::ObjectImpl>::INSTANCE_FINALIZE,
             abstract_: <$t as $crate::definitions::ObjectImpl>::ABSTRACT,
-            class_size:  ::core::mem::size_of::<<$t as $crate::definitions::ObjectImpl>::Class>() as $crate::bindings::size_t,
+            class_size:  ::core::mem::size_of::<<$t as $crate::definitions::ObjectImpl>::Class>(),
             class_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::Class>::CLASS_INIT,
             class_base_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::Class>::CLASS_BASE_INIT,
             class_data: ::core::ptr::null_mut(),
-- 
2.46.2

Re: [PATCH v2 04/13] rust: do not use --no-size_t-is-usize

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This not necessary and makes it harder to write code that

This *is* not ...

> is portable between 32- and 64-bit systems: it adds extra casts even
> though size_of, align_of or offset_of already return the right type.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

--
Best Regards
Junjie Mao

Re: [PATCH v2 04/13] rust: do not use --no-size_t-is-usize

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:29PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:29 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 04/13] rust: do not use --no-size_t-is-usize
> X-Mailer: git-send-email 2.46.2
> 
> This not necessary and makes it harder to write code that
> is portable between 32- and 64-bit systems: it adds extra casts even
> though size_of, align_of or offset_of already return the right type.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                      | 1 -
>  rust/qemu-api/src/definitions.rs | 6 +++---
>  2 files changed, 3 insertions(+), 4 deletions(-)

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

[PATCH v2 05/13] rust: remove uses of #[no_mangle]

[Paolo Bonzini]

Mangled symbols do not cause any issue; disabling mangling is only useful if
C headers reference the Rust function, which is not the case here.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs       | 5 -----
 rust/hw/char/pl011/src/device_class.rs | 2 --
 rust/hw/char/pl011/src/memory_ops.rs   | 2 --
 rust/qemu-api/src/definitions.rs       | 1 -
 rust/qemu-api/src/device_class.rs      | 2 --
 5 files changed, 12 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index c7193b41bee..2b43f5e0939 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -514,7 +514,6 @@ pub fn update(&self) {
 /// 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.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_can_receive(opaque: *mut c_void) -> c_int {
     unsafe {
         debug_assert!(!opaque.is_null());
@@ -530,7 +529,6 @@ pub fn update(&self) {
 /// readable/writeable from one thread at any time.
 ///
 /// The buffer and size arguments must also be valid.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_receive(
     opaque: *mut core::ffi::c_void,
     buf: *const u8,
@@ -554,7 +552,6 @@ pub fn update(&self) {
 /// 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.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_event(opaque: *mut core::ffi::c_void, event: QEMUChrEvent) {
     unsafe {
         debug_assert!(!opaque.is_null());
@@ -566,7 +563,6 @@ pub fn update(&self) {
 /// # Safety
 ///
 /// We expect the FFI user of this function to pass a valid pointer for `chr`.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_create(
     addr: u64,
     irq: qemu_irq,
@@ -589,7 +585,6 @@ pub fn update(&self) {
 /// 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.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_init(obj: *mut Object) {
     unsafe {
         debug_assert!(!obj.is_null());
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index b7ab31af02d..2ad80451e87 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -46,7 +46,6 @@
 /// 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.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_realize(dev: *mut DeviceState, _errp: *mut *mut Error) {
     unsafe {
         assert!(!dev.is_null());
@@ -60,7 +59,6 @@
 /// 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.
-#[no_mangle]
 pub unsafe extern "C" fn pl011_reset(dev: *mut DeviceState) {
     unsafe {
         assert!(!dev.is_null());
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
index 8d066ebf6d0..5a5320e66c3 100644
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ b/rust/hw/char/pl011/src/memory_ops.rs
@@ -22,7 +22,6 @@
     },
 };
 
-#[no_mangle]
 unsafe extern "C" fn pl011_read(
     opaque: *mut core::ffi::c_void,
     addr: hwaddr,
@@ -44,7 +43,6 @@
     }
 }
 
-#[no_mangle]
 unsafe extern "C" fn pl011_write(
     opaque: *mut core::ffi::c_void,
     addr: hwaddr,
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index 0b681c593f2..49ac59af123 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -53,7 +53,6 @@ extern "C" fn __load() {
         #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
         pub static LOAD_MODULE: extern "C" fn() = {
             extern "C" fn __load() {
-                #[no_mangle]
                 unsafe extern "C" fn $func() {
                     $body
                 }
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index b6b68cf9ce2..2219b9f73d0 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -9,7 +9,6 @@
 #[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$(,)*) => {
-        #[no_mangle]
         pub unsafe extern "C" fn $func(
             klass: *mut $crate::bindings::ObjectClass,
             _: *mut ::core::ffi::c_void,
@@ -103,7 +102,6 @@ const fn _calc_prop_len() -> usize {
             ]
         }
 
-        #[no_mangle]
         pub static mut $ident: $crate::device_class::Properties<PROP_LEN> = $crate::device_class::Properties(::std::sync::OnceLock::new(), _make_properties);
     };
 }
-- 
2.46.2

Re: [PATCH v2 05/13] rust: remove uses of #[no_mangle]

[Paolo Bonzini]

On 10/21/24 18:35, Paolo Bonzini wrote:
> @@ -566,7 +563,6 @@ pub fn update(&self) {
>   /// # Safety
>   ///
>   /// We expect the FFI user of this function to pass a valid pointer for `chr`.
> -#[no_mangle]
>   pub unsafe extern "C" fn pl011_create(
>       addr: u64,
>       irq: qemu_irq,
This _needs_ to be no_mangle actually, because it is called from C.

Paolo

Re: [PATCH v2 05/13] rust: remove uses of #[no_mangle]

[Zhao Liu]

On Wed, Oct 23, 2024 at 12:48:33PM +0200, Paolo Bonzini wrote:
> Date: Wed, 23 Oct 2024 12:48:33 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v2 05/13] rust: remove uses of #[no_mangle]
> 
> On 10/21/24 18:35, Paolo Bonzini wrote:
> > @@ -566,7 +563,6 @@ pub fn update(&self) {
> >   /// # Safety
> >   ///
> >   /// We expect the FFI user of this function to pass a valid pointer for `chr`.
> > -#[no_mangle]
> >   pub unsafe extern "C" fn pl011_create(
> >       addr: u64,
> >       irq: qemu_irq,
> This _needs_ to be no_mangle actually, because it is called from C.
>  

I'm a bit uncertain whether the "pub" keyword here should be removed.

I understand that pub and private are based on modules and do not apply
to FFI, but I haven't found any related documentation to confirm this.

However, technically, removing the pub here does not seem to cause any
issues during compilation and runtime.

Additionally, the pub keywords in the other functions involved in this
patch (except for the one in device_class_init) can also be removed, as
they are either only used as C callbacks or are indeed only used within
the current crate after macro expansion.

-Zhao

Re: [PATCH v2 05/13] rust: remove uses of #[no_mangle]

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:30PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:30 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 05/13] rust: remove uses of #[no_mangle]
> X-Mailer: git-send-email 2.46.2
> 
> Mangled symbols do not cause any issue; disabling mangling is only useful if
> C headers reference the Rust function, which is not the case here.
> 
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs       | 5 -----
>  rust/hw/char/pl011/src/device_class.rs | 2 --
>  rust/hw/char/pl011/src/memory_ops.rs   | 2 --
>  rust/qemu-api/src/definitions.rs       | 1 -
>  rust/qemu-api/src/device_class.rs      | 2 --
>  5 files changed, 12 deletions(-)
> 

With pl011_create() fixed,

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

[PATCH v2 06/13] rust: modernize link_section usage for ELF platforms

[Paolo Bonzini]

Some newer ABI implementations do not provide .ctors; and while
some linkers rewrite .ctors into .init_array, not all of them do.
Use the newer .init_array ABI, which works more reliably, and
apply it to all non-Apple, non-Windows platforms.

This is similar to how the ctor crate operates; without this change,
"#[derive(Object)]" does not work on Fedora 41.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api-macros/src/lib.rs  |  7 +++++--
 rust/qemu-api/src/definitions.rs | 14 ++++++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 59aba592d9a..70e3f920460 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -16,8 +16,11 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
     let expanded = quote! {
         #[allow(non_upper_case_globals)]
         #[used]
-        #[cfg_attr(target_os = "linux", link_section = ".ctors")]
-        #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
+        #[cfg_attr(
+            not(any(target_vendor = "apple", target_os = "windows")),
+            link_section = ".init_array"
+        )]
+        #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
         #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
         pub static #module_static: extern "C" fn() = {
             extern "C" fn __register() {
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index 49ac59af123..3323a665d92 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -31,8 +31,11 @@ pub trait Class {
 macro_rules! module_init {
     ($func:expr, $type:expr) => {
         #[used]
-        #[cfg_attr(target_os = "linux", link_section = ".ctors")]
-        #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
+        #[cfg_attr(
+            not(any(target_vendor = "apple", target_os = "windows")),
+            link_section = ".init_array"
+        )]
+        #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
         #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
         pub static LOAD_MODULE: extern "C" fn() = {
             extern "C" fn __load() {
@@ -48,8 +51,11 @@ extern "C" fn __load() {
         // NOTE: To have custom identifiers for the ctor func we need to either supply
         // them directly as a macro argument or create them with a proc macro.
         #[used]
-        #[cfg_attr(target_os = "linux", link_section = ".ctors")]
-        #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
+        #[cfg_attr(
+            not(any(target_vendor = "apple", target_os = "windows")),
+            link_section = ".init_array"
+        )]
+        #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
         #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
         pub static LOAD_MODULE: extern "C" fn() = {
             extern "C" fn __load() {
-- 
2.46.2

Re: [PATCH v2 06/13] rust: modernize link_section usage for ELF platforms

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:31PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:31 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 06/13] rust: modernize link_section usage for ELF
>  platforms
> X-Mailer: git-send-email 2.46.2
> 
> Some newer ABI implementations do not provide .ctors; and while
> some linkers rewrite .ctors into .init_array, not all of them do.
> Use the newer .init_array ABI, which works more reliably, and
> apply it to all non-Apple, non-Windows platforms.
> 
> This is similar to how the ctor crate operates; without this change,
> "#[derive(Object)]" does not work on Fedora 41.
> 
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api-macros/src/lib.rs  |  7 +++++--
>  rust/qemu-api/src/definitions.rs | 14 ++++++++++----
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
> index 59aba592d9a..70e3f920460 100644
> --- a/rust/qemu-api-macros/src/lib.rs
> +++ b/rust/qemu-api-macros/src/lib.rs
> @@ -16,8 +16,11 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
>      let expanded = quote! {
>          #[allow(non_upper_case_globals)]
>          #[used]
> -        #[cfg_attr(target_os = "linux", link_section = ".ctors")]
> -        #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
> +        #[cfg_attr(
> +            not(any(target_vendor = "apple", target_os = "windows")),

EMM, "apple" vendor contains macOS, iOS and other variations of iOS.
Do we need to consider other OSs besides macOS for now?

And it seems that the Rust people don't like "target_vendor = 'apple'".
(https://github.com/rust-lang/rust/issues/100343). :-)


Otherwise,

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

Re: [PATCH v2 06/13] rust: modernize link_section usage for ELF platforms

[Paolo Bonzini]

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

Il mer 23 ott 2024, 17:15 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> >      let expanded = quote! {
> >          #[allow(non_upper_case_globals)]
> >          #[used]
> > -        #[cfg_attr(target_os = "linux", link_section = ".ctors")]
> > -        #[cfg_attr(target_os = "macos", link_section =
> "__DATA,__mod_init_func")]
> > +        #[cfg_attr(
> > +            not(any(target_vendor = "apple", target_os = "windows")),
>
> EMM, "apple" vendor contains macOS, iOS and other variations of iOS.
> Do we need to consider other OSs besides macOS for now?
>

Yes, UTM runs on iOS.

And it seems that the Rust people don't like "target_vendor = 'apple'".
> (https://github.com/rust-lang/rust/issues/100343). :-)
>

That would be an edition break, I think, so we can worry about it in 2028.
:)

Paolo


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

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

[PATCH v2 07/13] rust: build integration test for the qemu_api crate

[Paolo Bonzini]

Adjust the integration test to compile with a subset of QEMU object
files, and make it actually create an object of the class it defines.

Follow the Rust filesystem conventions, where tests go in tests/ if
they use the library in the same way any other code would.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                  | 10 ++++-
 rust/qemu-api/meson.build    | 26 ++++++++++--
 rust/qemu-api/src/lib.rs     |  3 --
 rust/qemu-api/src/tests.rs   | 49 ----------------------
 rust/qemu-api/tests/tests.rs | 78 ++++++++++++++++++++++++++++++++++++
 5 files changed, 110 insertions(+), 56 deletions(-)
 delete mode 100644 rust/qemu-api/src/tests.rs
 create mode 100644 rust/qemu-api/tests/tests.rs

diff --git a/meson.build b/meson.build
index 6739165908e..3c71d129494 100644
--- a/meson.build
+++ b/meson.build
@@ -3326,7 +3326,15 @@ if have_rust and have_system
 
   # Prohibit code that is forbidden in Rust 2024
   rustc_args += ['-D', 'unsafe_op_in_unsafe_fn']
-  add_project_arguments(rustc_args, native: false, language: 'rust')
+
+  # 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
+  # passing -nodefaultlibs to the linker "was more ideological to
+  # start with than anything".
+  add_project_arguments(rustc_args + ['-C', 'default-linker-libraries'],
+      native: false, language: 'rust')
+
   add_project_arguments(rustc_args, native: true, language: 'rust')
 endif
 
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 42ea815fa5a..1fc36078027 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -14,11 +14,31 @@ _qemu_api_rs = static_library(
     '--cfg', 'MESON',
     # '--cfg', 'feature="allocator"',
   ],
-  dependencies: [
-    qemu_api_macros,
-  ],
 )
 
 qemu_api = declare_dependency(
   link_with: _qemu_api_rs,
+  dependencies: qemu_api_macros,
 )
+
+# Rust executables do not support objects, so add an intermediate step.
+rust_qemu_api_objs = static_library(
+    'rust_qemu_api_objs',
+    objects: [libqom.extract_all_objects(recursive: false),
+              libhwcore.extract_all_objects(recursive: false)])
+
+test('rust-qemu-api-integration',
+    executable(
+        'rust-qemu-api-integration',
+        'tests/tests.rs',
+        override_options: ['rust_std=2021', 'build.rust_std=2021'],
+        rust_args: ['--test'],
+        install: false,
+        dependencies: [qemu_api, qemu_api_macros],
+        link_whole: [rust_qemu_api_objs, libqemuutil]),
+    args: [
+        '--test',
+        '--format', 'pretty',
+    ],
+    protocol: 'rust',
+    suite: ['unit', 'rust'])
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index e72fb4b4bb1..6bc68076aae 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -30,9 +30,6 @@ unsafe impl Sync for bindings::VMStateDescription {}
 pub mod definitions;
 pub mod device_class;
 
-#[cfg(test)]
-mod tests;
-
 use std::alloc::{GlobalAlloc, Layout};
 
 #[cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)]
diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs
deleted file mode 100644
index df54edbd4e2..00000000000
--- a/rust/qemu-api/src/tests.rs
+++ /dev/null
@@ -1,49 +0,0 @@
-// Copyright 2024, Linaro Limited
-// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
-// SPDX-License-Identifier: GPL-2.0-or-later
-
-use crate::{
-    bindings::*, declare_properties, define_property, device_class_init, vm_state_description,
-};
-
-#[test]
-fn test_device_decl_macros() {
-    // Test that macros can compile.
-    vm_state_description! {
-        VMSTATE,
-        name: c"name",
-        unmigratable: true,
-    }
-
-    #[repr(C)]
-    pub struct DummyState {
-        pub char_backend: CharBackend,
-        pub migrate_clock: bool,
-    }
-
-    declare_properties! {
-        DUMMY_PROPERTIES,
-            define_property!(
-                c"chardev",
-                DummyState,
-                char_backend,
-                unsafe { &qdev_prop_chr },
-                CharBackend
-            ),
-            define_property!(
-                c"migrate-clk",
-                DummyState,
-                migrate_clock,
-                unsafe { &qdev_prop_bool },
-                bool
-            ),
-    }
-
-    device_class_init! {
-        dummy_class_init,
-        props => DUMMY_PROPERTIES,
-        realize_fn => None,
-        reset_fn => None,
-        vmsd => VMSTATE,
-    }
-}
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
new file mode 100644
index 00000000000..aa1e0568c69
--- /dev/null
+++ b/rust/qemu-api/tests/tests.rs
@@ -0,0 +1,78 @@
+// Copyright 2024, Linaro Limited
+// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use core::ffi::CStr;
+
+use qemu_api::{
+    bindings::*,
+    declare_properties, define_property,
+    definitions::{Class, ObjectImpl},
+    device_class_init, vm_state_description,
+};
+
+#[test]
+fn test_device_decl_macros() {
+    // Test that macros can compile.
+    vm_state_description! {
+        VMSTATE,
+        name: c"name",
+        unmigratable: true,
+    }
+
+    #[repr(C)]
+    #[derive(qemu_api_macros::Object)]
+    pub struct DummyState {
+        pub _parent: DeviceState,
+        pub migrate_clock: bool,
+    }
+
+    #[repr(C)]
+    pub struct DummyClass {
+        pub _parent: DeviceClass,
+    }
+
+    declare_properties! {
+        DUMMY_PROPERTIES,
+            define_property!(
+                c"migrate-clk",
+                DummyState,
+                migrate_clock,
+                unsafe { &qdev_prop_bool },
+                bool
+            ),
+    }
+
+    device_class_init! {
+        dummy_class_init,
+        props => DUMMY_PROPERTIES,
+        realize_fn => None,
+        legacy_reset_fn => None,
+        vmsd => VMSTATE,
+    }
+
+    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 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;
+        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;
+    }
+
+    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_BASE_INIT: Option<
+            unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void),
+        > = None;
+    }
+
+    unsafe {
+        module_call_init(module_init_type::MODULE_INIT_QOM);
+        object_unref(object_new(DummyState::TYPE_NAME.as_ptr()) as *mut _);
+    }
+}
-- 
2.46.2

Re: [PATCH v2 07/13] rust: build integration test for the qemu_api crate

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Adjust the integration test to compile with a subset of QEMU object
> files, and make it actually create an object of the class it defines.
>
> Follow the Rust filesystem conventions, where tests go in tests/ if
> they use the library in the same way any other code would.
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

--
Best Regards
Junjie Mao

Re: [PATCH v2 07/13] rust: build integration test for the qemu_api crate

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:32PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:32 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 07/13] rust: build integration test for the qemu_api
>  crate
> X-Mailer: git-send-email 2.46.2
> 
> Adjust the integration test to compile with a subset of QEMU object
> files, and make it actually create an object of the class it defines.
> 
> Follow the Rust filesystem conventions, where tests go in tests/ if
> they use the library in the same way any other code would.
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                  | 10 ++++-
>  rust/qemu-api/meson.build    | 26 ++++++++++--
>  rust/qemu-api/src/lib.rs     |  3 --
>  rust/qemu-api/src/tests.rs   | 49 ----------------------
>  rust/qemu-api/tests/tests.rs | 78 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 110 insertions(+), 56 deletions(-)
>  delete mode 100644 rust/qemu-api/src/tests.rs
>  create mode 100644 rust/qemu-api/tests/tests.rs

Ran "make check-unit":

qemu:unit+rust / rust-qemu-api-integration          OK

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

[PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Paolo Bonzini]

Remove the duplicate code by using the module_init! macro; at the same time,
simplify how module_init! is used, by taking inspiration from the implementation
of #[derive(Object)].

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api-macros/src/lib.rs  | 33 +++-------------
 rust/qemu-api/src/definitions.rs | 66 ++++++++++++++------------------
 2 files changed, 33 insertions(+), 66 deletions(-)

diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 70e3f920460..a4bc5d01ee8 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -3,43 +3,20 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 use proc_macro::TokenStream;
-use quote::{format_ident, quote};
+use quote::quote;
 use syn::{parse_macro_input, DeriveInput};
 
 #[proc_macro_derive(Object)]
 pub fn derive_object(input: TokenStream) -> TokenStream {
     let input = parse_macro_input!(input as DeriveInput);
-
     let name = input.ident;
-    let module_static = format_ident!("__{}_LOAD_MODULE", name);
 
     let expanded = quote! {
-        #[allow(non_upper_case_globals)]
-        #[used]
-        #[cfg_attr(
-            not(any(target_vendor = "apple", target_os = "windows")),
-            link_section = ".init_array"
-        )]
-        #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
-        #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
-        pub static #module_static: extern "C" fn() = {
-            extern "C" fn __register() {
-                unsafe {
-                    ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::definitions::ObjectImpl>::TYPE_INFO);
-                }
+        ::qemu_api::module_init! {
+            MODULE_INIT_QOM => unsafe {
+                ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::definitions::ObjectImpl>::TYPE_INFO);
             }
-
-            extern "C" fn __load() {
-                unsafe {
-                    ::qemu_api::bindings::register_module_init(
-                        Some(__register),
-                        ::qemu_api::bindings::module_init_type::MODULE_INIT_QOM
-                    );
-                }
-            }
-
-            __load
-        };
+        }
     };
 
     TokenStream::from(expanded)
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index 3323a665d92..f180c38bfb2 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -29,51 +29,40 @@ pub trait Class {
 
 #[macro_export]
 macro_rules! module_init {
-    ($func:expr, $type:expr) => {
-        #[used]
-        #[cfg_attr(
-            not(any(target_vendor = "apple", target_os = "windows")),
-            link_section = ".init_array"
-        )]
-        #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
-        #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
-        pub static LOAD_MODULE: extern "C" fn() = {
-            extern "C" fn __load() {
-                unsafe {
-                    $crate::bindings::register_module_init(Some($func), $type);
-                }
-            }
-
-            __load
-        };
-    };
-    (qom: $func:ident => $body:block) => {
-        // NOTE: To have custom identifiers for the ctor func we need to either supply
-        // them directly as a macro argument or create them with a proc macro.
-        #[used]
-        #[cfg_attr(
-            not(any(target_vendor = "apple", target_os = "windows")),
-            link_section = ".init_array"
-        )]
-        #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
-        #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
-        pub static LOAD_MODULE: extern "C" fn() = {
-            extern "C" fn __load() {
-                unsafe extern "C" fn $func() {
+    ($type:ident => $body:block) => {
+        const _: () = {
+            #[used]
+            #[cfg_attr(
+                not(any(target_vendor = "apple", target_os = "windows")),
+                link_section = ".init_array"
+            )]
+            #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
+            #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
+            pub static LOAD_MODULE: extern "C" fn() = {
+                extern "C" fn init_fn() {
                     $body
                 }
 
-                unsafe {
-                    $crate::bindings::register_module_init(
-                        Some($func),
-                        $crate::bindings::module_init_type::MODULE_INIT_QOM,
-                    );
+                extern "C" fn ctor_fn() {
+                    unsafe {
+                        $crate::bindings::register_module_init(
+                            Some(init_fn),
+                            $crate::bindings::module_init_type::$type,
+                        );
+                    }
                 }
-            }
 
-            __load
+                ctor_fn
+            };
         };
     };
+
+    // shortcut because it's quite common that $body needs unsafe {}
+    ($type:ident => unsafe $body:block) => {
+        $crate::module_init! {
+            $type => { unsafe { $body } }
+        }
+    };
 }
 
 #[macro_export]
-- 
2.46.2

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Remove the duplicate code by using the module_init! macro; at the same time,
> simplify how module_init! is used, by taking inspiration from the implementation
> of #[derive(Object)].
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

One minor comment below.

> ---
>  rust/qemu-api-macros/src/lib.rs  | 33 +++-------------
>  rust/qemu-api/src/definitions.rs | 66 ++++++++++++++------------------
>  2 files changed, 33 insertions(+), 66 deletions(-)
>
<snip>
> diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
> index 3323a665d92..f180c38bfb2 100644
> --- a/rust/qemu-api/src/definitions.rs
> +++ b/rust/qemu-api/src/definitions.rs
> @@ -29,51 +29,40 @@ pub trait Class {
>
>  #[macro_export]
>  macro_rules! module_init {
<snip>
> +    ($type:ident => $body:block) => {
> +        const _: () = {
> +            #[used]
> +            #[cfg_attr(
> +                not(any(target_vendor = "apple", target_os = "windows")),
> +                link_section = ".init_array"
> +            )]
> +            #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
> +            #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
> +            pub static LOAD_MODULE: extern "C" fn() = {
> +                extern "C" fn init_fn() {

init_fn() should be unsafe fn according to the signature of
register_module_init. Being unsafe fn also makes sense here because it
is the caller, not init_fn() itself, that is responsible for the safety of
the unsafe code in the body.

--
Best Regards
Junjie Mao

>                      $body
>                  }
>
> -                unsafe {
> -                    $crate::bindings::register_module_init(
> -                        Some($func),
> -                        $crate::bindings::module_init_type::MODULE_INIT_QOM,
> -                    );
> +                extern "C" fn ctor_fn() {
> +                    unsafe {
> +                        $crate::bindings::register_module_init(
> +                            Some(init_fn),
> +                            $crate::bindings::module_init_type::$type,
> +                        );
> +                    }
>                  }
> -            }
>
> -            __load
> +                ctor_fn
> +            };
>          };
>      };
> +
> +    // shortcut because it's quite common that $body needs unsafe {}
> +    ($type:ident => unsafe $body:block) => {
> +        $crate::module_init! {
> +            $type => { unsafe { $body } }
> +        }
> +    };
>  }
>
>  #[macro_export]

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Paolo Bonzini]

On Tue, Oct 22, 2024 at 4:12 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
>
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > Remove the duplicate code by using the module_init! macro; at the same time,
> > simplify how module_init! is used, by taking inspiration from the implementation
> > of #[derive(Object)].
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
>
> One minor comment below.
>
> > ---
> >  rust/qemu-api-macros/src/lib.rs  | 33 +++-------------
> >  rust/qemu-api/src/definitions.rs | 66 ++++++++++++++------------------
> >  2 files changed, 33 insertions(+), 66 deletions(-)
> >
> <snip>
> > diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
> > index 3323a665d92..f180c38bfb2 100644
> > --- a/rust/qemu-api/src/definitions.rs
> > +++ b/rust/qemu-api/src/definitions.rs
> > @@ -29,51 +29,40 @@ pub trait Class {
> >
> >  #[macro_export]
> >  macro_rules! module_init {
> <snip>
> > +    ($type:ident => $body:block) => {
> > +        const _: () = {
> > +            #[used]
> > +            #[cfg_attr(
> > +                not(any(target_vendor = "apple", target_os = "windows")),
> > +                link_section = ".init_array"
> > +            )]
> > +            #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
> > +            #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
> > +            pub static LOAD_MODULE: extern "C" fn() = {
> > +                extern "C" fn init_fn() {
>
> init_fn() should be unsafe fn according to the signature of
> register_module_init.

I think it *can* be unsafe (which bindgen does by default). It's
always okay to pass a non-unsafe function as unsafe function pointer:

fn f() {
    println!("abc");
}

fn g(pf: unsafe fn()) {
    unsafe {
        pf();
    }
}

fn main() {
    g(f);
}

> Being unsafe fn also makes sense here because it
> is the caller, not init_fn() itself, that is responsible for the safety of
> the unsafe code in the body.

Isn't it the opposite? Since the caller of module_init! is responsible
for the safety, init_fn() itself can be safe. With
unsafe_op_in_unsafe_fn it's not a big deal; but without it, an unsafe
init_fn would hide what is safe and what is not in the argument of
module_init!.

It's also relevant in this respect that init_fn is called *after*
main(), only ctor_fn() is called before main.

Thanks,

Paolo

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Tue, Oct 22, 2024 at 4:12 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
>> > +    ($type:ident => $body:block) => {
>> > +        const _: () = {
>> > +            #[used]
>> > +            #[cfg_attr(
>> > +                not(any(target_vendor = "apple", target_os = "windows")),
>> > +                link_section = ".init_array"
>> > +            )]
>> > +            #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
>> > +            #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
>> > +            pub static LOAD_MODULE: extern "C" fn() = {
>> > +                extern "C" fn init_fn() {
>>
>> init_fn() should be unsafe fn according to the signature of
>> register_module_init.
>
> I think it *can* be unsafe (which bindgen does by default). It's
> always okay to pass a non-unsafe function as unsafe function pointer:
>
> fn f() {
>     println!("abc");
> }
>
> fn g(pf: unsafe fn()) {
>     unsafe {
>         pf();
>     }
> }
>
> fn main() {
>     g(f);
> }
>
>> Being unsafe fn also makes sense here because it
>> is the caller, not init_fn() itself, that is responsible for the safety of
>> the unsafe code in the body.
>
> Isn't it the opposite? Since the caller of module_init! is responsible
> for the safety, init_fn() itself can be safe.

My understanding is that:

  1. init_fn() is called by QEMU QOM subsystem with certain timing
     (e.g., called after main()) and concurrency (e.g., all init_fn()
     are called sequentially) preconditions.

  2. The caller of module_init! is responsible for justifying the safety
     of their $body under the preconditions established in #1.

"unsafe fn" in Rust is designed to mark functions with safety
preconditions so that the compiler can raise an error if the caller
forgets that it has those preconditions to uphold [1].

Under that interpretation, a safe "fn init_fn()" reads that init_fn() is
safe to call without the preconditions mentioned in #1. That is rarely
the case to me.

Using "unsafe" to mark the responsibility of device backends in #2 looks
like repurposing the keyword. That may make more sense in this specific
context because:

  1. the callers of init_fn() are in C, so Rust compiler cannot really
     check them,

  2. the caller will by design upholds the safety preconditions
     regardless of whether a specific callback needs it or not, and

  3. without unsafe_op_in_unsafe_fn an unsafe fn hides unsafe operation
     in its body from the compiler.

If that's what we prefer, I would suggest add the considerations above
to the code as comments, so that future readers are not confused by the
usage of "unsafe" here.

[1] https://rust-lang.github.io/rfcs/2585-unsafe-block-in-unsafe-fn.html

> With unsafe_op_in_unsafe_fn it's not a big deal; but without it, an
> unsafe init_fn would hide what is safe and what is not in the argument
> of module_init!.
>
> It's also relevant in this respect that init_fn is called *after*
> main(), only ctor_fn() is called before main.
>
> Thanks,
>
> Paolo

--
Best Regards
Junjie Mao

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Paolo Bonzini]

On Tue, Oct 22, 2024 at 9:12 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
> My understanding is that:
>
>   1. init_fn() is called by QEMU QOM subsystem with certain timing
>      (e.g., called after main()) and concurrency (e.g., all init_fn()
>      are called sequentially) preconditions.
>
>   2. The caller of module_init! is responsible for justifying the safety
>      of their $body under the preconditions established in #1.
>
> "unsafe fn" in Rust is designed to mark functions with safety
> preconditions so that the compiler can raise an error if the caller
> forgets that it has those preconditions to uphold [1].
>
> Under that interpretation, a safe "fn init_fn()" reads that init_fn() is
> safe to call without the preconditions mentioned in #1. That is rarely
> the case to me.
>
> Using "unsafe" to mark the responsibility of device backends in #2 looks
> like repurposing the keyword. That may make more sense in this specific
> context because:
>
>   1. the callers of init_fn() are in C, so Rust compiler cannot really
>      check them,
>
>   2. the caller will by design upholds the safety preconditions
>      regardless of whether a specific callback needs it or not, and

Or at least will try.

>   3. without unsafe_op_in_unsafe_fn an unsafe fn hides unsafe operation
>      in its body from the compiler.

4. In this specific case, init_fn is only used from ctor_fn and even
there only as a function pointer; it is not visible from outside. So
the "unsafe" marker's only purpose is documentation (and in fact, as
you point out in (3), it would even be harmful without
unsafe_op_in_unsafe_fn).

> If that's what we prefer, I would suggest add the considerations above
> to the code as comments, so that future readers are not confused by the
> usage of "unsafe" here.

Yes, I agree that since we're talking about a macro (not a function
which can itself be annotated with "unsafe") that lies at the C<->Rust
boundary, this is mostly a documentation issue.

In general the documentation of qemu-api and qemu-api-macros leaves
something to be desired. This is okay, but it is also the kind of
technical debt that we need to look at as soon as we can actually get
the thing to compile. :)

I'll add it shortly to https://wiki.qemu.org/Features/Rust.

Paolo

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Kevin Wolf]

Am 22.10.2024 um 08:00 hat Junjie Mao geschrieben:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On Tue, Oct 22, 2024 at 4:12 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
> >> > +    ($type:ident => $body:block) => {
> >> > +        const _: () = {
> >> > +            #[used]
> >> > +            #[cfg_attr(
> >> > +                not(any(target_vendor = "apple", target_os = "windows")),
> >> > +                link_section = ".init_array"
> >> > +            )]
> >> > +            #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
> >> > +            #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
> >> > +            pub static LOAD_MODULE: extern "C" fn() = {
> >> > +                extern "C" fn init_fn() {
> >>
> >> init_fn() should be unsafe fn according to the signature of
> >> register_module_init.
> >
> > I think it *can* be unsafe (which bindgen does by default). It's
> > always okay to pass a non-unsafe function as unsafe function pointer:
> >
> > fn f() {
> >     println!("abc");
> > }
> >
> > fn g(pf: unsafe fn()) {
> >     unsafe {
> >         pf();
> >     }
> > }
> >
> > fn main() {
> >     g(f);
> > }
> >
> >> Being unsafe fn also makes sense here because it
> >> is the caller, not init_fn() itself, that is responsible for the safety of
> >> the unsafe code in the body.
> >
> > Isn't it the opposite? Since the caller of module_init! is responsible
> > for the safety, init_fn() itself can be safe.
> 
> My understanding is that:
> 
>   1. init_fn() is called by QEMU QOM subsystem with certain timing
>      (e.g., called after main()) and concurrency (e.g., all init_fn()
>      are called sequentially) preconditions.

Though these conditions are not a matter of safety, but of correctness.

>   2. The caller of module_init! is responsible for justifying the safety
>      of their $body under the preconditions established in #1.
> 
> "unsafe fn" in Rust is designed to mark functions with safety
> preconditions so that the compiler can raise an error if the caller
> forgets that it has those preconditions to uphold [1].

I don't think we expect to have preconditions here that are required for
safety (in the sense with which the term is used in Rust).

But safe code is not automatically correct.

If you added "unsafe" for every function that requires some
preconditions to be met so that it functions correctly (instead of only
so that it doesn't have undefined behaviour on the language level), then
you would annotate almost every function as "unsafe".

I think the rule of thumb for marking a function unsafe is when you have
unsafe code inside of it whose safety (not correctness!) depends on a
condition that the caller must reason about because you can't guarantee
it in the function itself.

This macro should probably only be used with code that can guarantee the
safety of its unsafe blocks in itself. The C side of constructors can't
make many guarantees anyway, and there is nobody who would reason about
them.

Kevin

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Junjie Mao]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 22.10.2024 um 08:00 hat Junjie Mao geschrieben:
>>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>> > On Tue, Oct 22, 2024 at 4:12 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
>> >> > +    ($type:ident => $body:block) => {
>> >> > +        const _: () = {
>> >> > +            #[used]
>> >> > +            #[cfg_attr(
>> >> > +                not(any(target_vendor = "apple", target_os = "windows")),
>> >> > +                link_section = ".init_array"
>> >> > +            )]
>> >> > +            #[cfg_attr(target_vendor = "apple", link_section = "__DATA,__mod_init_func")]
>> >> > +            #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
>> >> > +            pub static LOAD_MODULE: extern "C" fn() = {
>> >> > +                extern "C" fn init_fn() {
>> >>
>> >> init_fn() should be unsafe fn according to the signature of
>> >> register_module_init.
>> >
>> > I think it *can* be unsafe (which bindgen does by default). It's
>> > always okay to pass a non-unsafe function as unsafe function pointer:
>> >
>> > fn f() {
>> >     println!("abc");
>> > }
>> >
>> > fn g(pf: unsafe fn()) {
>> >     unsafe {
>> >         pf();
>> >     }
>> > }
>> >
>> > fn main() {
>> >     g(f);
>> > }
>> >
>> >> Being unsafe fn also makes sense here because it
>> >> is the caller, not init_fn() itself, that is responsible for the safety of
>> >> the unsafe code in the body.
>> >
>> > Isn't it the opposite? Since the caller of module_init! is responsible
>> > for the safety, init_fn() itself can be safe.
>>
>> My understanding is that:
>>
>>   1. init_fn() is called by QEMU QOM subsystem with certain timing
>>      (e.g., called after main()) and concurrency (e.g., all init_fn()
>>      are called sequentially) preconditions.
>
> Though these conditions are not a matter of safety, but of correctness.
>

The "no concurrency" condition is related to Rust safety. Deep in
type_register_static(), which is commonly used in such init_fn(), there
is a modification to mutable statics in type_table_get():

    static GHashTable *type_table_get(void)
    {
        static GHashTable *type_table;

        if (type_table == NULL) {
            type_table = g_hash_table_new(g_str_hash, g_str_equal);
        }

        return type_table;
    }

It'll cause data race when multiple init_fn() are concurrently called,
and data races has UB.

Also, glib hashtables are not automatically thread-safe [1]:

    ... individual data structure instances are not automatically locked
    for performance reasons. For example, you must coordinate accesses
    to the same GHashTable from multiple threads.

[1] https://docs.gtk.org/glib/threads.html

I don't have evidence yet for the relationship between timing condition
and Rust safety, though.

>>   2. The caller of module_init! is responsible for justifying the safety
>>      of their $body under the preconditions established in #1.
>>
>> "unsafe fn" in Rust is designed to mark functions with safety
>> preconditions so that the compiler can raise an error if the caller
>> forgets that it has those preconditions to uphold [1].
>
> I don't think we expect to have preconditions here that are required for
> safety (in the sense with which the term is used in Rust).
>
> But safe code is not automatically correct.
>
> If you added "unsafe" for every function that requires some
> preconditions to be met so that it functions correctly (instead of only
> so that it doesn't have undefined behaviour on the language level), then
> you would annotate almost every function as "unsafe".
>
> I think the rule of thumb for marking a function unsafe is when you have
> unsafe code inside of it whose safety (not correctness!) depends on a
> condition that the caller must reason about because you can't guarantee
> it in the function itself.

I fully agree with your rules for "unsafe fn" uses.

--
Best Regards
Junjie Mao

>
> This macro should probably only be used with code that can guarantee
> the safety of its unsafe blocks in itself. The C side of constructors
> can't make many guarantees anyway, and there is nobody who would
> reason about them.
>
> Kevin

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:33PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:33 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 08/13] rust: cleanup module_init!, use it from
>  #[derive(Object)]
> X-Mailer: git-send-email 2.46.2
> 
> Remove the duplicate code by using the module_init! macro; at the same time,
> simplify how module_init! is used, by taking inspiration from the implementation
> of #[derive(Object)].
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api-macros/src/lib.rs  | 33 +++-------------
>  rust/qemu-api/src/definitions.rs | 66 ++++++++++++++------------------
>  2 files changed, 33 insertions(+), 66 deletions(-)

LGTM (with some questions related type_init usage inline)

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

> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
> index 70e3f920460..a4bc5d01ee8 100644
> --- a/rust/qemu-api-macros/src/lib.rs
> +++ b/rust/qemu-api-macros/src/lib.rs
> @@ -3,43 +3,20 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #[proc_macro_derive(Object)]
>  pub fn derive_object(input: TokenStream) -> TokenStream {

[snip]

> +            MODULE_INIT_QOM => unsafe {
> +                ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::definitions::ObjectImpl>::TYPE_INFO);
>              }

I want to see how general this macro could be, so I checked current
type_init() cases for TypeInfo. In most cases, only type_register_static()
is called directly in the init_fn() callback.

There are only two exceptions:

1. Some init_fn callbacks contain more complex validation or register
logic.

  For example, in backends/hostmem-epc.c, sgx_epc_backed_info involves
  extra check before type_register_static().

  static void register_types(void)
  {
      int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
      if (fd >= 0) {
          close(fd);

          type_register_static(&sgx_epc_backed_info);
      }
  }


  And in hw/audio/intel-hda.c, there's extra pci_register_soundhw afer
  type_register_static():

  static void intel_hda_register_types(void)
  {
      type_register_static(&hda_codec_bus_info);
      type_register_static(&intel_hda_info);
      type_register_static(&intel_hda_info_ich6);
      type_register_static(&intel_hda_info_ich9);
      type_register_static(&hda_codec_device_type_info);
      pci_register_soundhw("hda", "Intel HD Audio", intel_hda_and_codec_init);
  }

  The device can define a custom init_fn() for TypeInfo based on
  module_init!, but I wonder if the examples above are valid. Is it
  allowed to include other logic in init_fn()?


2. Some init_fn callbacks use type_register() instead of
type_register_static().

  TypeImpl *type_register_static(const TypeInfo *info)
  {
      return type_register(info);
  }

  It seems that type_register() and type_register_static() are the same.
  I guess I could clean up one of them, right? (type_register() was added
  by your earlie commit 049cb3cfdac1 :-) ).

Thanks,
Zhao

Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Paolo Bonzini]

On 10/25/24 10:59, Zhao Liu wrote:
> I want to see how general this macro could be, so I checked current
> type_init() cases for TypeInfo. In most cases, only type_register_static()
> is called directly in the init_fn() callback.

First of all, it's okay if the Rust code does not cover everything.  But 
looking at your cases:

> There are only two exceptions:
> 
> 1. Some init_fn callbacks contain more complex validation or register
> logic.
> 
>    For example, in backends/hostmem-epc.c, sgx_epc_backed_info involves
>    extra check before type_register_static().
> 
>    static void register_types(void)
>    {
>        int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
>        if (fd >= 0) {
>            close(fd);
> 
>            type_register_static(&sgx_epc_backed_info);
>        }
>    }

This one is okay to just change to type_register_static(), moving the 
/dev/sgx_vepc code to the "complete" callback.

> 
>    And in hw/audio/intel-hda.c, there's extra pci_register_soundhw afer
>    type_register_static():
> 
>    static void intel_hda_register_types(void)
>    {
>        type_register_static(&hda_codec_bus_info);
>        type_register_static(&intel_hda_info);
>        type_register_static(&intel_hda_info_ich6);
>        type_register_static(&intel_hda_info_ich9);
>        type_register_static(&hda_codec_device_type_info);
>        pci_register_soundhw("hda", "Intel HD Audio", intel_hda_and_codec_init);
>    }
> 
>    The device can define a custom init_fn() for TypeInfo based on
>    module_init!, but I wonder if the examples above are valid. Is it
>    allowed to include other logic in init_fn()?

Yes, it is but it is also possible to use your own module_init! 
invocation outside #[derive(Object)].
> 2. Some init_fn callbacks use type_register() instead of
> type_register_static().
> 
>    TypeImpl *type_register_static(const TypeInfo *info)
>    {
>        return type_register(info);
>    }
> 
>    It seems that type_register() and type_register_static() are the same.
>    I guess I could clean up one of them, right? (type_register() was added
>    by your earlie commit 049cb3cfdac1 :-) ).

Yeah, you can!

Paolo

[PATCH v2 09/13] rust: clean up define_property macro

[Paolo Bonzini]

Use the "struct update" syntax to initialize most of the fields to zero,
and simplify the handmade type-checking of $name.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/device_class.rs | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 2219b9f73d0..5aba426d243 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -29,44 +29,27 @@ macro_rules! device_class_init {
 macro_rules! define_property {
     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
         $crate::bindings::Property {
-            name: {
-                #[used]
-                static _TEMP: &::core::ffi::CStr = $name;
-                _TEMP.as_ptr()
-            },
+            // use associated function syntax for type checking
+            name: ::core::ffi::CStr::as_ptr($name),
             info: $prop,
             offset: ::core::mem::offset_of!($state, $field)
                 .try_into()
                 .expect("Could not fit offset value to type"),
-            bitnr: 0,
-            bitmask: 0,
             set_default: true,
             defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() },
-            arrayoffset: 0,
-            arrayinfo: ::core::ptr::null(),
-            arrayfieldsize: 0,
-            link_type: ::core::ptr::null(),
+            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
         }
     };
     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
         $crate::bindings::Property {
-            name: {
-                #[used]
-                static _TEMP: &::core::ffi::CStr = $name;
-                _TEMP.as_ptr()
-            },
+            // use associated function syntax for type checking
+            name: ::core::ffi::CStr::as_ptr($name),
             info: $prop,
             offset: ::core::mem::offset_of!($state, $field)
                 .try_into()
                 .expect("Could not fit offset value to type"),
-            bitnr: 0,
-            bitmask: 0,
             set_default: false,
-            defval: $crate::bindings::Property__bindgen_ty_1 { i: 0 },
-            arrayoffset: 0,
-            arrayinfo: ::core::ptr::null(),
-            arrayfieldsize: 0,
-            link_type: ::core::ptr::null(),
+            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
         }
     };
 }
-- 
2.46.2

Re: [PATCH v2 09/13] rust: clean up define_property macro

[Kevin Wolf]

Am 21.10.2024 um 18:35 hat Paolo Bonzini geschrieben:
> Use the "struct update" syntax to initialize most of the fields to zero,
> and simplify the handmade type-checking of $name.
> 
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/device_class.rs | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
> index 2219b9f73d0..5aba426d243 100644
> --- a/rust/qemu-api/src/device_class.rs
> +++ b/rust/qemu-api/src/device_class.rs
> @@ -29,44 +29,27 @@ macro_rules! device_class_init {
>  macro_rules! define_property {
>      ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
>          $crate::bindings::Property {
> -            name: {
> -                #[used]
> -                static _TEMP: &::core::ffi::CStr = $name;
> -                _TEMP.as_ptr()
> -            },
> +            // use associated function syntax for type checking
> +            name: ::core::ffi::CStr::as_ptr($name),

I like this part.

>              info: $prop,
>              offset: ::core::mem::offset_of!($state, $field)
>                  .try_into()
>                  .expect("Could not fit offset value to type"),
> -            bitnr: 0,
> -            bitmask: 0,
>              set_default: true,
>              defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() },
> -            arrayoffset: 0,
> -            arrayinfo: ::core::ptr::null(),
> -            arrayfieldsize: 0,
> -            link_type: ::core::ptr::null(),
> +            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }

But is it really worth introducing unsafe code just for a more compact
notation? If the compiler doesn't actually understand the pattern, it
might even be less efficient than what we had (i.e. if it really creates
the zeroed object and copies stuff over).

Kevin

Re: [PATCH v2 09/13] rust: clean up define_property macro

[Paolo Bonzini]

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

Il mar 22 ott 2024, 21:46 Kevin Wolf <kwolf@redhat.com> ha scritto:

> >              info: $prop,
> >              offset: ::core::mem::offset_of!($state, $field)
> >                  .try_into()
> >                  .expect("Could not fit offset value to type"),
> > -            bitnr: 0,
> > -            bitmask: 0,
> >              set_default: true,
> >              defval: $crate::bindings::Property__bindgen_ty_1 { u:
> $defval.into() },
> > -            arrayoffset: 0,
> > -            arrayinfo: ::core::ptr::null(),
> > -            arrayfieldsize: 0,
> > -            link_type: ::core::ptr::null(),
> > +            ..unsafe {
> ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init()
> }
>
> But is it really worth introducing unsafe code just for a more compact
> notation? If the compiler doesn't actually understand the pattern, it
> might even be less efficient than what we had (i.e. if it really creates
> the zeroed object and copies stuff over).
>

It goes away later in the series (patch 11 replaces it with a more
manageable "Zeroable::ZERO"), but I wanted to split the patches in "parts
that change existing code" and "parts that introduce new concepts".

I agree that it's not optimal either way, I went like this because at this
point this "unsafe { zeroed() }" idiom is present several times in the code
and one more doesn't really change much.

Paolo


> Kevin
>
>

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

Re: [PATCH v2 09/13] rust: clean up define_property macro

[Manos Pitsidianakis]

Hello Paolo,

On Mon, 21 Oct 2024 19:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>Use the "struct update" syntax to initialize most of the fields to zero,
>and simplify the handmade type-checking of $name.

Note: It wasn't meant for type checking but for making sure the linker 
doesn't strip the symbol (hence the #[used] attribute). These were left 
over when I was debugging linker issues and slapped #[used] everywhere 
but they are not needed in this case indeed.

>
>Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> rust/qemu-api/src/device_class.rs | 29 ++++++-----------------------
> 1 file changed, 6 insertions(+), 23 deletions(-)
>
>diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
>index 2219b9f73d0..5aba426d243 100644
>--- a/rust/qemu-api/src/device_class.rs
>+++ b/rust/qemu-api/src/device_class.rs
>@@ -29,44 +29,27 @@ macro_rules! device_class_init {
> macro_rules! define_property {
>     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
>         $crate::bindings::Property {
>-            name: {
>-                #[used]
>-                static _TEMP: &::core::ffi::CStr = $name;
>-                _TEMP.as_ptr()
>-            },
>+            // use associated function syntax for type checking
>+            name: ::core::ffi::CStr::as_ptr($name),
>             info: $prop,
>             offset: ::core::mem::offset_of!($state, $field)
>                 .try_into()
>                 .expect("Could not fit offset value to type"),
>-            bitnr: 0,
>-            bitmask: 0,
>             set_default: true,
>             defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() },
>-            arrayoffset: 0,
>-            arrayinfo: ::core::ptr::null(),
>-            arrayfieldsize: 0,
>-            link_type: ::core::ptr::null(),
>+            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }

Call it personal taste but I don't like emulating C's zero initializer 
syntax in Rust :) Is it that much trouble to explicitly write down every 
field in a macro, anyway? No strong preference here though.

>         }
>     };
>     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
>         $crate::bindings::Property {
>-            name: {
>-                #[used]
>-                static _TEMP: &::core::ffi::CStr = $name;
>-                _TEMP.as_ptr()
>-            },
>+            // use associated function syntax for type checking
>+            name: ::core::ffi::CStr::as_ptr($name),
>             info: $prop,
>             offset: ::core::mem::offset_of!($state, $field)
>                 .try_into()
>                 .expect("Could not fit offset value to type"),
>-            bitnr: 0,
>-            bitmask: 0,
>             set_default: false,
>-            defval: $crate::bindings::Property__bindgen_ty_1 { i: 0 },
>-            arrayoffset: 0,
>-            arrayinfo: ::core::ptr::null(),
>-            arrayfieldsize: 0,
>-            link_type: ::core::ptr::null(),
>+            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
>         }
>     };
> }
>-- 
>2.46.2
>

Re: [PATCH v2 09/13] rust: clean up define_property macro

[Paolo Bonzini]

On 10/23/24 12:38, Manos Pitsidianakis wrote:
> Hello Paolo,
> 
> On Mon, 21 Oct 2024 19:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Use the "struct update" syntax to initialize most of the fields to zero,
>> and simplify the handmade type-checking of $name.
> 
> Note: It wasn't meant for type checking but for making sure the linker
> doesn't strip the symbol (hence the #[used] attribute). These were left
> over when I was debugging linker issues and slapped #[used] everywhere
> but they are not needed in this case indeed.

Well, it does do type checking as well, :) otherwise you end up 
duck-typing on whether $name as as_ptr().  I guess you are okay with the 
change below and the comment:

>> -            name: {
>> -                #[used]
>> -                static _TEMP: &::core::ffi::CStr = $name;
>> -                _TEMP.as_ptr()
>> -            },
>> +            // use associated function syntax for type checking
>> +            name: ::core::ffi::CStr::as_ptr($name),

?

>>              info: $prop,
>>              offset: ::core::mem::offset_of!($state, $field)
>>                  .try_into()
>>                  .expect("Could not fit offset value to type"),
>> -            bitnr: 0,
>> -            bitmask: 0,
>>              set_default: true,
>>              defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() },
>> -            arrayoffset: 0,
>> -            arrayinfo: ::core::ptr::null(),
>> -            arrayfieldsize: 0,
>> -            link_type: ::core::ptr::null(),
>> +            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
> 
> Call it personal taste but I don't like emulating C's zero initializer
> syntax in Rust :) Is it that much trouble to explicitly write down every
> field in a macro, anyway? No strong preference here though.

Rust does make generous use of "..Default::default()", so I think it's 
more idiomatic to use the struct update syntax.  We just cannot use it 
here because it's not const-ified.

I'm okay with switching from Zeroable::ZERO to something like 
ConstDefault::CONST_DEFAULT; it's basically just a rename.  On the other 
hand you do rely on "zero-ness" in PL011State::init()... so I thought I 
was actually following your style. :)

Paolo

Re: [PATCH v2 09/13] rust: clean up define_property macro

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:34PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:34 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 09/13] rust: clean up define_property macro
> X-Mailer: git-send-email 2.46.2
> 
> Use the "struct update" syntax to initialize most of the fields to zero,
> and simplify the handmade type-checking of $name.
> 
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/device_class.rs | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)

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

[PATCH v2 10/13] qdev: make properties array "const"

[Paolo Bonzini]

Constify all accesses to qdev properties, except for the
ObjectPropertyAccessor itself.  This makes it possible to place them in
read-only memory, and also lets Rust bindings switch from "static mut"
arrays to "static"; which is advantageous, because mutable statics are
highly discouraged.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/qdev-core.h       |  4 ++--
 include/hw/qdev-properties.h |  4 ++--
 hw/core/qdev-properties.c    | 26 +++++++++++++-------------
 system/qdev-monitor.c        |  2 +-
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index aa97c34a4be..f9fa291cc63 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -132,7 +132,7 @@ struct DeviceClass {
      * ensures a compile-time error if someone attempts to assign
      * dc->props directly.
      */
-    Property *props_;
+    const Property *props_;
 
     /**
      * @user_creatable: Can user instantiate with -device / device_add?
@@ -935,7 +935,7 @@ char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev);
  * you attempt to add an existing property defined by a parent class.
  * To modify an inherited property you need to use????
  */
-void device_class_set_props(DeviceClass *dc, Property *props);
+void device_class_set_props(DeviceClass *dc, const Property *props);
 
 /**
  * device_class_set_parent_realize() - set up for chaining realize fns
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 09aa04ca1e2..26ebd230685 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -37,7 +37,7 @@ struct PropertyInfo {
     int (*print)(Object *obj, Property *prop, char *dest, size_t len);
     void (*set_default_value)(ObjectProperty *op, const Property *prop);
     ObjectProperty *(*create)(ObjectClass *oc, const char *name,
-                              Property *prop);
+                              const Property *prop);
     ObjectPropertyAccessor *get;
     ObjectPropertyAccessor *set;
     ObjectPropertyRelease *release;
@@ -223,7 +223,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
  * On error, store error in @errp.  Static properties access data in a struct.
  * The type of the QOM property is derived from prop->info.
  */
-void qdev_property_add_static(DeviceState *dev, Property *prop);
+void qdev_property_add_static(DeviceState *dev, const Property *prop);
 
 /**
  * qdev_alias_all_properties: Create aliases on source for all target properties
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 86a583574dd..315196bd85a 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -749,7 +749,7 @@ const PropertyInfo qdev_prop_array = {
 
 /* --- public helpers --- */
 
-static Property *qdev_prop_walk(Property *props, const char *name)
+static const Property *qdev_prop_walk(const Property *props, const char *name)
 {
     if (!props) {
         return NULL;
@@ -763,10 +763,10 @@ static Property *qdev_prop_walk(Property *props, const char *name)
     return NULL;
 }
 
-static Property *qdev_prop_find(DeviceState *dev, const char *name)
+static const Property *qdev_prop_find(DeviceState *dev, const char *name)
 {
     ObjectClass *class;
-    Property *prop;
+    const Property *prop;
 
     /* device properties */
     class = object_get_class(OBJECT(dev));
@@ -840,7 +840,7 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value)
 
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
 {
-    Property *prop;
+    const Property *prop;
 
     prop = qdev_prop_find(dev, name);
     object_property_set_str(OBJECT(dev), name,
@@ -956,7 +956,7 @@ const PropertyInfo qdev_prop_size = {
 /* --- object link property --- */
 
 static ObjectProperty *create_link_property(ObjectClass *oc, const char *name,
-                                            Property *prop)
+                                            const Property *prop)
 {
     return object_class_property_add_link(oc, name, prop->link_type,
                                           prop->offset,
@@ -969,7 +969,7 @@ const PropertyInfo qdev_prop_link = {
     .create = create_link_property,
 };
 
-void qdev_property_add_static(DeviceState *dev, Property *prop)
+void qdev_property_add_static(DeviceState *dev, const Property *prop)
 {
     Object *obj = OBJECT(dev);
     ObjectProperty *op;
@@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
                              field_prop_getter(prop->info),
                              field_prop_setter(prop->info),
                              prop->info->release,
-                             prop);
+                             (Property *)prop);
 
     object_property_set_description(obj, prop->name,
                                     prop->info->description);
@@ -994,7 +994,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
 }
 
 static void qdev_class_add_property(DeviceClass *klass, const char *name,
-                                    Property *prop)
+                                    const Property *prop)
 {
     ObjectClass *oc = OBJECT_CLASS(klass);
     ObjectProperty *op;
@@ -1007,7 +1007,7 @@ static void qdev_class_add_property(DeviceClass *klass, const char *name,
                                        field_prop_getter(prop->info),
                                        field_prop_setter(prop->info),
                                        prop->info->release,
-                                       prop);
+                                       (Property *)prop);
     }
     if (prop->set_default) {
         prop->info->set_default_value(op, prop);
@@ -1046,7 +1046,7 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
  * Do not use this in new code!  QOM Properties added through this interface
  * will be given names in the "legacy" namespace.
  */
-static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop)
+static void qdev_class_add_legacy_property(DeviceClass *dc, const Property *prop)
 {
     g_autofree char *name = NULL;
 
@@ -1058,12 +1058,12 @@ static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop)
     name = g_strdup_printf("legacy-%s", prop->name);
     object_class_property_add(OBJECT_CLASS(dc), name, "str",
         prop->info->print ? qdev_get_legacy_property : prop->info->get,
-        NULL, NULL, prop);
+        NULL, NULL, (Property *)prop);
 }
 
-void device_class_set_props(DeviceClass *dc, Property *props)
+void device_class_set_props(DeviceClass *dc, const Property *props)
 {
-    Property *prop;
+    const Property *prop;
 
     dc->props_ = props;
     for (prop = props; prop && prop->name; prop++) {
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 44994ea0e16..c346ea6ae4b 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -751,7 +751,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 
 #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
 
-static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
+static void qdev_print_props(Monitor *mon, DeviceState *dev, const Property *props,
                              int indent)
 {
     if (!props)
-- 
2.46.2

Re: [PATCH v2 10/13] qdev: make properties array "const"

[Philippe Mathieu-Daudé]

Hi,

On 21/10/24 13:35, Paolo Bonzini wrote:
> Constify all accesses to qdev properties, except for the
> ObjectPropertyAccessor itself.  This makes it possible to place them in
> read-only memory, and also lets Rust bindings switch from "static mut"
> arrays to "static"; which is advantageous, because mutable statics are
> highly discouraged.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/hw/qdev-core.h       |  4 ++--
>   include/hw/qdev-properties.h |  4 ++--
>   hw/core/qdev-properties.c    | 26 +++++++++++++-------------
>   system/qdev-monitor.c        |  2 +-
>   4 files changed, 18 insertions(+), 18 deletions(-)


> -void qdev_property_add_static(DeviceState *dev, Property *prop)
> +void qdev_property_add_static(DeviceState *dev, const Property *prop)
>   {
>       Object *obj = OBJECT(dev);
>       ObjectProperty *op;
> @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
>                                field_prop_getter(prop->info),
>                                field_prop_setter(prop->info),
>                                prop->info->release,
> -                             prop);
> +                             (Property *)prop);

I like the overall patch idea, but I'm not keep on casting
const to non-const. Should we adapt the callee -- here
object_property_add() -- to also take a const argument?

>   
>       object_property_set_description(obj, prop->name,
>                                       prop->info->description);
> @@ -994,7 +994,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
>   }
>   
>   static void qdev_class_add_property(DeviceClass *klass, const char *name,
> -                                    Property *prop)
> +                                    const Property *prop)
>   {
>       ObjectClass *oc = OBJECT_CLASS(klass);
>       ObjectProperty *op;
> @@ -1007,7 +1007,7 @@ static void qdev_class_add_property(DeviceClass *klass, const char *name,
>                                          field_prop_getter(prop->info),
>                                          field_prop_setter(prop->info),
>                                          prop->info->release,
> -                                       prop);
> +                                       (Property *)prop);
>       }
>       if (prop->set_default) {
>           prop->info->set_default_value(op, prop);
> @@ -1046,7 +1046,7 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v,
>    * Do not use this in new code!  QOM Properties added through this interface
>    * will be given names in the "legacy" namespace.
>    */
> -static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop)
> +static void qdev_class_add_legacy_property(DeviceClass *dc, const Property *prop)
>   {
>       g_autofree char *name = NULL;
>   
> @@ -1058,12 +1058,12 @@ static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop)
>       name = g_strdup_printf("legacy-%s", prop->name);
>       object_class_property_add(OBJECT_CLASS(dc), name, "str",
>           prop->info->print ? qdev_get_legacy_property : prop->info->get,
> -        NULL, NULL, prop);
> +        NULL, NULL, (Property *)prop);
>   }

Re: [PATCH v2 10/13] qdev: make properties array "const"

[Paolo Bonzini]

On Tue, Oct 22, 2024 at 6:31 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
> > -void qdev_property_add_static(DeviceState *dev, Property *prop)
> > +void qdev_property_add_static(DeviceState *dev, const Property *prop)
> >   {
> >       Object *obj = OBJECT(dev);
> >       ObjectProperty *op;
> > @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
> >                                field_prop_getter(prop->info),
> >                                field_prop_setter(prop->info),
> >                                prop->info->release,
> > -                             prop);
> > +                             (Property *)prop);
>
> I like the overall patch idea, but I'm not keen on casting
> const to non-const. Should we adapt the callee -- here
> object_property_add() -- to also take a const argument?

This argument goes into prop->opaque and is passed to all
accessor/resolver/finalizers functions. So it would be a much larger
change because it needs to change all those functions from "void
*opaque" to "const void *opaque".

It would also be an issue because some finalizers write to the opaque
for good reason:

static void object_finalize_child_property(
    Object *obj, const char *name, void *opaque)
{
    Object *child = opaque;

    if (child->class->unparent) {
        (child->class->unparent)(child);
    }
    child->parent = NULL; // <--- here
    object_unref(child);
}

So, it's not great but it seems necessary. At least keeping the const
within qdev properties makes things "safer" within that realm.

Paolo

Re: [PATCH v2 10/13] qdev: make properties array "const"

[Philippe Mathieu-Daudé]

On 22/10/24 02:23, Paolo Bonzini wrote:
> On Tue, Oct 22, 2024 at 6:31 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>> -void qdev_property_add_static(DeviceState *dev, Property *prop)
>>> +void qdev_property_add_static(DeviceState *dev, const Property *prop)
>>>    {
>>>        Object *obj = OBJECT(dev);
>>>        ObjectProperty *op;
>>> @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop)
>>>                                 field_prop_getter(prop->info),
>>>                                 field_prop_setter(prop->info),
>>>                                 prop->info->release,
>>> -                             prop);
>>> +                             (Property *)prop);
>>
>> I like the overall patch idea, but I'm not keen on casting
>> const to non-const. Should we adapt the callee -- here
>> object_property_add() -- to also take a const argument?
> 
> This argument goes into prop->opaque and is passed to all
> accessor/resolver/finalizers functions. So it would be a much larger
> change because it needs to change all those functions from "void
> *opaque" to "const void *opaque".
> 
> It would also be an issue because some finalizers write to the opaque
> for good reason:
> 
> static void object_finalize_child_property(
>      Object *obj, const char *name, void *opaque)
> {
>      Object *child = opaque;
> 
>      if (child->class->unparent) {
>          (child->class->unparent)(child);
>      }
>      child->parent = NULL; // <--- here
>      object_unref(child);
> }
> 
> So, it's not great but it seems necessary. At least keeping the const
> within qdev properties makes things "safer" within that realm.

Since it is only within qdev-properties.c, it is indeed reasonable to
accept. Maybe make it explicit via a well-named macro to do the cast?

   /* NON_CONST_PROP: Specific macro to this file because ... */
   #define NON_CONST_PROP(prop) (Property *)(prop)

Regards,

Phil.

Re: [PATCH v2 10/13] qdev: make properties array "const"

[Paolo Bonzini]

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

Il mar 22 ott 2024, 23:43 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:

> On 22/10/24 02:23, Paolo Bonzini wrote:
> > At least keeping the const
> > within qdev properties makes things "safer" within that realm.
>
> Since it is only within qdev-properties.c, it is indeed reasonable to
> accept. Maybe make it explicit via a well-named macro to do the cast?
>
>    /* NON_CONST_PROP: Specific macro to this file because ... */
>    #define NON_CONST_PROP(prop) (Property *)(prop)
>

The name of the macro suggests the opposite, i.e. that you'd use it to cast
const void* to Property* in the callbacks. I would leave it as it is.

Paolo

>
> Regards,
>
> Phil.
>
>

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

[PATCH v2 11/13] rust: make properties array immutable

[Paolo Bonzini]

Now that device_class_set_props() takes a const pointer, the only part of
"define_property!" that needs to be non-const is the call to try_into().
This in turn will only break if offset_of returns a value with the most
significant bit set (i.e. a struct size that is >=2^31 or >= 2^63,
respectively on 32- and 64-bit system), which is impossible.

Just use a cast and clean everything up to remove the run-time
initialization.  This also removes a use of OnceLock, which was only
stabilized in 1.70.0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/device_class.rs | 42 ++++++-------------------------
 1 file changed, 8 insertions(+), 34 deletions(-)

diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index 5aba426d243..d885f2fcf19 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -2,10 +2,6 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use std::sync::OnceLock;
-
-use crate::bindings::Property;
-
 #[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$(,)*) => {
@@ -19,7 +15,7 @@ macro_rules! device_class_init {
                 dc.as_mut().realize = $realize_fn;
                 dc.as_mut().vmsd = &$vmsd;
                 $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn);
-                $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr());
+                $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_ptr());
             }
         }
     };
@@ -32,9 +28,7 @@ macro_rules! define_property {
             // use associated function syntax for type checking
             name: ::core::ffi::CStr::as_ptr($name),
             info: $prop,
-            offset: ::core::mem::offset_of!($state, $field)
-                .try_into()
-                .expect("Could not fit offset value to type"),
+            offset: ::core::mem::offset_of!($state, $field) as isize,
             set_default: true,
             defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() },
             ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
@@ -45,47 +39,27 @@ macro_rules! define_property {
             // use associated function syntax for type checking
             name: ::core::ffi::CStr::as_ptr($name),
             info: $prop,
-            offset: ::core::mem::offset_of!($state, $field)
-                .try_into()
-                .expect("Could not fit offset value to type"),
+            offset: ::core::mem::offset_of!($state, $field) as isize,
             set_default: false,
             ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
         }
     };
 }
 
-#[repr(C)]
-pub struct Properties<const N: usize>(pub OnceLock<[Property; N]>, pub fn() -> [Property; N]);
-
-impl<const N: usize> Properties<N> {
-    pub fn as_mut_ptr(&mut self) -> *mut Property {
-        _ = self.0.get_or_init(self.1);
-        self.0.get_mut().unwrap().as_mut_ptr()
-    }
-}
-
 #[macro_export]
 macro_rules! declare_properties {
     ($ident:ident, $($prop:expr),*$(,)*) => {
-
-        const fn _calc_prop_len() -> usize {
+        pub static $ident: [$crate::bindings::Property; {
             let mut len = 1;
             $({
                 _ = stringify!($prop);
                 len += 1;
             })*
             len
-        }
-        const PROP_LEN: usize = _calc_prop_len();
-
-        fn _make_properties() -> [$crate::bindings::Property; PROP_LEN] {
-            [
-                $($prop),*,
-                    unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() },
-            ]
-        }
-
-        pub static mut $ident: $crate::device_class::Properties<PROP_LEN> = $crate::device_class::Properties(::std::sync::OnceLock::new(), _make_properties);
+        }] = [
+            $($prop),*,
+            unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() },
+        ];
     };
 }
 
-- 
2.46.2

Re: [PATCH v2 11/13] rust: make properties array immutable

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:36PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:36 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 11/13] rust: make properties array immutable
> X-Mailer: git-send-email 2.46.2
> 
> Now that device_class_set_props() takes a const pointer, the only part of
> "define_property!" that needs to be non-const is the call to try_into().
> This in turn will only break if offset_of returns a value with the most
> significant bit set (i.e. a struct size that is >=2^31 or >= 2^63,
> respectively on 32- and 64-bit system), which is impossible.
> 
> Just use a cast and clean everything up to remove the run-time
> initialization.  This also removes a use of OnceLock, which was only
> stabilized in 1.70.0.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/device_class.rs | 42 ++++++-------------------------
>  1 file changed, 8 insertions(+), 34 deletions(-)
> 

Very elegant!

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

[PATCH v2 12/13] rust: provide safe wrapper for MaybeUninit::zeroed()

[Paolo Bonzini]

MaybeUninit::zeroed() is handy, but it introduces unsafe (and has a
pretty heavy syntax in general).  Introduce a trait that provides the
same functionality while staying within safe Rust.

In addition, MaybeUninit::zeroed() is not available as a "const"
function until Rust 1.75.0, so this also prepares for having handwritten
implementations of the trait until we can assume that version.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device_class.rs |  8 ++++++--
 rust/hw/char/pl011/src/memory_ops.rs   | 11 +++++++----
 rust/qemu-api/meson.build              |  1 +
 rust/qemu-api/src/device_class.rs      |  8 ++++----
 rust/qemu-api/src/lib.rs               |  1 +
 rust/qemu-api/src/zeroable.rs          | 23 +++++++++++++++++++++++
 6 files changed, 42 insertions(+), 10 deletions(-)
 create mode 100644 rust/qemu-api/src/zeroable.rs

diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 2ad80451e87..78fa1cdd5b6 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -4,7 +4,11 @@
 
 use core::ptr::NonNull;
 
-use qemu_api::{bindings::*, definitions::ObjectImpl};
+use qemu_api::{
+    bindings::*,
+    definitions::ObjectImpl,
+    zeroable::Zeroable,
+};
 
 use crate::device::PL011State;
 
@@ -12,7 +16,7 @@
 pub static VMSTATE_PL011: VMStateDescription = VMStateDescription {
     name: PL011State::TYPE_INFO.name,
     unmigratable: true,
-    ..unsafe { ::core::mem::MaybeUninit::<VMStateDescription>::zeroed().assume_init() }
+    ..Zeroable::ZERO
 };
 
 qemu_api::declare_properties! {
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
index 5a5320e66c3..24ac9c870c1 100644
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ b/rust/hw/char/pl011/src/memory_ops.rs
@@ -2,9 +2,12 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use core::{mem::MaybeUninit, ptr::NonNull};
+use core::ptr::NonNull;
 
-use qemu_api::bindings::*;
+use qemu_api::{
+    bindings::*,
+    zeroable::Zeroable
+};
 
 use crate::device::PL011State;
 
@@ -14,11 +17,11 @@
     read_with_attrs: None,
     write_with_attrs: None,
     endianness: device_endian::DEVICE_NATIVE_ENDIAN,
-    valid: unsafe { MaybeUninit::<MemoryRegionOps__bindgen_ty_1>::zeroed().assume_init() },
+    valid: Zeroable::ZERO,
     impl_: MemoryRegionOps__bindgen_ty_2 {
         min_access_size: 4,
         max_access_size: 4,
-        ..unsafe { MaybeUninit::<MemoryRegionOps__bindgen_ty_2>::zeroed().assume_init() }
+        ..Zeroable::ZERO
     },
 };
 
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 1fc36078027..1b0fd406378 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/zeroable.rs',
     ],
     {'.' : bindings_rs},
   ),
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index d885f2fcf19..ed2d7ce1a54 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -31,7 +31,7 @@ macro_rules! define_property {
             offset: ::core::mem::offset_of!($state, $field) as isize,
             set_default: true,
             defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() },
-            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
+            ..$crate::zeroable::Zeroable::ZERO
         }
     };
     ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => {
@@ -41,7 +41,7 @@ macro_rules! define_property {
             info: $prop,
             offset: ::core::mem::offset_of!($state, $field) as isize,
             set_default: false,
-            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }
+            ..$crate::zeroable::Zeroable::ZERO
         }
     };
 }
@@ -58,7 +58,7 @@ macro_rules! declare_properties {
             len
         }] = [
             $($prop),*,
-            unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() },
+            $crate::zeroable::Zeroable::ZERO,
         ];
     };
 }
@@ -79,7 +79,7 @@ macro_rules! vm_state_description {
                 $vname.as_ptr()
             },)*
             unmigratable: true,
-            ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::VMStateDescription>::zeroed().assume_init() }
+            ..$crate::zeroable::Zeroable::ZERO
         };
     }
 }
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 6bc68076aae..e94a15bb823 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 {}
 
 pub mod definitions;
 pub mod device_class;
+pub mod zeroable;
 
 use std::alloc::{GlobalAlloc, Layout};
 
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
new file mode 100644
index 00000000000..45ec95c9f70
--- /dev/null
+++ b/rust/qemu-api/src/zeroable.rs
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/// Encapsulates the requirement that
+/// `MaybeUninit::<Self>::zeroed().assume_init()` does not cause
+/// undefined behavior.
+///
+/// # 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<>`.
+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() };
+}
+
+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 {}
-- 
2.46.2

Re: [PATCH v2 12/13] rust: provide safe wrapper for MaybeUninit::zeroed()

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:37PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:37 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 12/13] rust: provide safe wrapper for
>  MaybeUninit::zeroed()
> X-Mailer: git-send-email 2.46.2
> 
> MaybeUninit::zeroed() is handy, but it introduces unsafe (and has a
> pretty heavy syntax in general).  Introduce a trait that provides the
> same functionality while staying within safe Rust.
> 
> In addition, MaybeUninit::zeroed() is not available as a "const"
> function until Rust 1.75.0, so this also prepares for having handwritten
> implementations of the trait until we can assume that version.
> 
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device_class.rs |  8 ++++++--
>  rust/hw/char/pl011/src/memory_ops.rs   | 11 +++++++----
>  rust/qemu-api/meson.build              |  1 +
>  rust/qemu-api/src/device_class.rs      |  8 ++++----
>  rust/qemu-api/src/lib.rs               |  1 +
>  rust/qemu-api/src/zeroable.rs          | 23 +++++++++++++++++++++++
>  6 files changed, 42 insertions(+), 10 deletions(-)
>  create mode 100644 rust/qemu-api/src/zeroable.rs

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

[PATCH v2 13/13] rust: do not use TYPE_CHARDEV unnecessarily

[Paolo Bonzini]

In the invocation of qdev_prop_set_chr(), "chardev" is the name of a
property rather than a type and has to match the name of the property
in device_class.rs.  Do not use TYPE_CHARDEV here, just like in the C
version of pl011_create.

Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2b43f5e0939..0f6918dd224 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -572,7 +572,7 @@ pub fn update(&self) {
         let dev: *mut DeviceState = qdev_new(PL011State::TYPE_INFO.name);
         let sysbus: *mut SysBusDevice = dev.cast::<SysBusDevice>();
 
-        qdev_prop_set_chr(dev, bindings::TYPE_CHARDEV.as_ptr(), chr);
+        qdev_prop_set_chr(dev, c"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);
-- 
2.46.2

Re: [PATCH v2 13/13] rust: do not use TYPE_CHARDEV unnecessarily

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:38PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:38 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 13/13] rust: do not use TYPE_CHARDEV unnecessarily
> X-Mailer: git-send-email 2.46.2
> 
> In the invocation of qdev_prop_set_chr(), "chardev" is the name of a
> property rather than a type and has to match the name of the property
> in device_class.rs.  Do not use TYPE_CHARDEV here, just like in the C
> version of pl011_create.
> 
> Reviewed-by: Junjie Mao <junjie.mao@hotmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v2 00/13] rust: miscellaneous cleanups + QOM integration tests

[Kevin Wolf]

Am 21.10.2024 um 18:35 hat Paolo Bonzini geschrieben:
> This series integrates some of the observations from the MSRV patches at
> https://lore.kernel.org/qemu-devel/20241015131735.518771-1-pbonzini@redhat.com/.
> 
> The main changes here are two: first, build an integration test that
> actually tries to create a QOM object that is defined by Rust code;
> second, make the properties array immutable so that declare_properties!
> is enforced to use only const-friendly constructs.  These are patches
> 6-11; the others consist of small cleanups.
> 
> Hidden in here is actually a C patch (#10) which makes the
> bindgen-generated prototypes use "*const" instead of "*mut".
> 
> Tested with Rust nightly and (together with more patches from the
> RFC), with Rust 1.63.0.
> 
> Unlike the MSRV patches, this should be ready for inclusion; the
> changes should be mostly uncontroversial.

I'm not convinced that Zeroable has sufficient justification when all it
does is saving us a few lines of code at the expense of making things
more implicit. But it's used correctly as far as I can tell, so:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH v2 00/13] rust: miscellaneous cleanups + QOM integration tests

[Paolo Bonzini]

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

Il mar 22 ott 2024, 22:46 Kevin Wolf <kwolf@redhat.com> ha scritto:

> Am 21.10.2024 um 18:35 hat Paolo Bonzini geschrieben:
> > This series integrates some of the observations from the MSRV patches at
> >
> https://lore.kernel.org/qemu-devel/20241015131735.518771-1-pbonzini@redhat.com/
> .
> >
> > The main changes here are two: first, build an integration test that
> > actually tries to create a QOM object that is defined by Rust code;
> > second, make the properties array immutable so that declare_properties!
> > is enforced to use only const-friendly constructs.  These are patches
> > 6-11; the others consist of small cleanups.
> >
> > Hidden in here is actually a C patch (#10) which makes the
> > bindgen-generated prototypes use "*const" instead of "*mut".
> >
> > Tested with Rust nightly and (together with more patches from the
> > RFC), with Rust 1.63.0.
> >
> > Unlike the MSRV patches, this should be ready for inclusion; the
> > changes should be mostly uncontroversial.
>
> I'm not convinced that Zeroable has sufficient justification when all it
> does is saving us a few lines of code at the expense of making things
> more implicit


Implicit zero fields are used a lot in C code, including in
VMStateDescription and various const vtable structs, so I think we need
something of equivalent brevity.

There could be other solutions, for example a more generic
ConstDefault::DEFAULT trait. For now I went for something that resembles
existing code, as well as the C code we're converting.

. But it's used correctly as far as I can tell, so:
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>

Thanks!

Paolo


>

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