Re: [RFC 06/13] rust: add bindings for memattrs
On 1/23/25 16:10, Zhao Liu wrote: Another solution would be to implement Zeroable for __BindgenBitfieldUnit in bindings.rs, but this is much nicer! It works even with old Rust versions and, even though it needs manual implementation of the trait each type, it doesn't require enumerating the fields one by one. So it's better than the current version of Zeroable and, if you wish, you can also replace existing implementations of Zeroable with const_zero. I'm working on this, and it's just a simple patch. But I'm not sure why Zeroable needs a Default constraint. I think Sized seems to be enough, doesn't it? It's not needed but I think it makes sense semantically: 1) the idea was that Default::default() would always return the same value as Zeroable::ZERO, apart possibly from padding. That is, for something that's Zeroable it should always make sense to use an all-zero value for Default::default(). And since there are standard library methods that use Default, the bound makes it possible to use them and know that they fill-with-zeroes. 2) I wasn't sure in which cases bindgen doesn't generate Default, if any; and with Default as a supertrait we'd have to double-check the first occurrence of a non-Default but zeroable struct. Paolo
Re: [RFC 06/13] rust: add bindings for memattrs
> Another solution would be to implement Zeroable for __BindgenBitfieldUnit in > bindings.rs, but this is much nicer! It works even with old Rust versions > and, even though it needs manual implementation of the trait each type, it > doesn't require enumerating the fields one by one. So it's better than the > current version of Zeroable and, if you wish, you can also replace existing > implementations of Zeroable with const_zero. I'm working on this, and it's just a simple patch. But I'm not sure why Zeroable needs a Default constraint. I think Sized seems to be enough, doesn't it? Thanks, Zhao
Re: [RFC 06/13] rust: add bindings for memattrs
On 1/20/25 17:52, Zhao Liu wrote: Sorry I missed this comment before... Now I have a MemTxAttrs like, typedef struct MemTxAttrs { unsigned int secure:1; unsigned int space:2; unsigned int user:1; unsigned int memory:1; unsigned int requester_id:16; unsigned int pid:8; bool unspecified; uint8_t _reserved1; uint16_t _reserved2; } MemTxAttrs; and its binding is, #[repr(C)] #[repr(align(4))] #[derive(Debug, Default, Copy, Clone)] pub struct MemTxAttrs { pub _bitfield_align_1: [u16; 0], pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize]>, pub unspecified: bool, pub _reserved1: u8, pub _reserved2: u16, } unfortunately, Zeroable can't be applied to __BindgenBitfieldUnit since event its member (`storage`) is private :-(. But there's a solution to force (and at the same time unsafely) ZERO the entire structure in const: * const_zero macro: https://docs.rs/const-zero/latest/const_zero/ With const_zero, we can implement Zeroable for MemTxAttrs: unsafe impl Zeroable for MemTxAttrs { const ZERO: Self = unsafe {const_zero!(MemTxAttrs)}; } Another solution would be to implement Zeroable for __BindgenBitfieldUnit in bindings.rs, but this is much nicer! It works even with old Rust versions and, even though it needs manual implementation of the trait each type, it doesn't require enumerating the fields one by one. So it's better than the current version of Zeroable and, if you wish, you can also replace existing implementations of Zeroable with const_zero. I wouldn't bother adding a subproject; just include the macro in zeroable.rs, with attribution and MIT license, and document that it might go away once we require a newer rustc. Thanks very much! Paolo
Re: [RFC 06/13] rust: add bindings for memattrs
On Fri, Dec 06, 2024 at 02:13:57PM -0500, Paolo Bonzini wrote: > Date: Fri, 6 Dec 2024 14:13:57 -0500 > From: Paolo Bonzini > Subject: Re: [RFC 06/13] rust: add bindings for memattrs > > Il ven 6 dic 2024, 09:42 Peter Maydell ha > scritto: > > > On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini wrote: > > > Yes, hence "decently" packed. But I think in both cases it's passed in > > registers, and for 64-bit machine that shouldn't change anything. > > > > True. Though it does mean we go from "space to add new fields > > without making it overflow from one register to two" to > > "completely full and no space for expanding it". > > > > I guess it's enough to make unspecified the only non-bitfield. Then you can > declare MEMTXATTRS_UNSPECIFIED as { unspecified: true, ..Zeroable::ZERO } > ("start with Zeroable::ZERO and change unspecified to true"). For the rest > it is not important to make them available in a "const". > Sorry I missed this comment before... Now I have a MemTxAttrs like, typedef struct MemTxAttrs { unsigned int secure:1; unsigned int space:2; unsigned int user:1; unsigned int memory:1; unsigned int requester_id:16; unsigned int pid:8; bool unspecified; uint8_t _reserved1; uint16_t _reserved2; } MemTxAttrs; and its binding is, #[repr(C)] #[repr(align(4))] #[derive(Debug, Default, Copy, Clone)] pub struct MemTxAttrs { pub _bitfield_align_1: [u16; 0], pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize]>, pub unspecified: bool, pub _reserved1: u8, pub _reserved2: u16, } unfortunately, Zeroable can't be applied to __BindgenBitfieldUnit since event its member (`storage`) is private :-(. But there's a solution to force (and at the same time unsafely) ZERO the entire structure in const: * const_zero macro: https://docs.rs/const-zero/latest/const_zero/ With const_zero, we can implement Zeroable for MemTxAttrs: unsafe impl Zeroable for MemTxAttrs { const ZERO: Self = unsafe {const_zero!(MemTxAttrs)}; } pub static MEMTXATTRS_UNSPECIFIED: MemTxAttrs = MemTxAttrs { unspecified: true, ..Zeroable::ZERO }; So do you like this idea? If so, I can be a mover to introduce const_zero macro. Thanks, Zhao
Re: [RFC 06/13] rust: add bindings for memattrs
On Sun, Dec 08, 2024 at 10:30:34AM +0100, Paolo Bonzini wrote: > Date: Sun, 8 Dec 2024 10:30:34 +0100 > From: Paolo Bonzini > Subject: Re: [RFC 06/13] rust: add bindings for memattrs > > Il sab 7 dic 2024, 10:21 Philippe Mathieu-Daudé ha > scritto: > > > >> is still decently packed and simplifies things a lot. > > > > > > The old struct is 4 bytes, and the new one is 8 bytes. We do > > > a lot of directly passing 'struct MemTxAttrs' arguments around > > > as arguments to functions (i.e. not passing pointers to them), > > > so increasing it in size is not completely free. > > > > Should we add a check to not pass 8 bytes? > > > >QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) != sizeof(uint64_t)); > > > > Yes, why not. > Thank you all! Will also include this in the followup clean-up patches. Regards, Zhao
Re: [RFC 06/13] rust: add bindings for memattrs
Il sab 7 dic 2024, 10:21 Philippe Mathieu-Daudé ha scritto: > >> is still decently packed and simplifies things a lot. > > > > The old struct is 4 bytes, and the new one is 8 bytes. We do > > a lot of directly passing 'struct MemTxAttrs' arguments around > > as arguments to functions (i.e. not passing pointers to them), > > so increasing it in size is not completely free. > > Should we add a check to not pass 8 bytes? > >QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) != sizeof(uint64_t)); > Yes, why not. Paolo >
Re: [RFC 06/13] rust: add bindings for memattrs
On 6/12/24 11:59, Peter Maydell wrote: On Thu, 5 Dec 2024 at 18:30, Paolo Bonzini wrote: On 12/5/24 19:15, Richard Henderson wrote: On 12/5/24 00:07, Zhao Liu wrote: The MemTxAttrs structure is composed of bitfield members, and bindgen is unable to generate an equivalent macro definition for MEMTXATTRS_UNSPECIFIED. I'm happy to move away from bit fields to uint32_t or suchlike to enable MEMTXATTRS_UNSPECIFIED be a compile-time constant. Yeah, if we go from typedef struct MemTxAttrs { unsigned int unspecified:1; unsigned int secure:1; unsigned int space:2; unsigned int user:1; unsigned int memory:1; unsigned int requester_id:16; unsigned int pid:8; } MemTxAttrs; to typedef struct MemTxAttrs { uint8_t unspecified; uint8_t secure; uint8_t space; uint8_t user; uint8_t memory; uint8_t pid; uint16_t requester_id; } MemTxAttrs; is still decently packed and simplifies things a lot. The old struct is 4 bytes, and the new one is 8 bytes. We do a lot of directly passing 'struct MemTxAttrs' arguments around as arguments to functions (i.e. not passing pointers to them), so increasing it in size is not completely free. Should we add a check to not pass 8 bytes? QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) != sizeof(uint64_t));
Re: [RFC 06/13] rust: add bindings for memattrs
Il ven 6 dic 2024, 09:42 Peter Maydell ha scritto: > On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini wrote: > > Yes, hence "decently" packed. But I think in both cases it's passed in > registers, and for 64-bit machine that shouldn't change anything. > > True. Though it does mean we go from "space to add new fields > without making it overflow from one register to two" to > "completely full and no space for expanding it". > I guess it's enough to make unspecified the only non-bitfield. Then you can declare MEMTXATTRS_UNSPECIFIED as { unspecified: true, ..Zeroable::ZERO } ("start with Zeroable::ZERO and change unspecified to true"). For the rest it is not important to make them available in a "const". Paolo -- PMM > >
Re: [RFC 06/13] rust: add bindings for memattrs
On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini wrote: > > > > Il ven 6 dic 2024, 05:59 Peter Maydell ha scritto: >> >> > is still decently packed and simplifies things a lot. >> >> The old struct is 4 bytes, and the new one is 8 bytes. > > > Yes, hence "decently" packed. But I think in both cases it's passed in > registers, and for 64-bit machine that shouldn't change anything. True. Though it does mean we go from "space to add new fields without making it overflow from one register to two" to "completely full and no space for expanding it". -- PMM
Re: [RFC 06/13] rust: add bindings for memattrs
Il ven 6 dic 2024, 05:59 Peter Maydell ha scritto: > > is still decently packed and simplifies things a lot. > > The old struct is 4 bytes, and the new one is 8 bytes. Yes, hence "decently" packed. But I think in both cases it's passed in registers, and for 64-bit machine that shouldn't change anything. Paolo We do > a lot of directly passing 'struct MemTxAttrs' arguments around > as arguments to functions (i.e. not passing pointers to them), > so increasing it in size is not completely free. > > thanks > -- PMM > >
Re: [RFC 06/13] rust: add bindings for memattrs
On 12/6/24 02:41, Zhao Liu wrote: BTW, may I ask why you favor bool over uint8_t, and is it because they are indeed just 0/1 as well? :) Yes, exactly. :-) r~
Re: [RFC 06/13] rust: add bindings for memattrs
On Thu, 5 Dec 2024 at 18:30, Paolo Bonzini wrote: > > On 12/5/24 19:15, Richard Henderson wrote: > > On 12/5/24 00:07, Zhao Liu wrote: > >> The MemTxAttrs structure is composed of bitfield members, and bindgen is > >> unable to generate an equivalent macro definition for > >> MEMTXATTRS_UNSPECIFIED. > > > > I'm happy to move away from bit fields to uint32_t or suchlike to enable > > MEMTXATTRS_UNSPECIFIED be a compile-time constant. > > Yeah, if we go from > > typedef struct MemTxAttrs { > unsigned int unspecified:1; > unsigned int secure:1; > unsigned int space:2; > unsigned int user:1; > unsigned int memory:1; > unsigned int requester_id:16; > unsigned int pid:8; > } MemTxAttrs; > > to > > typedef struct MemTxAttrs { > uint8_t unspecified; > uint8_t secure; > uint8_t space; > uint8_t user; > uint8_t memory; > uint8_t pid; > uint16_t requester_id; > } MemTxAttrs; > > is still decently packed and simplifies things a lot. The old struct is 4 bytes, and the new one is 8 bytes. We do a lot of directly passing 'struct MemTxAttrs' arguments around as arguments to functions (i.e. not passing pointers to them), so increasing it in size is not completely free. thanks -- PMM
Re: [RFC 06/13] rust: add bindings for memattrs
On Thu, Dec 05, 2024 at 05:51:42PM -0600, Richard Henderson wrote: > Date: Thu, 5 Dec 2024 17:51:42 -0600 > From: Richard Henderson > Subject: Re: [RFC 06/13] rust: add bindings for memattrs > > On 12/5/24 12:30, Paolo Bonzini wrote: > > On 12/5/24 19:15, Richard Henderson wrote: > > > On 12/5/24 00:07, Zhao Liu wrote: > > > > The MemTxAttrs structure is composed of bitfield members, and bindgen is > > > > unable to generate an equivalent macro definition for > > > > MEMTXATTRS_UNSPECIFIED. > > > > > > I'm happy to move away from bit fields to uint32_t or suchlike to > > > enable MEMTXATTRS_UNSPECIFIED be a compile-time constant. > > > > Yeah, if we go from > > > > typedef struct MemTxAttrs { > > unsigned int unspecified:1; > > unsigned int secure:1; > > unsigned int space:2; > > unsigned int user:1; > > unsigned int memory:1; > > unsigned int requester_id:16; > > unsigned int pid:8; > > } MemTxAttrs; > > > > to > > > > typedef struct MemTxAttrs { > > uint8_t unspecified; > > uint8_t secure; > > uint8_t space; > > uint8_t user; > > uint8_t memory; > > uint8_t pid; > > uint16_t requester_id; > > } MemTxAttrs; > > > > is still decently packed and simplifies things a lot. Zhao, can you > > submit that as an independent patch? > Hmm. I'd been thinking of uint32_t and hw/registerfields.h, but I have not > scoped the size of that conversion. > > But short of that, please use 'bool' not 'uint8_t' for those single-bit flags. > Sure! Thank u both! BTW, may I ask why you favor bool over uint8_t, and is it because they are indeed just 0/1 as well? :) Thanks, Zhao
Re: [RFC 06/13] rust: add bindings for memattrs
On 12/5/24 12:30, Paolo Bonzini wrote: On 12/5/24 19:15, Richard Henderson wrote: On 12/5/24 00:07, Zhao Liu wrote: The MemTxAttrs structure is composed of bitfield members, and bindgen is unable to generate an equivalent macro definition for MEMTXATTRS_UNSPECIFIED. I'm happy to move away from bit fields to uint32_t or suchlike to enable MEMTXATTRS_UNSPECIFIED be a compile-time constant. Yeah, if we go from typedef struct MemTxAttrs { unsigned int unspecified:1; unsigned int secure:1; unsigned int space:2; unsigned int user:1; unsigned int memory:1; unsigned int requester_id:16; unsigned int pid:8; } MemTxAttrs; to typedef struct MemTxAttrs { uint8_t unspecified; uint8_t secure; uint8_t space; uint8_t user; uint8_t memory; uint8_t pid; uint16_t requester_id; } MemTxAttrs; is still decently packed and simplifies things a lot. Zhao, can you submit that as an independent patch? Hmm. I'd been thinking of uint32_t and hw/registerfields.h, but I have not scoped the size of that conversion. But short of that, please use 'bool' not 'uint8_t' for those single-bit flags. r~
Re: [RFC 06/13] rust: add bindings for memattrs
On 12/5/24 19:15, Richard Henderson wrote: On 12/5/24 00:07, Zhao Liu wrote: The MemTxAttrs structure is composed of bitfield members, and bindgen is unable to generate an equivalent macro definition for MEMTXATTRS_UNSPECIFIED. I'm happy to move away from bit fields to uint32_t or suchlike to enable MEMTXATTRS_UNSPECIFIED be a compile-time constant. Yeah, if we go from typedef struct MemTxAttrs { unsigned int unspecified:1; unsigned int secure:1; unsigned int space:2; unsigned int user:1; unsigned int memory:1; unsigned int requester_id:16; unsigned int pid:8; } MemTxAttrs; to typedef struct MemTxAttrs { uint8_t unspecified; uint8_t secure; uint8_t space; uint8_t user; uint8_t memory; uint8_t pid; uint16_t requester_id; } MemTxAttrs; is still decently packed and simplifies things a lot. Zhao, can you submit that as an independent patch? Paolo
Re: [RFC 06/13] rust: add bindings for memattrs
On 12/5/24 00:07, Zhao Liu wrote: The MemTxAttrs structure is composed of bitfield members, and bindgen is unable to generate an equivalent macro definition for MEMTXATTRS_UNSPECIFIED. I'm happy to move away from bit fields to uint32_t or suchlike to enable MEMTXATTRS_UNSPECIFIED be a compile-time constant. r~
[RFC 06/13] rust: add bindings for memattrs
The MemTxAttrs structure is composed of bitfield members, and bindgen is unable to generate an equivalent macro definition for MEMTXATTRS_UNSPECIFIED. Therefore, we have to manually define a global constant variable MEMTXATTRS_UNSPECIFIED to support calls from Rust code. However, the binding methods of MemTxAttrs are non-const, so we cannot directly use them when defining MEMTXATTRS_UNSPECIFIED. As a result, add the third-party crate once_cell to use its Lazy to help define MEMTXATTRS_UNSPECIFIED. Note, lazy_static has been deprecated and LazyCell (in std) became stable since v1.80. When the minimum supported rustc version is bumped to v1.80 in the future, LazyCell can be used to replace the current once_cell. Signed-off-by: Zhao Liu --- rust/Cargo.lock | 7 ++ rust/qemu-api/Cargo.toml | 1 + rust/qemu-api/meson.build | 9 ++-- rust/qemu-api/src/lib.rs | 1 + rust/qemu-api/src/memattrs.rs | 21 + rust/wrapper.h| 1 + scripts/archive-source.sh | 2 +- scripts/make-release | 2 +- subprojects/.gitignore| 1 + subprojects/once_cell-1.20-rs.wrap| 7 ++ .../once_cell-1.20-rs/meson.build | 23 +++ 11 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 rust/qemu-api/src/memattrs.rs create mode 100644 subprojects/once_cell-1.20-rs.wrap create mode 100644 subprojects/packagefiles/once_cell-1.20-rs/meson.build diff --git a/rust/Cargo.lock b/rust/Cargo.lock index c0c6069247a8..6b19553b6d10 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -46,6 +46,12 @@ dependencies = [ "either", ] +[[package]] +name = "once_cell" +version = "1.20.2" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "1261fe7e33c73b354eab43b1273a57c8f967d0391e80353e51f764ac02cf6775" + [[package]] name = "pl011" version = "0.1.0" @@ -92,6 +98,7 @@ dependencies = [ name = "qemu_api" version = "0.1.0" dependencies = [ + "once_cell", "qemu_api_macros", "version_check", ] diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml index 4aa22f319860..265e00f97176 100644 --- a/rust/qemu-api/Cargo.toml +++ b/rust/qemu-api/Cargo.toml @@ -14,6 +14,7 @@ keywords = [] categories = [] [dependencies] +once_cell = { version = "1.20.2" } qemu_api_macros = { path = "../qemu-api-macros" } [build-dependencies] diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 00e86a679d8a..508986948883 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -10,6 +10,9 @@ if get_option('debug_mutex') _qemu_api_cfg += ['--feature', 'debug_cell'] endif +subproject('once_cell-1.20-rs', required: true) +once_cell_dep = dependency('once_cell-1.20-rs') + _qemu_api_rs = static_library( 'qemu_api', structured_sources( @@ -20,6 +23,7 @@ _qemu_api_rs = static_library( 'src/cell.rs', 'src/c_str.rs', 'src/irq.rs', + 'src/memattrs.rs', 'src/module.rs', 'src/offset_of.rs', 'src/qdev.rs', @@ -33,6 +37,7 @@ _qemu_api_rs = static_library( override_options: ['rust_std=2021', 'build.rust_std=2021'], rust_abi: 'rust', rust_args: _qemu_api_cfg, + dependencies: once_cell_dep, ) rust.test('rust-qemu-api-tests', _qemu_api_rs, @@ -40,7 +45,7 @@ rust.test('rust-qemu-api-tests', _qemu_api_rs, qemu_api = declare_dependency( link_with: _qemu_api_rs, - dependencies: qemu_api_macros, + dependencies: [qemu_api_macros, once_cell_dep], ) # Rust executables do not support objects, so add an intermediate step. @@ -56,7 +61,7 @@ test('rust-qemu-api-integration', override_options: ['rust_std=2021', 'build.rust_std=2021'], rust_args: ['--test'], install: false, -dependencies: [qemu_api, qemu_api_macros], +dependencies: [qemu_api, qemu_api_macros, once_cell_dep], link_whole: [rust_qemu_api_objs, libqemuutil]), args: [ '--test', diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 009906c907e7..e60c9ac16409 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -11,6 +11,7 @@ pub mod c_str; pub mod cell; pub mod irq; +pub mod memattrs; pub mod module; pub mod offset_of; pub mod qdev; diff --git a/rust/qemu-api/src/memattrs.rs b/rust/qemu-api/src/memattrs.rs new file mode 100644 index ..7cc8aea4b7b7 --- /dev/null +++ b/rust/qemu-api/src/memattrs.rs @@ -0,0 +1,21 @@ +// Copyright (C) 2024 Intel Corporation. +// Author(s): Zhao Liu +// SPDX-License-Identifier: GPL-2.0-or-later + +use once_cell::sync::Lazy; + +use crate::bindings::MemTxAttrs; + +impl MemTxAttrs { +fn memtxattrs_unspecified() -> Self { +let mut attrs = MemTxAttrs::default(); +attrs.set_unspecified(1); +attrs +} +} + +/// Bu