| Message ID | 20241205060714.256270-7-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Reinvent the wheel for HPET timer in Rust | expand |
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~
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
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~
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 <richard.henderson@linaro.org> > 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
On Thu, 5 Dec 2024 at 18:30, Paolo Bonzini <pbonzini@redhat.com> 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
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~
Il ven 6 dic 2024, 05:59 Peter Maydell <peter.maydell@linaro.org> 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 > >
On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > Il ven 6 dic 2024, 05:59 Peter Maydell <peter.maydell@linaro.org> 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
Il ven 6 dic 2024, 09:42 Peter Maydell <peter.maydell@linaro.org> ha scritto: > On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini <pbonzini@redhat.com> 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 > >
On 6/12/24 11:59, Peter Maydell wrote: > On Thu, 5 Dec 2024 at 18:30, Paolo Bonzini <pbonzini@redhat.com> 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));
Il sab 7 dic 2024, 10:21 Philippe Mathieu-Daudé <philmd@linaro.org> 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 >
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 <pbonzini@redhat.com> > Subject: Re: [RFC 06/13] rust: add bindings for memattrs > > Il sab 7 dic 2024, 10:21 Philippe Mathieu-Daudé <philmd@linaro.org> 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
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 000000000000..7cc8aea4b7b7 --- /dev/null +++ b/rust/qemu-api/src/memattrs.rs @@ -0,0 +1,21 @@ +// Copyright (C) 2024 Intel Corporation. +// Author(s): Zhao Liu <zhai1.liu@intel.com> +// 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 + } +} + +/// Bus masters which don't specify any attributes will get this, +/// which has all attribute bits clear except the topmost one +/// (so that we can distinguish "all attributes deliberately clear" +/// from "didn't specify" if necessary). +pub static MEMTXATTRS_UNSPECIFIED: Lazy<MemTxAttrs> = Lazy::new(MemTxAttrs::memtxattrs_unspecified); diff --git a/rust/wrapper.h b/rust/wrapper.h index 285d0eb6ad01..033f3e9cf32c 100644 --- a/rust/wrapper.h +++ b/rust/wrapper.h @@ -62,3 +62,4 @@ typedef enum memory_order { #include "qapi/error.h" #include "migration/vmstate.h" #include "chardev/char-serial.h" +#include "exec/memattrs.h" diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh index 30677c3ec903..59e72d92498a 100755 --- a/scripts/archive-source.sh +++ b/scripts/archive-source.sh @@ -30,7 +30,7 @@ subprojects="keycodemapdb libvfio-user berkeley-softfloat-3 berkeley-testfloat-3 arbitrary-int-1-rs bilge-0.2-rs bilge-impl-0.2-rs either-1-rs itertools-0.11-rs proc-macro2-1-rs proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs - syn-2-rs unicode-ident-1-rs" + syn-2-rs unicode-ident-1-rs once_cell-1.20-rs" sub_deinit="" function cleanup() { diff --git a/scripts/make-release b/scripts/make-release index 8dc939124c4f..b6b48bdf6f08 100755 --- a/scripts/make-release +++ b/scripts/make-release @@ -21,7 +21,7 @@ SUBPROJECTS="libvfio-user keycodemapdb berkeley-softfloat-3 berkeley-testfloat-3 arbitrary-int-1-rs bilge-0.2-rs bilge-impl-0.2-rs either-1-rs itertools-0.11-rs proc-macro2-1-rs proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs - syn-2-rs unicode-ident-1-rs" + syn-2-rs unicode-ident-1-rs once_cell-1.20-rs" src="$1" version="$2" diff --git a/subprojects/.gitignore b/subprojects/.gitignore index 50f173f90dbe..dba8ea74b823 100644 --- a/subprojects/.gitignore +++ b/subprojects/.gitignore @@ -11,6 +11,7 @@ /bilge-impl-0.2.0 /either-1.12.0 /itertools-0.11.0 +/once_cell-1.20.2 /proc-macro-error-1.0.4 /proc-macro-error-attr-1.0.4 /proc-macro2-1.0.84 diff --git a/subprojects/once_cell-1.20-rs.wrap b/subprojects/once_cell-1.20-rs.wrap new file mode 100644 index 000000000000..b13ba81a22b1 --- /dev/null +++ b/subprojects/once_cell-1.20-rs.wrap @@ -0,0 +1,7 @@ +[wrap-file] +directory = once_cell-1.20.2 +source_url = https://crates.io/api/v1/crates/once_cell/1.20.2/download +source_filename = once_cell-1.20.2.tar.gz +source_hash = 1261fe7e33c73b354eab43b1273a57c8f967d0391e80353e51f764ac02cf6775 +#method = cargo +patch_directory = once_cell-1.20-rs diff --git a/subprojects/packagefiles/once_cell-1.20-rs/meson.build b/subprojects/packagefiles/once_cell-1.20-rs/meson.build new file mode 100644 index 000000000000..0b81f8e76250 --- /dev/null +++ b/subprojects/packagefiles/once_cell-1.20-rs/meson.build @@ -0,0 +1,23 @@ +project('once_cell-1.20-rs', 'rust', + version: '1.20.2', + license: 'MIT OR Apache-2.0', + default_options: []) + +_once_cell_rs = static_library( + 'once_cell', + files('src/lib.rs'), + gnu_symbol_visibility: 'hidden', + override_options: ['rust_std=2021', 'build.rust_std=2021'], + rust_abi: 'rust', + rust_args: [ + '--cfg', 'feature="std"' + ], + dependencies: [], + native: true, +) + +once_cell_dep = declare_dependency( + link_with: _once_cell_rs, +) + +meson.override_dependency('once_cell-1.20-rs', once_cell_dep, native: true)
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 <zhao1.liu@intel.com> --- 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