* [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 10:01 ` Paolo Bonzini
2025-02-11 21:43 ` [PATCH 02/11] meson: Add rust_block_ss and link tools with it Kevin Wolf
` (9 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
The existing qemu_api library can't be linked into tools because it
contains a few bindings for things that only exist in the system
emulator.
This adds a new "system" feature to the qemu_api crate that enables the
system emulator parts in it, and build the crate twice: qemu_api_tools
is the core library that can be linked into tools, and qemu_api_system
is the full one for the system emulator.
Unfortunately, since library names have to be unique in meson, this
means that every user of the library now needs to specify a
rust_dependency_map to make either qemu_api_tools or qemu_api_system
show up as the qemu_api crate in Rust.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/wrapper-system.h | 44 +++++++++++++++++++++
rust/wrapper.h | 9 -----
meson.build | 11 +++++-
rust/hw/char/pl011/meson.build | 3 +-
rust/meson.build | 11 +++---
rust/qemu-api/Cargo.toml | 1 +
rust/qemu-api/build.rs | 10 ++++-
rust/qemu-api/meson.build | 70 ++++++++++++++++++++++------------
rust/qemu-api/src/bindings.rs | 16 ++++++--
rust/qemu-api/src/lib.rs | 4 ++
rust/qemu-api/src/prelude.rs | 2 +
rust/qemu-api/src/zeroable.rs | 10 +++++
12 files changed, 143 insertions(+), 48 deletions(-)
create mode 100644 rust/wrapper-system.h
diff --git a/rust/wrapper-system.h b/rust/wrapper-system.h
new file mode 100644
index 0000000000..fc6c571e6d
--- /dev/null
+++ b/rust/wrapper-system.h
@@ -0,0 +1,44 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2024 Linaro Ltd.
+ *
+ * Authors: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+
+/*
+ * This header file is meant to be used as input to the `bindgen` application
+ * in order to generate C FFI compatible Rust bindings.
+ */
+
+/* The system emulator has all of the bindings tools have */
+#include "wrapper.h"
+
+#include "system/system.h"
+#include "hw/sysbus.h"
+#include "exec/memory.h"
+#include "hw/clock.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
diff --git a/rust/wrapper.h b/rust/wrapper.h
index a9bc67af0d..41be87adcf 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -50,15 +50,6 @@ typedef enum memory_order {
#include "qemu/osdep.h"
#include "qemu/module.h"
#include "qemu-io.h"
-#include "system/system.h"
-#include "hw/sysbus.h"
-#include "exec/memory.h"
#include "chardev/char-fe.h"
-#include "hw/clock.h"
-#include "hw/qdev-clock.h"
-#include "hw/qdev-properties.h"
-#include "hw/qdev-properties-system.h"
-#include "hw/irq.h"
#include "qapi/error.h"
-#include "migration/vmstate.h"
#include "chardev/char-serial.h"
diff --git a/meson.build b/meson.build
index 18cf9e2913..1f26801b69 100644
--- a/meson.build
+++ b/meson.build
@@ -4094,10 +4094,17 @@ if have_rust
# this case you must pass the path to `clang` and `libclang` to your build
# command invocation using the environment variables CLANG_PATH and
# LIBCLANG_PATH
- bindings_rs = rust.bindgen(
+ bindings_rs_tools = rust.bindgen(
input: 'rust/wrapper.h',
+ output: 'bindings_tools.inc.rs',
+ include_directories: include_directories('.', 'include'),
+ bindgen_version: ['>=0.60.0'],
+ args: bindgen_args,
+ )
+ bindings_rs_system = rust.bindgen(
+ input: 'rust/wrapper-system.h',
dependencies: common_ss.all_dependencies(),
- output: 'bindings.inc.rs',
+ output: 'bindings_system.inc.rs',
include_directories: include_directories('.', 'include'),
bindgen_version: ['>=0.60.0'],
args: bindgen_args,
diff --git a/rust/hw/char/pl011/meson.build b/rust/hw/char/pl011/meson.build
index 547cca5a96..d2cfede5dc 100644
--- a/rust/hw/char/pl011/meson.build
+++ b/rust/hw/char/pl011/meson.build
@@ -12,9 +12,10 @@ _libpl011_rs = static_library(
dependencies: [
bilge_dep,
bilge_impl_dep,
- qemu_api,
+ qemu_api_system,
qemu_api_macros,
],
+ rust_dependency_map: {'qemu_api_system': 'qemu_api'},
)
rust_devices_ss.add(when: 'CONFIG_X_PL011_RUST', if_true: [declare_dependency(
diff --git a/rust/meson.build b/rust/meson.build
index 91e52b8fb8..50eb23b072 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -9,18 +9,19 @@ if cargo.found()
run_target('clippy',
command: [config_host['MESON'], 'devenv',
'--workdir', '@CURRENT_SOURCE_DIR@',
- cargo, 'clippy', '--tests'],
- depends: bindings_rs)
+ cargo, 'clippy', '--tests', '--features', 'system'],
+ depends: bindings_rs_system)
run_target('rustfmt',
command: [config_host['MESON'], 'devenv',
'--workdir', '@CURRENT_SOURCE_DIR@',
cargo, 'fmt'],
- depends: bindings_rs)
+ depends: bindings_rs_system)
run_target('rustdoc',
command: [config_host['MESON'], 'devenv',
'--workdir', '@CURRENT_SOURCE_DIR@',
- cargo, 'doc', '--no-deps', '--document-private-items'],
- depends: bindings_rs)
+ cargo, 'doc', '--no-deps', '--document-private-items',
+ '--features', 'system'],
+ depends: bindings_rs_system)
endif
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index a51dd14285..ed7b60bc80 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -24,6 +24,7 @@ version_check = "~0.9"
default = ["debug_cell"]
allocator = []
debug_cell = []
+system= []
[lints]
workspace = true
diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
index 471e6c633d..b14f9d4e4a 100644
--- a/rust/qemu-api/build.rs
+++ b/rust/qemu-api/build.rs
@@ -16,7 +16,13 @@ fn main() -> Result<()> {
let path = env::var("MESON_BUILD_ROOT")
.unwrap_or_else(|_| format!("{}/src", env!("CARGO_MANIFEST_DIR")));
- let file = format!("{}/bindings.inc.rs", path);
+ let basename = if cfg!(feature = "system") {
+ "bindings_system.inc.rs"
+ } else {
+ "bindings_tools.inc.rs"
+ };
+
+ let file = format!("{}/{}", path, basename);
let file = Path::new(&file);
if !Path::new(&file).exists() {
panic!(concat!(
@@ -31,7 +37,7 @@ fn main() -> Result<()> {
}
let out_dir = env::var("OUT_DIR").unwrap();
- let dest_path = format!("{}/bindings.inc.rs", out_dir);
+ let dest_path = format!("{}/{}", out_dir, basename);
let dest_path = Path::new(&dest_path);
if dest_path.symlink_metadata().is_ok() {
remove_file(dest_path)?;
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 60944a657d..acac384936 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -10,39 +10,58 @@ if get_option('debug_mutex')
_qemu_api_cfg += ['--cfg', 'feature="debug_cell"']
endif
-_qemu_api_rs = static_library(
- 'qemu_api',
+sources_core = [
+ 'src/lib.rs',
+ 'src/assertions.rs',
+ 'src/bindings.rs',
+ 'src/bitops.rs',
+ 'src/callbacks.rs',
+ 'src/cell.rs',
+ 'src/c_str.rs',
+ 'src/module.rs',
+ 'src/offset_of.rs',
+ 'src/prelude.rs',
+ 'src/qom.rs',
+ 'src/zeroable.rs',
+]
+sources_system = sources_core + [
+ 'src/irq.rs',
+ 'src/qdev.rs',
+ 'src/sysbus.rs',
+ 'src/vmstate.rs',
+]
+
+
+_qemu_api_tools_rs = static_library(
+ 'qemu_api_tools',
structured_sources(
- [
- 'src/lib.rs',
- 'src/assertions.rs',
- 'src/bindings.rs',
- 'src/bitops.rs',
- 'src/callbacks.rs',
- 'src/cell.rs',
- 'src/c_str.rs',
- 'src/irq.rs',
- 'src/module.rs',
- 'src/offset_of.rs',
- 'src/prelude.rs',
- 'src/qdev.rs',
- 'src/qom.rs',
- 'src/sysbus.rs',
- 'src/vmstate.rs',
- 'src/zeroable.rs',
- ],
- {'.' : bindings_rs},
+ sources_core,
+ {'.' : bindings_rs_tools},
),
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
)
+_qemu_api_system_rs = static_library(
+ 'qemu_api_system',
+ structured_sources(
+ sources_system,
+ {'.' : bindings_rs_system},
+ ),
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ rust_args: _qemu_api_cfg + ['--cfg', 'feature="system"'],
+)
-rust.test('rust-qemu-api-tests', _qemu_api_rs,
+rust.test('rust-qemu-api-tests', _qemu_api_system_rs,
suite: ['unit', 'rust'])
-qemu_api = declare_dependency(
- link_with: _qemu_api_rs,
+qemu_api_tools = declare_dependency(
+ link_with: _qemu_api_tools_rs,
+ dependencies: qemu_api_macros,
+)
+qemu_api_system = declare_dependency(
+ link_with: _qemu_api_system_rs,
dependencies: qemu_api_macros,
)
@@ -59,7 +78,8 @@ 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_system, qemu_api_macros],
+ rust_dependency_map: {'qemu_api_system': 'qemu_api'},
link_whole: [rust_qemu_api_objs, libqemuutil]),
args: [
'--test', '--test-threads', '1',
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 8a9b821bb9..2bf6c13a32 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -15,15 +15,23 @@
clippy::missing_safety_doc
)]
-#[cfg(MESON)]
-include!("bindings.inc.rs");
+#[cfg(all(MESON, not(feature="system")))]
+include!("bindings_tools.inc.rs");
+#[cfg(all(MESON, feature="system"))]
+include!("bindings_system.inc.rs");
-#[cfg(not(MESON))]
-include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));
+#[cfg(all(not(MESON), not(feature="system")))]
+include!(concat!(env!("OUT_DIR"), "/bindings_tools.inc.rs"));
+#[cfg(all(not(MESON), feature="system"))]
+include!(concat!(env!("OUT_DIR"), "/bindings_system.inc.rs"));
unsafe impl Send for Property {}
unsafe impl Sync for Property {}
unsafe impl Sync for TypeInfo {}
+
+#[cfg(feature="system")]
unsafe impl Sync for VMStateDescription {}
+#[cfg(feature="system")]
unsafe impl Sync for VMStateField {}
+#[cfg(feature="system")]
unsafe impl Sync for VMStateInfo {}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 3cf9371cff..3c6c154f3d 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -18,12 +18,16 @@
pub mod c_str;
pub mod callbacks;
pub mod cell;
+#[cfg(feature = "system")]
pub mod irq;
pub mod module;
pub mod offset_of;
+#[cfg(feature = "system")]
pub mod qdev;
pub mod qom;
+#[cfg(feature = "system")]
pub mod sysbus;
+#[cfg(feature = "system")]
pub mod vmstate;
pub mod zeroable;
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 2dc86e19b2..1b8d7d319e 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -17,6 +17,8 @@
pub use crate::qom_isa;
+#[cfg(feature="system")]
pub use crate::sysbus::SysBusDeviceMethods;
+#[cfg(feature="system")]
pub use crate::vmstate::VMState;
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 7b04947cb6..b454e9e05e 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -56,6 +56,7 @@ pub unsafe trait Zeroable: Default {
/// ## Differences with `core::mem::zeroed`
///
/// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't
+#[allow(unused)]
macro_rules! const_zero {
// This macro to produce a type-generic zero constant is taken from the
// const_zero crate (v0.1.1):
@@ -77,6 +78,7 @@ union TypeAsBytes {
}
/// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
+#[allow(unused)]
macro_rules! impl_zeroable {
($type:ty) => {
unsafe impl Zeroable for $type {
@@ -86,6 +88,7 @@ unsafe impl Zeroable for $type {
}
// bindgen does not derive Default here
+#[cfg(feature = "system")]
#[allow(clippy::derivable_impls)]
impl Default for crate::bindings::VMStateFlags {
fn default() -> Self {
@@ -93,10 +96,17 @@ fn default() -> Self {
}
}
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::Property__bindgen_ty_1);
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::Property);
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::VMStateFlags);
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::VMStateField);
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::VMStateDescription);
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
+#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system
2025-02-11 21:43 ` [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system Kevin Wolf
@ 2025-02-12 10:01 ` Paolo Bonzini
2025-02-12 15:29 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 10:01 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/11/25 22:43, Kevin Wolf wrote:
> The existing qemu_api library can't be linked into tools because it
> contains a few bindings for things that only exist in the system
> emulator.
>
> This adds a new "system" feature to the qemu_api crate that enables the
> system emulator parts in it, and build the crate twice: qemu_api_tools
> is the core library that can be linked into tools, and qemu_api_system
> is the full one for the system emulator.
As discussed on IRC, the issue here is ClassInitImpl<>, which has to be
defined in the same crate for qemu_api::qom and qemu_api::qdev.
Right now, the block layer has no use for QOM, but later it will (for
secret management, for example), so splitting QOM into a separate crate
does not work long term.
I'll try to figure out an alternative way to do the class_init bindings.
Paolo
> Unfortunately, since library names have to be unique in meson, this
> means that every user of the library now needs to specify a
> rust_dependency_map to make either qemu_api_tools or qemu_api_system
> show up as the qemu_api crate in Rust.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> rust/wrapper-system.h | 44 +++++++++++++++++++++
> rust/wrapper.h | 9 -----
> meson.build | 11 +++++-
> rust/hw/char/pl011/meson.build | 3 +-
> rust/meson.build | 11 +++---
> rust/qemu-api/Cargo.toml | 1 +
> rust/qemu-api/build.rs | 10 ++++-
> rust/qemu-api/meson.build | 70 ++++++++++++++++++++++------------
> rust/qemu-api/src/bindings.rs | 16 ++++++--
> rust/qemu-api/src/lib.rs | 4 ++
> rust/qemu-api/src/prelude.rs | 2 +
> rust/qemu-api/src/zeroable.rs | 10 +++++
> 12 files changed, 143 insertions(+), 48 deletions(-)
> create mode 100644 rust/wrapper-system.h
>
> diff --git a/rust/wrapper-system.h b/rust/wrapper-system.h
> new file mode 100644
> index 0000000000..fc6c571e6d
> --- /dev/null
> +++ b/rust/wrapper-system.h
> @@ -0,0 +1,44 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2024 Linaro Ltd.
> + *
> + * Authors: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +
> +/*
> + * This header file is meant to be used as input to the `bindgen` application
> + * in order to generate C FFI compatible Rust bindings.
> + */
> +
> +/* The system emulator has all of the bindings tools have */
> +#include "wrapper.h"
> +
> +#include "system/system.h"
> +#include "hw/sysbus.h"
> +#include "exec/memory.h"
> +#include "hw/clock.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index a9bc67af0d..41be87adcf 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -50,15 +50,6 @@ typedef enum memory_order {
> #include "qemu/osdep.h"
> #include "qemu/module.h"
> #include "qemu-io.h"
> -#include "system/system.h"
> -#include "hw/sysbus.h"
> -#include "exec/memory.h"
> #include "chardev/char-fe.h"
> -#include "hw/clock.h"
> -#include "hw/qdev-clock.h"
> -#include "hw/qdev-properties.h"
> -#include "hw/qdev-properties-system.h"
> -#include "hw/irq.h"
> #include "qapi/error.h"
> -#include "migration/vmstate.h"
> #include "chardev/char-serial.h"
> diff --git a/meson.build b/meson.build
> index 18cf9e2913..1f26801b69 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -4094,10 +4094,17 @@ if have_rust
> # this case you must pass the path to `clang` and `libclang` to your build
> # command invocation using the environment variables CLANG_PATH and
> # LIBCLANG_PATH
> - bindings_rs = rust.bindgen(
> + bindings_rs_tools = rust.bindgen(
> input: 'rust/wrapper.h',
> + output: 'bindings_tools.inc.rs',
> + include_directories: include_directories('.', 'include'),
> + bindgen_version: ['>=0.60.0'],
> + args: bindgen_args,
> + )
> + bindings_rs_system = rust.bindgen(
> + input: 'rust/wrapper-system.h',
> dependencies: common_ss.all_dependencies(),
> - output: 'bindings.inc.rs',
> + output: 'bindings_system.inc.rs',
> include_directories: include_directories('.', 'include'),
> bindgen_version: ['>=0.60.0'],
> args: bindgen_args,
> diff --git a/rust/hw/char/pl011/meson.build b/rust/hw/char/pl011/meson.build
> index 547cca5a96..d2cfede5dc 100644
> --- a/rust/hw/char/pl011/meson.build
> +++ b/rust/hw/char/pl011/meson.build
> @@ -12,9 +12,10 @@ _libpl011_rs = static_library(
> dependencies: [
> bilge_dep,
> bilge_impl_dep,
> - qemu_api,
> + qemu_api_system,
> qemu_api_macros,
> ],
> + rust_dependency_map: {'qemu_api_system': 'qemu_api'},
> )
>
> rust_devices_ss.add(when: 'CONFIG_X_PL011_RUST', if_true: [declare_dependency(
> diff --git a/rust/meson.build b/rust/meson.build
> index 91e52b8fb8..50eb23b072 100644
> --- a/rust/meson.build
> +++ b/rust/meson.build
> @@ -9,18 +9,19 @@ if cargo.found()
> run_target('clippy',
> command: [config_host['MESON'], 'devenv',
> '--workdir', '@CURRENT_SOURCE_DIR@',
> - cargo, 'clippy', '--tests'],
> - depends: bindings_rs)
> + cargo, 'clippy', '--tests', '--features', 'system'],
> + depends: bindings_rs_system)
>
> run_target('rustfmt',
> command: [config_host['MESON'], 'devenv',
> '--workdir', '@CURRENT_SOURCE_DIR@',
> cargo, 'fmt'],
> - depends: bindings_rs)
> + depends: bindings_rs_system)
>
> run_target('rustdoc',
> command: [config_host['MESON'], 'devenv',
> '--workdir', '@CURRENT_SOURCE_DIR@',
> - cargo, 'doc', '--no-deps', '--document-private-items'],
> - depends: bindings_rs)
> + cargo, 'doc', '--no-deps', '--document-private-items',
> + '--features', 'system'],
> + depends: bindings_rs_system)
> endif
> diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
> index a51dd14285..ed7b60bc80 100644
> --- a/rust/qemu-api/Cargo.toml
> +++ b/rust/qemu-api/Cargo.toml
> @@ -24,6 +24,7 @@ version_check = "~0.9"
> default = ["debug_cell"]
> allocator = []
> debug_cell = []
> +system= []
>
> [lints]
> workspace = true
> diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
> index 471e6c633d..b14f9d4e4a 100644
> --- a/rust/qemu-api/build.rs
> +++ b/rust/qemu-api/build.rs
> @@ -16,7 +16,13 @@ fn main() -> Result<()> {
> let path = env::var("MESON_BUILD_ROOT")
> .unwrap_or_else(|_| format!("{}/src", env!("CARGO_MANIFEST_DIR")));
>
> - let file = format!("{}/bindings.inc.rs", path);
> + let basename = if cfg!(feature = "system") {
> + "bindings_system.inc.rs"
> + } else {
> + "bindings_tools.inc.rs"
> + };
> +
> + let file = format!("{}/{}", path, basename);
> let file = Path::new(&file);
> if !Path::new(&file).exists() {
> panic!(concat!(
> @@ -31,7 +37,7 @@ fn main() -> Result<()> {
> }
>
> let out_dir = env::var("OUT_DIR").unwrap();
> - let dest_path = format!("{}/bindings.inc.rs", out_dir);
> + let dest_path = format!("{}/{}", out_dir, basename);
> let dest_path = Path::new(&dest_path);
> if dest_path.symlink_metadata().is_ok() {
> remove_file(dest_path)?;
> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index 60944a657d..acac384936 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -10,39 +10,58 @@ if get_option('debug_mutex')
> _qemu_api_cfg += ['--cfg', 'feature="debug_cell"']
> endif
>
> -_qemu_api_rs = static_library(
> - 'qemu_api',
> +sources_core = [
> + 'src/lib.rs',
> + 'src/assertions.rs',
> + 'src/bindings.rs',
> + 'src/bitops.rs',
> + 'src/callbacks.rs',
> + 'src/cell.rs',
> + 'src/c_str.rs',
> + 'src/module.rs',
> + 'src/offset_of.rs',
> + 'src/prelude.rs',
> + 'src/qom.rs',
> + 'src/zeroable.rs',
> +]
> +sources_system = sources_core + [
> + 'src/irq.rs',
> + 'src/qdev.rs',
> + 'src/sysbus.rs',
> + 'src/vmstate.rs',
> +]
> +
> +
> +_qemu_api_tools_rs = static_library(
> + 'qemu_api_tools',
> structured_sources(
> - [
> - 'src/lib.rs',
> - 'src/assertions.rs',
> - 'src/bindings.rs',
> - 'src/bitops.rs',
> - 'src/callbacks.rs',
> - 'src/cell.rs',
> - 'src/c_str.rs',
> - 'src/irq.rs',
> - 'src/module.rs',
> - 'src/offset_of.rs',
> - 'src/prelude.rs',
> - 'src/qdev.rs',
> - 'src/qom.rs',
> - 'src/sysbus.rs',
> - 'src/vmstate.rs',
> - 'src/zeroable.rs',
> - ],
> - {'.' : bindings_rs},
> + sources_core,
> + {'.' : bindings_rs_tools},
> ),
> override_options: ['rust_std=2021', 'build.rust_std=2021'],
> rust_abi: 'rust',
> rust_args: _qemu_api_cfg,
> )
> +_qemu_api_system_rs = static_library(
> + 'qemu_api_system',
> + structured_sources(
> + sources_system,
> + {'.' : bindings_rs_system},
> + ),
> + override_options: ['rust_std=2021', 'build.rust_std=2021'],
> + rust_abi: 'rust',
> + rust_args: _qemu_api_cfg + ['--cfg', 'feature="system"'],
> +)
>
> -rust.test('rust-qemu-api-tests', _qemu_api_rs,
> +rust.test('rust-qemu-api-tests', _qemu_api_system_rs,
> suite: ['unit', 'rust'])
>
> -qemu_api = declare_dependency(
> - link_with: _qemu_api_rs,
> +qemu_api_tools = declare_dependency(
> + link_with: _qemu_api_tools_rs,
> + dependencies: qemu_api_macros,
> +)
> +qemu_api_system = declare_dependency(
> + link_with: _qemu_api_system_rs,
> dependencies: qemu_api_macros,
> )
>
> @@ -59,7 +78,8 @@ 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_system, qemu_api_macros],
> + rust_dependency_map: {'qemu_api_system': 'qemu_api'},
> link_whole: [rust_qemu_api_objs, libqemuutil]),
> args: [
> '--test', '--test-threads', '1',
> diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
> index 8a9b821bb9..2bf6c13a32 100644
> --- a/rust/qemu-api/src/bindings.rs
> +++ b/rust/qemu-api/src/bindings.rs
> @@ -15,15 +15,23 @@
> clippy::missing_safety_doc
> )]
>
> -#[cfg(MESON)]
> -include!("bindings.inc.rs");
> +#[cfg(all(MESON, not(feature="system")))]
> +include!("bindings_tools.inc.rs");
> +#[cfg(all(MESON, feature="system"))]
> +include!("bindings_system.inc.rs");
>
> -#[cfg(not(MESON))]
> -include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));
> +#[cfg(all(not(MESON), not(feature="system")))]
> +include!(concat!(env!("OUT_DIR"), "/bindings_tools.inc.rs"));
> +#[cfg(all(not(MESON), feature="system"))]
> +include!(concat!(env!("OUT_DIR"), "/bindings_system.inc.rs"));
>
> unsafe impl Send for Property {}
> unsafe impl Sync for Property {}
> unsafe impl Sync for TypeInfo {}
> +
> +#[cfg(feature="system")]
> unsafe impl Sync for VMStateDescription {}
> +#[cfg(feature="system")]
> unsafe impl Sync for VMStateField {}
> +#[cfg(feature="system")]
> unsafe impl Sync for VMStateInfo {}
> diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
> index 3cf9371cff..3c6c154f3d 100644
> --- a/rust/qemu-api/src/lib.rs
> +++ b/rust/qemu-api/src/lib.rs
> @@ -18,12 +18,16 @@
> pub mod c_str;
> pub mod callbacks;
> pub mod cell;
> +#[cfg(feature = "system")]
> pub mod irq;
> pub mod module;
> pub mod offset_of;
> +#[cfg(feature = "system")]
> pub mod qdev;
> pub mod qom;
> +#[cfg(feature = "system")]
> pub mod sysbus;
> +#[cfg(feature = "system")]
> pub mod vmstate;
> pub mod zeroable;
>
> diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
> index 2dc86e19b2..1b8d7d319e 100644
> --- a/rust/qemu-api/src/prelude.rs
> +++ b/rust/qemu-api/src/prelude.rs
> @@ -17,6 +17,8 @@
>
> pub use crate::qom_isa;
>
> +#[cfg(feature="system")]
> pub use crate::sysbus::SysBusDeviceMethods;
>
> +#[cfg(feature="system")]
> pub use crate::vmstate::VMState;
> diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
> index 7b04947cb6..b454e9e05e 100644
> --- a/rust/qemu-api/src/zeroable.rs
> +++ b/rust/qemu-api/src/zeroable.rs
> @@ -56,6 +56,7 @@ pub unsafe trait Zeroable: Default {
> /// ## Differences with `core::mem::zeroed`
> ///
> /// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't
> +#[allow(unused)]
> macro_rules! const_zero {
> // This macro to produce a type-generic zero constant is taken from the
> // const_zero crate (v0.1.1):
> @@ -77,6 +78,7 @@ union TypeAsBytes {
> }
>
> /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
> +#[allow(unused)]
> macro_rules! impl_zeroable {
> ($type:ty) => {
> unsafe impl Zeroable for $type {
> @@ -86,6 +88,7 @@ unsafe impl Zeroable for $type {
> }
>
> // bindgen does not derive Default here
> +#[cfg(feature = "system")]
> #[allow(clippy::derivable_impls)]
> impl Default for crate::bindings::VMStateFlags {
> fn default() -> Self {
> @@ -93,10 +96,17 @@ fn default() -> Self {
> }
> }
>
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::Property__bindgen_ty_1);
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::Property);
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::VMStateFlags);
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::VMStateField);
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::VMStateDescription);
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
> +#[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system
2025-02-12 10:01 ` Paolo Bonzini
@ 2025-02-12 15:29 ` Kevin Wolf
2025-02-12 16:59 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 15:29 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 11:01 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > The existing qemu_api library can't be linked into tools because it
> > contains a few bindings for things that only exist in the system
> > emulator.
> >
> > This adds a new "system" feature to the qemu_api crate that enables the
> > system emulator parts in it, and build the crate twice: qemu_api_tools
> > is the core library that can be linked into tools, and qemu_api_system
> > is the full one for the system emulator.
>
> As discussed on IRC, the issue here is ClassInitImpl<>, which has to be
> defined in the same crate for qemu_api::qom and qemu_api::qdev.
>
> Right now, the block layer has no use for QOM, but later it will (for secret
> management, for example), so splitting QOM into a separate crate does not
> work long term.
>
> I'll try to figure out an alternative way to do the class_init bindings.
There were more problems with splitting the qemu_api crate related to
bindgen. Basically, you would want the system emulator bindings to
contain only those things that aren't already part of the common
bindings. But the system emulator headers will obviously include common
headers, too, so this becomes tricky.
If you don't do this, you get two bindings for the same type, but the
binding types won't be compatible with each other etc.
This approach of just building two separate libraries was a lot easier.
Apart from the obvious inefficiency, I just hate the need for
rust_dependency_map everywhere to make the library show up with the
neutral 'qemu_api' name in both cases. Maybe there is a better approach
where this could be defined for the library itself rather than for each
user, but I couldn't find one. meson is still black magic to me.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system
2025-02-12 15:29 ` Kevin Wolf
@ 2025-02-12 16:59 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 16:59 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/12/25 16:29, Kevin Wolf wrote:
> Am 12.02.2025 um 11:01 hat Paolo Bonzini geschrieben:
>> On 2/11/25 22:43, Kevin Wolf wrote:
>>> The existing qemu_api library can't be linked into tools because it
>>> contains a few bindings for things that only exist in the system
>>> emulator.
>>>
>>> This adds a new "system" feature to the qemu_api crate that enables the
>>> system emulator parts in it, and build the crate twice: qemu_api_tools
>>> is the core library that can be linked into tools, and qemu_api_system
>>> is the full one for the system emulator.
>>
>> As discussed on IRC, the issue here is ClassInitImpl<>, which has to be
>> defined in the same crate for qemu_api::qom and qemu_api::qdev.
>>
>> Right now, the block layer has no use for QOM, but later it will (for secret
>> management, for example), so splitting QOM into a separate crate does not
>> work long term.
>>
>> I'll try to figure out an alternative way to do the class_init bindings.
>
> There were more problems with splitting the qemu_api crate related to
> bindgen. Basically, you would want the system emulator bindings to
> contain only those things that aren't already part of the common
> bindings. But the system emulator headers will obviously include common
> headers, too, so this becomes tricky.
>
> If you don't do this, you get two bindings for the same type, but the
> binding types won't be compatible with each other etc.
That might be a good reason to move the bindings to their own crate.
Then you don't really care if the bindings crate has declarations for
things that are only for system emulation, because they're just externs.
qemu_api is currently doing "impl Foo" directly on structs defined by
bindgen, but that can/should be changed. This way a PhantomPinned field
can be added, they can be wrapped with UnsafeCell<>, etc. I need to
understand better what Linux does[1] and document it.
Anyhow this is not a blocker, this patch is easily reverted.
> This approach of just building two separate libraries was a lot easier.
> Apart from the obvious inefficiency, I just hate the need for
> rust_dependency_map everywhere to make the library show up with the
> neutral 'qemu_api' name in both cases. Maybe there is a better approach
> where this could be defined for the library itself rather than for each
> user, but I couldn't find one. meson is still black magic to me.
Yeah that's ugly. There's no way to define it for the library indeed.
I'd like to have split crates because for example we're now building the
QOM and block layer code twice as well. Ideally, I'd like to have
crates roughly matching the C static_libraries, so for example utils,
bindings, block, chardev, hw, etc.
Paolo
[1] https://rust-for-linux.github.io/docs/kernel/struct.Opaque.html
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 02/11] meson: Add rust_block_ss and link tools with it
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
2025-02-11 21:43 ` [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 7:38 ` Philippe Mathieu-Daudé
2025-02-11 21:43 ` [PATCH 03/11] rust: Add some block layer bindings Kevin Wolf
` (8 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
meson.build | 36 ++++++++++++++++++++++++++++++++----
storage-daemon/meson.build | 2 +-
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/meson.build b/meson.build
index 1f26801b69..30aae6b3c3 100644
--- a/meson.build
+++ b/meson.build
@@ -3640,6 +3640,7 @@ qom_ss = ss.source_set()
system_ss = ss.source_set()
specific_fuzz_ss = ss.source_set()
specific_ss = ss.source_set()
+rust_block_ss = ss.source_set()
rust_devices_ss = ss.source_set()
stub_ss = ss.source_set()
trace_ss = ss.source_set()
@@ -4209,7 +4210,11 @@ foreach target : target_dirs
arch_deps += target_specific.dependencies()
if have_rust and target_type == 'system'
- target_rust = rust_devices_ss.apply(config_target, strict: false)
+ target_rust_ss = ss.source_set()
+ target_rust_ss.add_all(rust_block_ss)
+ target_rust_ss.add_all(rust_devices_ss)
+
+ target_rust = target_rust_ss.apply(config_target, strict: false)
crates = []
foreach dep : target_rust.dependencies()
crates += dep.get_variable('crate')
@@ -4349,15 +4354,38 @@ if xkbcommon.found()
endif
if have_tools
+ tools_deps = []
+ if have_rust
+ tools_rust = rust_block_ss.apply({})
+ crates = []
+ foreach dep : tools_rust.dependencies()
+ crates += dep.get_variable('crate')
+ endforeach
+ if crates.length() > 0
+ rlib_rs = custom_target('rust_tools.rs',
+ output: 'rust_tools.rs',
+ command: [rust_root_crate, crates],
+ capture: true,
+ build_by_default: true,
+ build_always_stale: true)
+ rlib = static_library('rust_tools',
+ rlib_rs,
+ dependencies: tools_rust.dependencies(),
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'c')
+ tools_deps += declare_dependency(link_whole: [rlib])
+ endif
+ endif
+
qemu_img = executable('qemu-img', [files('qemu-img.c'), hxdep],
link_args: '@block.syms', link_depends: block_syms,
- dependencies: [authz, block, crypto, io, qom, qemuutil], install: true)
+ dependencies: tools_deps + [authz, block, crypto, io, qom, qemuutil], install: true)
qemu_io = executable('qemu-io', files('qemu-io.c'),
link_args: '@block.syms', link_depends: block_syms,
- dependencies: [block, qemuutil], install: true)
+ dependencies: tools_deps + [block, qemuutil], install: true)
qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
link_args: '@block.syms', link_depends: block_syms,
- dependencies: [blockdev, qemuutil, selinux],
+ dependencies: tools_deps + [blockdev, qemuutil, selinux],
install: true)
subdir('storage-daemon')
diff --git a/storage-daemon/meson.build b/storage-daemon/meson.build
index 5e61a9d1bd..92bc2e0cba 100644
--- a/storage-daemon/meson.build
+++ b/storage-daemon/meson.build
@@ -9,6 +9,6 @@ if have_tools
qsd = executable('qemu-storage-daemon',
qsd_ss.sources(),
link_args: '@block.syms', link_depends: block_syms,
- dependencies: qsd_ss.dependencies(),
+ dependencies: tools_deps + qsd_ss.dependencies(),
install: true)
endif
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 02/11] meson: Add rust_block_ss and link tools with it
2025-02-11 21:43 ` [PATCH 02/11] meson: Add rust_block_ss and link tools with it Kevin Wolf
@ 2025-02-12 7:38 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 7:38 UTC (permalink / raw)
To: Kevin Wolf, qemu-block
Cc: hreitz, pbonzini, manos.pitsidianakis, qemu-devel, qemu-rust
On 11/2/25 22:43, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> meson.build | 36 ++++++++++++++++++++++++++++++++----
> storage-daemon/meson.build | 2 +-
> 2 files changed, 33 insertions(+), 5 deletions(-)
> if have_rust and target_type == 'system'
> - target_rust = rust_devices_ss.apply(config_target, strict: false)
> + target_rust_ss = ss.source_set()
> + target_rust_ss.add_all(rust_block_ss)
> + target_rust_ss.add_all(rust_devices_ss)
> +
> + target_rust = target_rust_ss.apply(config_target, strict: false)
> crates = []
> foreach dep : target_rust.dependencies()
> crates += dep.get_variable('crate')
> @@ -4349,15 +4354,38 @@ if xkbcommon.found()
> endif
>
> if have_tools
> + tools_deps = []
To simplify, we can likely start with:
tools_deps = [qemuutil]
Maybe clearer to introduce tools_deps[] as a preliminary
patch although.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 03/11] rust: Add some block layer bindings
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
2025-02-11 21:43 ` [PATCH 01/11] rust: Build separate qemu_api_tools and qemu_api_system Kevin Wolf
2025-02-11 21:43 ` [PATCH 02/11] meson: Add rust_block_ss and link tools with it Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 9:29 ` Paolo Bonzini
2025-02-11 21:43 ` [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU Kevin Wolf
` (7 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/wrapper.h | 4 ++++
meson.build | 1 +
rust/qemu-api/src/zeroable.rs | 5 +++--
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/rust/wrapper.h b/rust/wrapper.h
index 41be87adcf..c3e1e6f9cf 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -53,3 +53,7 @@ typedef enum memory_order {
#include "chardev/char-fe.h"
#include "qapi/error.h"
#include "chardev/char-serial.h"
+#include "block/block.h"
+#include "block/block_int.h"
+#include "block/qdict.h"
+#include "qapi/qapi-visit-block-core.h"
diff --git a/meson.build b/meson.build
index 30aae6b3c3..154195bc80 100644
--- a/meson.build
+++ b/meson.build
@@ -4045,6 +4045,7 @@ if have_rust
'--with-derive-default',
'--no-layout-tests',
'--no-prepend-enum-name',
+ '--allowlist-item', 'EINVAL|EIO',
'--allowlist-file', meson.project_source_root() + '/include/.*',
'--allowlist-file', meson.project_source_root() + '/.*',
'--allowlist-file', meson.project_build_root() + '/.*'
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index b454e9e05e..60af681293 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -56,7 +56,6 @@ pub unsafe trait Zeroable: Default {
/// ## Differences with `core::mem::zeroed`
///
/// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't
-#[allow(unused)]
macro_rules! const_zero {
// This macro to produce a type-generic zero constant is taken from the
// const_zero crate (v0.1.1):
@@ -78,7 +77,6 @@ union TypeAsBytes {
}
/// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
-#[allow(unused)]
macro_rules! impl_zeroable {
($type:ty) => {
unsafe impl Zeroable for $type {
@@ -110,3 +108,6 @@ fn default() -> Self {
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
#[cfg(feature = "system")]
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
+
+impl_zeroable!(crate::bindings::BlockDriver);
+impl_zeroable!(crate::bindings::BlockDriver__bindgen_ty_1);
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-11 21:43 ` [PATCH 03/11] rust: Add some block layer bindings Kevin Wolf
@ 2025-02-12 9:29 ` Paolo Bonzini
2025-02-12 13:13 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 9:29 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/11/25 22:43, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> rust/wrapper.h | 4 ++++
> meson.build | 1 +
> rust/qemu-api/src/zeroable.rs | 5 +++--
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 41be87adcf..c3e1e6f9cf 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -53,3 +53,7 @@ typedef enum memory_order {
> #include "chardev/char-fe.h"
> #include "qapi/error.h"
> #include "chardev/char-serial.h"
> +#include "block/block.h"
> +#include "block/block_int.h"
> +#include "block/qdict.h"
> +#include "qapi/qapi-visit-block-core.h"
> diff --git a/meson.build b/meson.build
> index 30aae6b3c3..154195bc80 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -4045,6 +4045,7 @@ if have_rust
> '--with-derive-default',
> '--no-layout-tests',
> '--no-prepend-enum-name',
> + '--allowlist-item', 'EINVAL|EIO',
I've got some errno bindings that I wrote for chardev, I'll send them
shortly.
Paolo
> '--allowlist-file', meson.project_source_root() + '/include/.*',
> '--allowlist-file', meson.project_source_root() + '/.*',
> '--allowlist-file', meson.project_build_root() + '/.*'
> diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
> index b454e9e05e..60af681293 100644
> --- a/rust/qemu-api/src/zeroable.rs
> +++ b/rust/qemu-api/src/zeroable.rs
> @@ -56,7 +56,6 @@ pub unsafe trait Zeroable: Default {
> /// ## Differences with `core::mem::zeroed`
> ///
> /// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't
> -#[allow(unused)]
> macro_rules! const_zero {
> // This macro to produce a type-generic zero constant is taken from the
> // const_zero crate (v0.1.1):
> @@ -78,7 +77,6 @@ union TypeAsBytes {
> }
>
> /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
> -#[allow(unused)]
> macro_rules! impl_zeroable {
> ($type:ty) => {
> unsafe impl Zeroable for $type {
> @@ -110,3 +108,6 @@ fn default() -> Self {
> impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
> #[cfg(feature = "system")]
> impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
> +
> +impl_zeroable!(crate::bindings::BlockDriver);
> +impl_zeroable!(crate::bindings::BlockDriver__bindgen_ty_1);
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-12 9:29 ` Paolo Bonzini
@ 2025-02-12 13:13 ` Kevin Wolf
2025-02-12 13:47 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 13:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 10:29 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > rust/wrapper.h | 4 ++++
> > meson.build | 1 +
> > rust/qemu-api/src/zeroable.rs | 5 +++--
> > 3 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/wrapper.h b/rust/wrapper.h
> > index 41be87adcf..c3e1e6f9cf 100644
> > --- a/rust/wrapper.h
> > +++ b/rust/wrapper.h
> > @@ -53,3 +53,7 @@ typedef enum memory_order {
> > #include "chardev/char-fe.h"
> > #include "qapi/error.h"
> > #include "chardev/char-serial.h"
> > +#include "block/block.h"
> > +#include "block/block_int.h"
> > +#include "block/qdict.h"
> > +#include "qapi/qapi-visit-block-core.h"
> > diff --git a/meson.build b/meson.build
> > index 30aae6b3c3..154195bc80 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -4045,6 +4045,7 @@ if have_rust
> > '--with-derive-default',
> > '--no-layout-tests',
> > '--no-prepend-enum-name',
> > + '--allowlist-item', 'EINVAL|EIO',
>
> I've got some errno bindings that I wrote for chardev, I'll send them
> shortly.
Yes, we definitely need some proper bindings there. I'm already tired of
writing things like this:
return -(bindings::EINVAL as std::os::raw::c_int)
Or even:
return e
.raw_os_error()
.unwrap_or(-(bindings::EIO as std::os::raw::c_int))
Which actually already shows that your errno binding patch does the
opposite direction of what I needed in this series. My problem is when I
need to return an int to C, and I either have an io::Result or I just
want to directly return an errno value. So we'll have to add that part
to your errno module, too.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-12 13:13 ` Kevin Wolf
@ 2025-02-12 13:47 ` Paolo Bonzini
2025-02-12 15:13 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 13:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On Wed, Feb 12, 2025 at 2:13 PM Kevin Wolf <kwolf@redhat.com> wrote:
> Yes, we definitely need some proper bindings there. I'm already tired of
> writing things like this:
>
> return -(bindings::EINVAL as std::os::raw::c_int)
>
> Or even:
>
> return e
> .raw_os_error()
> .unwrap_or(-(bindings::EIO as std::os::raw::c_int))
This by the way seemed incorrect to me as it should be
return -(e
.raw_os_error()
.unwrap_or(bindings::EIO as std::os::raw::c_int))
(leaving aside that raw_os_error does not work on Windows)... But then
I noticed that read_raw() also does not negate, which causes the error
to print incorrectly...
> Which actually already shows that your errno binding patch does the
> opposite direction of what I needed in this series.
... so my patch already helps a bit: you can still change
if ret < 0 {
Err(Error::from_raw_os_error(ret))
} else {
Ok(())
}
to
errno::into_io_result(ret)?;
Ok(())
and avoid the positive/negative confusion.
Anyhow, I guess the first one wouldn't be much better:
return errno::into_negative_errno(ErrorKind::InvalidInput);
whereas the second could be
return errno::into_negative_errno(e);
But then the first is already a special case; it only happens where
your bindings are not trivial thanks to the introduction of the
Mapping type.
Paolo
> My problem is when I
> need to return an int to C, and I either have an io::Result or I just
> want to directly return an errno value. So we'll have to add that part
> to your errno module, too.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-12 13:47 ` Paolo Bonzini
@ 2025-02-12 15:13 ` Kevin Wolf
2025-02-12 17:16 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 15:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 14:47 hat Paolo Bonzini geschrieben:
> On Wed, Feb 12, 2025 at 2:13 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > Yes, we definitely need some proper bindings there. I'm already tired of
> > writing things like this:
> >
> > return -(bindings::EINVAL as std::os::raw::c_int)
> >
> > Or even:
> >
> > return e
> > .raw_os_error()
> > .unwrap_or(-(bindings::EIO as std::os::raw::c_int))
>
> This by the way seemed incorrect to me as it should be
>
> return -(e
> .raw_os_error()
> .unwrap_or(bindings::EIO as std::os::raw::c_int))
>
> (leaving aside that raw_os_error does not work on Windows)... But then
> I noticed that read_raw() also does not negate, which causes the error
> to print incorrectly...
Yes, I actually noticed that after sending the email and fixed it (and
another instances of the same) up locally.
> > Which actually already shows that your errno binding patch does the
> > opposite direction of what I needed in this series.
>
> ... so my patch already helps a bit: you can still change
>
> if ret < 0 {
> Err(Error::from_raw_os_error(ret))
> } else {
> Ok(())
> }
>
> to
>
> errno::into_io_result(ret)?;
> Ok(())
>
> and avoid the positive/negative confusion.
No reason to write essentially if (ret != 0) { ret } else { 0 }. This
one should do:
errno::into_io_result(ret)
And yes, it's already a good improvement.
> Anyhow, I guess the first one wouldn't be much better:
>
> return errno::into_negative_errno(ErrorKind::InvalidInput);
>
> whereas the second could be
>
> return errno::into_negative_errno(e);
>
> But then the first is already a special case; it only happens where
> your bindings are not trivial thanks to the introduction of the
> Mapping type.
Yes, the second one seems like the more important one because the other
one should only happen in bindings eventually. We could still have
something like an errno!(InvalidInput) to make the code in bindings less
verbose.
Or if you have to define the constants anyway - you currently do this
only for Windows, but for into_negative_errno() you might need it on
Linux, too - and it wouldn't be a problem for the constants to be
signed (that they are unsigned is the main reason why it becomes so ugly
with the bindgen constants), you could just make it -errno::EINVAL
again.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-12 15:13 ` Kevin Wolf
@ 2025-02-12 17:16 ` Paolo Bonzini
2025-02-12 19:52 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 17:16 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 455 bytes --]
On 2/12/25 16:13, Kevin Wolf wrote:
> Or if you have to define the constants anyway - you currently do this
> only for Windows, but for into_negative_errno() you might need it on
> Linux, too - and it wouldn't be a problem for the constants to be
> signed (that they are unsigned is the main reason why it becomes so ugly
> with the bindgen constants), you could just make it -errno::EINVAL
> again.
Or just include the libc crate, see attachment.
Paolo
[-- Attachment #2: 0001-rust-subprojects-add-libc-crate.patch --]
[-- Type: text/x-patch, Size: 7529 bytes --]
From 526ad76001e788eb9adb2624eb4bbedae50301f9 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 16 Dec 2024 09:42:35 +0100
Subject: [PATCH] rust: subprojects: add libc crate
This allows access to errno values.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/Cargo.lock | 7 ++++
rust/qemu-api/Cargo.toml | 1 +
rust/qemu-api/src/errno.rs | 32 ++---------------
scripts/archive-source.sh | 2 +-
scripts/make-release | 4 +--
subprojects/libc-0.2-rs.wrap | 7 ++++
.../packagefiles/libc-0.2-rs/meson.build | 36 +++++++++++++++++++
7 files changed, 56 insertions(+), 33 deletions(-)
create mode 100644 subprojects/libc-0.2-rs.wrap
create mode 100644 subprojects/packagefiles/libc-0.2-rs/meson.build
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index bd22cc39c16..ac9806a1b4f 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -56,4 +56,10 @@ dependencies = [
"either",
]
+[[package]]
+name = "libc"
+version = "0.2.162"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "18d287de67fe55fd7e1581fe933d965a5a9477b38e949cfa9f8574ef01506398"
+
[[package]]
@@ -126,4 +132,5 @@ dependencies = [
name = "qemu_api"
version = "0.1.0"
dependencies = [
+ "libc",
"qemu_api_macros",
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index d273e5d903f..3708d97ae75 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -1,6 +1,8 @@
_qemu_api_cfg = run_command(rustc_args,
'--config-headers', config_host_h, '--features', files('Cargo.toml'),
capture: true, check: true).stdout().strip().splitlines()
+
+libc_dep = dependency('libc-0.2-rs')
if get_option('debug_mutex')
_qemu_api_cfg += ['--cfg', 'feature="debug_cell"']
@@ -38,6 +38,7 @@ _qemu_api_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
+ link_with: libc_dep,
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index c96e87f592e..7255e9ee596 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -18,6 +18,7 @@ rust-version = "1.63.0"
[dependencies]
qemu_api_macros = { path = "../qemu-api-macros" }
+libc = "0.2.162"
[build-dependencies]
version_check = "~0.9"
diff --git a/rust/qemu-api/src/errno.rs b/rust/qemu-api/src/errno.rs
index c697f9bef05..58d46abc21c 100644
--- a/rust/qemu-api/src/errno.rs
+++ b/rust/qemu-api/src/errno.rs
@@ -16,32 +16,6 @@
// into io::Error by hand. For simplicity use ErrorKind and use
// the standard library's simple-minded mapping of ErrorKind to Error
// (`impl From<ErrorKind> for io::Error`).
-//
-// Since this is just for Windows, do not bother with using the libc
-// crate or generating the constants from C. Just list here the
-// constants that map to stable error kinds.
-#[cfg(windows)]
-mod libc {
- pub const EPERM: u16 = 1;
- pub const ENOENT: u16 = 2;
- pub const EINTR: u16 = 4;
- pub const EAGAIN: u16 = 11;
- pub const ENOMEM: u16 = 12;
- pub const EACCES: u16 = 13;
- pub const EEXIST: u16 = 17;
- pub const EINVAL: u16 = 22;
- pub const EPIPE: u16 = 32;
- pub const EADDRINUSE: u16 = 100;
- pub const EADDRNOTAVAIL: u16 = 101;
- pub const ECONNABORTED: u16 = 106;
- pub const ECONNREFUSED: u16 = 107;
- pub const ECONNRESET: u16 = 108;
- pub const ENOTCONN: u16 = 126;
- pub const ENOTSUP: u16 = 129;
- pub const ETIMEDOUT: u16 = 138;
- pub const EWOULDBLOCK: u16 = 140;
-}
-
impl From<Errno> for io::Error {
#[cfg(unix)]
fn from(value: Errno) -> io::Error {
@@ -52,13 +26,11 @@ fn from(value: Errno) -> io::Error {
#[cfg(windows)]
fn from(value: Errno) -> io::Error {
let Errno(errno) = value;
- let error_kind = match errno {
+ let error_kind = match errno as i32 {
libc::EPERM | libc::EACCES => ErrorKind::PermissionDenied,
libc::ENOENT => ErrorKind::NotFound,
libc::EINTR => ErrorKind::Interrupted,
- // Note that on Windows we know these two are distinct. In general,
- // it would not be possible to use "|".
- libc::EAGAIN | libc::EWOULDBLOCK => ErrorKind::WouldBlock,
+ x if x == libc::EAGAIN || x == libc::EWOULDBLOCK => ErrorKind::WouldBlock,
libc::ENOMEM => ErrorKind::OutOfMemory,
libc::EEXIST => ErrorKind::AlreadyExists,
libc::EINVAL => ErrorKind::InvalidInput,
diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
index e72bc7aa61e..00a1254ce8b 100755
--- a/scripts/archive-source.sh
+++ b/scripts/archive-source.sh
@@ -29,7 +29,7 @@ sub_file="${sub_tdir}/submodule.tar"
# different to the host OS.
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
+ bilge-impl-0.2-rs either-1-rs itertools-0.11-rs libc-0.2-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"
sub_deinit=""
diff --git a/scripts/make-release b/scripts/make-release
index 2acbfed8f2d..a53aebd1f1e 100755
--- a/scripts/make-release
+++ b/scripts/make-release
@@ -40,7 +40,7 @@ fi
# Only include wraps that are invoked with subproject()
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
+ bilge-impl-0.2-rs either-1-rs itertools-0.11-rs libc-0.2-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
syn-2-rs unicode-ident-1-rs"
diff --git a/subprojects/libc-0.2-rs.wrap b/subprojects/libc-0.2-rs.wrap
new file mode 100644
index 00000000000..0ce0e75291a
--- /dev/null
+++ b/subprojects/libc-0.2-rs.wrap
@@ -0,0 +1,7 @@
+[wrap-file]
+directory = libc-0.2
+source_url = https://crates.io/api/v1/crates/libc/0.2.162/download
+source_filename = libc-0.2.162.tar.gz
+source_hash = 18d287de67fe55fd7e1581fe933d965a5a9477b38e949cfa9f8574ef01506398
+#method = cargo
+patch_directory = libc-0.2-rs
diff --git a/subprojects/packagefiles/libc-0.2-rs/meson.build b/subprojects/packagefiles/libc-0.2-rs/meson.build
new file mode 100644
index 00000000000..11c4ada33a5
--- /dev/null
+++ b/subprojects/packagefiles/libc-0.2-rs/meson.build
@@ -0,0 +1,36 @@
+project('libc-0.2-rs', 'rust',
+ meson_version: '>=1.5.0',
+ version: '0.2.162',
+ license: 'MIT OR Apache-2.0',
+ default_options: [])
+
+_libc_rs = static_library(
+ 'libc',
+ files('src/lib.rs'),
+ gnu_symbol_visibility: 'hidden',
+ override_options: ['rust_std=2015', 'build.rust_std=2015'],
+ rust_abi: 'rust',
+ rust_args: [
+ '--cfg', 'freebsd11',
+ '--cfg', 'libc_priv_mod_use',
+ '--cfg', 'libc_union',
+ '--cfg', 'libc_const_size_of',
+ '--cfg', 'libc_align',
+ '--cfg', 'libc_int128',
+ '--cfg', 'libc_core_cvoid',
+ '--cfg', 'libc_packedN',
+ '--cfg', 'libc_cfg_target_vendor',
+ '--cfg', 'libc_non_exhaustive',
+ '--cfg', 'libc_long_array',
+ '--cfg', 'libc_ptr_addr_of',
+ '--cfg', 'libc_underscore_const_names',
+ '--cfg', 'libc_const_extern_fn',
+ ],
+ dependencies: [],
+)
+
+libc_dep = declare_dependency(
+ link_with: _libc_rs,
+)
+
+meson.override_dependency('libc-0.2-rs', libc_dep)
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-12 17:16 ` Paolo Bonzini
@ 2025-02-12 19:52 ` Kevin Wolf
2025-02-13 11:06 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 19:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 18:16 hat Paolo Bonzini geschrieben:
> On 2/12/25 16:13, Kevin Wolf wrote:
> > Or if you have to define the constants anyway - you currently do this
> > only for Windows, but for into_negative_errno() you might need it on
> > Linux, too - and it wouldn't be a problem for the constants to be
> > signed (that they are unsigned is the main reason why it becomes so ugly
> > with the bindgen constants), you could just make it -errno::EINVAL
> > again.
>
> Or just include the libc crate, see attachment.
I assume that sooner or later we'll have a reason to include it anyway,
so that might honestly be the best option.
Do you want to post it as a proper patch? It seems to depend on your
errno patch, but that shouldn't be a problem because we still want that
one, too.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 03/11] rust: Add some block layer bindings
2025-02-12 19:52 ` Kevin Wolf
@ 2025-02-13 11:06 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-13 11:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On Wed, Feb 12, 2025 at 8:52 PM Kevin Wolf <kwolf@redhat.com> wrote:
> I assume that sooner or later we'll have a reason to include it anyway,
> so that might honestly be the best option.
Sounds good.
> Do you want to post it as a proper patch? It seems to depend on your
> errno patch, but that shouldn't be a problem because we still want that
> one, too.
Yes, I wrote it as an addendum, as a kind of "let's see later what's
best"; that's also why the patch I posted uses "mod libc" for the
errno values. (The attachment gives away the date, too :)). I'll
clean up everything and add errno::into_neg_errno(); it should take
both an unsigned Ok value or Ok(()), and will return i32 or i64.
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (2 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 03/11] rust: Add some block layer bindings Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 9:28 ` Paolo Bonzini
2025-02-11 21:43 ` [PATCH 05/11] rust/block: Add empty crate Kevin Wolf
` (6 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
This adds helper functions that allow running Rust futures to completion
using QEMU's event loops.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/qemu/coroutine-rust.h | 24 +++++++++++
rust/wrapper.h | 1 +
util/qemu-co-rust-async.c | 55 ++++++++++++++++++++++++
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/futures.rs | 78 +++++++++++++++++++++++++++++++++++
rust/qemu-api/src/lib.rs | 1 +
util/meson.build | 3 ++
7 files changed, 163 insertions(+)
create mode 100644 include/qemu/coroutine-rust.h
create mode 100644 util/qemu-co-rust-async.c
create mode 100644 rust/qemu-api/src/futures.rs
diff --git a/include/qemu/coroutine-rust.h b/include/qemu/coroutine-rust.h
new file mode 100644
index 0000000000..0c5cf42a6b
--- /dev/null
+++ b/include/qemu/coroutine-rust.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Helpers to run Rust futures using QEMU coroutines
+ *
+ * Copyright Red Hat
+ *
+ * Author:
+ * Kevin Wolf <kwolf@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef QEMU_COROUTINE_RUST_H
+#define QEMU_COROUTINE_RUST_H
+
+typedef struct RustBoxedFuture RustBoxedFuture;
+typedef void coroutine_fn RunFuture(RustBoxedFuture *future, void *opaque);
+
+void no_coroutine_fn rust_run_future(RustBoxedFuture *future,
+ RunFuture *entry,
+ void *opaque);
+
+#endif
diff --git a/rust/wrapper.h b/rust/wrapper.h
index c3e1e6f9cf..61afac0494 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -57,3 +57,4 @@ typedef enum memory_order {
#include "block/block_int.h"
#include "block/qdict.h"
#include "qapi/qapi-visit-block-core.h"
+#include "qemu/coroutine-rust.h"
diff --git a/util/qemu-co-rust-async.c b/util/qemu-co-rust-async.c
new file mode 100644
index 0000000000..d893dfb7bd
--- /dev/null
+++ b/util/qemu-co-rust-async.c
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Helpers to run Rust futures using QEMU coroutines
+ *
+ * Copyright Red Hat
+ *
+ * Author:
+ * Kevin Wolf <kwolf@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/aio-wait.h"
+#include "qemu/coroutine.h"
+#include "qemu/coroutine-rust.h"
+#include "qemu/main-loop.h"
+
+typedef struct FutureCo {
+ RustBoxedFuture *future;
+ RunFuture *entry;
+ void *opaque;
+ bool done;
+} FutureCo;
+
+static void coroutine_fn rust_co_run_future_entry(void *opaque)
+{
+ FutureCo *data = opaque;
+
+ data->entry(data->future, data->opaque);
+ data->done = true;
+ aio_wait_kick();
+}
+
+void no_coroutine_fn rust_run_future(RustBoxedFuture *future,
+ RunFuture *entry,
+ void *opaque)
+{
+ AioContext *ctx = qemu_get_current_aio_context();
+ Coroutine *co;
+ FutureCo data = {
+ .future = future,
+ .entry = entry,
+ .opaque = opaque,
+ .done = false,
+ };
+
+ GLOBAL_STATE_CODE();
+
+ co = qemu_coroutine_create(rust_co_run_future_entry, &data);
+ aio_co_enter(ctx, co);
+ AIO_WAIT_WHILE(ctx, !data.done);
+}
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index acac384936..713812bc2f 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -18,6 +18,7 @@ sources_core = [
'src/callbacks.rs',
'src/cell.rs',
'src/c_str.rs',
+ 'src/futures.rs',
'src/module.rs',
'src/offset_of.rs',
'src/prelude.rs',
diff --git a/rust/qemu-api/src/futures.rs b/rust/qemu-api/src/futures.rs
new file mode 100644
index 0000000000..485041fd98
--- /dev/null
+++ b/rust/qemu-api/src/futures.rs
@@ -0,0 +1,78 @@
+use crate::bindings;
+use std::ffi::c_void;
+use std::future::Future;
+use std::mem::MaybeUninit;
+use std::sync::Arc;
+use std::task::{Context, Poll, Wake};
+
+struct RunFutureWaker {
+ co: *mut bindings::Coroutine,
+}
+unsafe impl Send for RunFutureWaker {}
+unsafe impl Sync for RunFutureWaker {}
+
+impl Wake for RunFutureWaker {
+ fn wake(self: Arc<Self>) {
+ unsafe {
+ bindings::aio_co_wake(self.co);
+ }
+ }
+}
+
+/// Use QEMU's event loops to run a Rust [`Future`] to completion and return its result.
+///
+/// This function must be called in coroutine context. If the future isn't ready yet, it yields.
+pub fn qemu_co_run_future<F: Future>(future: F) -> F::Output {
+ let waker = Arc::new(RunFutureWaker {
+ co: unsafe { bindings::qemu_coroutine_self() },
+ })
+ .into();
+ let mut cx = Context::from_waker(&waker);
+
+ let mut pinned_future = std::pin::pin!(future);
+ loop {
+ match pinned_future.as_mut().poll(&mut cx) {
+ Poll::Ready(res) => return res,
+ Poll::Pending => unsafe {
+ bindings::qemu_coroutine_yield();
+ },
+ }
+ }
+}
+
+/// Wrapper around [`qemu_co_run_future`] that can be called from C.
+///
+/// # Safety
+///
+/// `future` must be a valid pointer to an owned `F` (it will be freed in this function). `output`
+/// must be a valid pointer representing a mutable reference to an `F::Output` where the result can
+/// be stored.
+unsafe extern "C" fn rust_co_run_future<F: Future>(
+ future: *mut bindings::RustBoxedFuture,
+ output: *mut c_void,
+) {
+ let future = unsafe { Box::from_raw(future.cast::<F>()) };
+ let output = output.cast::<F::Output>();
+ let ret = qemu_co_run_future(*future);
+ unsafe {
+ *output = ret;
+ }
+}
+
+/// Use QEMU's event loops to run a Rust [`Future`] to completion and return its result.
+///
+/// This function must be called outside of coroutine context to avoid deadlocks. It blocks and
+/// runs a nested even loop until the future is ready and returns a result.
+pub fn qemu_run_future<F: Future>(future: F) -> F::Output {
+ let future_ptr = Box::into_raw(Box::new(future));
+ let mut output = MaybeUninit::<F::Output>::uninit();
+ unsafe {
+ bindings::rust_run_future(
+ future_ptr.cast::<bindings::RustBoxedFuture>(),
+ #[allow(clippy::as_underscore)]
+ Some(rust_co_run_future::<F> as _),
+ output.as_mut_ptr().cast::<c_void>(),
+ );
+ output.assume_init()
+ }
+}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 3c6c154f3d..9b8f5fa4f1 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -18,6 +18,7 @@
pub mod c_str;
pub mod callbacks;
pub mod cell;
+pub mod futures;
#[cfg(feature = "system")]
pub mod irq;
pub mod module;
diff --git a/util/meson.build b/util/meson.build
index 780b5977a8..14a2ae17fd 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -101,6 +101,9 @@ if have_block
util_ss.add(files('qemu-coroutine-sleep.c'))
util_ss.add(files('qemu-co-shared-resource.c'))
util_ss.add(files('qemu-co-timeout.c'))
+ if have_rust
+ util_ss.add(files('qemu-co-rust-async.c'))
+ endif
util_ss.add(files('readline.c'))
util_ss.add(files('throttle.c'))
util_ss.add(files('timed-average.c'))
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
2025-02-11 21:43 ` [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU Kevin Wolf
@ 2025-02-12 9:28 ` Paolo Bonzini
2025-02-12 12:47 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 9:28 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/11/25 22:43, Kevin Wolf wrote:
> +/// Use QEMU's event loops to run a Rust [`Future`] to completion and return its result.
> +///
> +/// This function must be called in coroutine context. If the future isn't ready yet, it yields.
> +pub fn qemu_co_run_future<F: Future>(future: F) -> F::Output {
> + let waker = Arc::new(RunFutureWaker {
> + co: unsafe { bindings::qemu_coroutine_self() },
> + })
> + .into();
into what? :) Maybe you can add the type to the "let" for clarity.
> + let mut cx = Context::from_waker(&waker);
> +
> + let mut pinned_future = std::pin::pin!(future);
> + loop {
> + match pinned_future.as_mut().poll(&mut cx) {
> + Poll::Ready(res) => return res,
Alternatively, "break res" (matter of taste).
> + Poll::Pending => unsafe {
> + bindings::qemu_coroutine_yield();
> + },
> + }
> + }
> +}
> +/// Wrapper around [`qemu_co_run_future`] that can be called from C.
> +///
> +/// # Safety
> +///
> +/// `future` must be a valid pointer to an owned `F` (it will be freed in this function). `output`
> +/// must be a valid pointer representing a mutable reference to an `F::Output` where the result can
> +/// be stored.
> +unsafe extern "C" fn rust_co_run_future<F: Future>(
> + future: *mut bindings::RustBoxedFuture,
> + output: *mut c_void,
> +) {
> + let future = unsafe { Box::from_raw(future.cast::<F>()) };
> + let output = output.cast::<F::Output>();
> + let ret = qemu_co_run_future(*future);
> + unsafe {
> + *output = ret;
This should use output.write(ret), to ensure that the output is written
without dropping the previous value.
Also, would qemu_co_run_future() and qemu_run_future() become methods on
an Executor later? Maybe it make sense to have already something like
pub trait QemuExecutor {
fn run_until<F: Future>(future: F) -> F::Output;
}
pub struct Executor;
pub struct CoExecutor;
and pass an executor to Rust functions (&Executor for no_coroutine_fn,
&CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would
that be premature in your opinion?
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
2025-02-12 9:28 ` Paolo Bonzini
@ 2025-02-12 12:47 ` Kevin Wolf
2025-02-12 13:22 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 12:47 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 10:28 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > +/// Use QEMU's event loops to run a Rust [`Future`] to completion and return its result.
> > +///
> > +/// This function must be called in coroutine context. If the future isn't ready yet, it yields.
> > +pub fn qemu_co_run_future<F: Future>(future: F) -> F::Output {
> > + let waker = Arc::new(RunFutureWaker {
> > + co: unsafe { bindings::qemu_coroutine_self() },
> > + })
> > + .into();
>
> into what? :) Maybe you can add the type to the "let" for clarity.
Into Waker. Do we have any idea yet how explicit we want to be with type
annotations that aren't strictly necessary? I didn't think of making it
explicit here because the only thing that is done with it is building a
Context from it, so it seemed obvious enough.
If you think that being explicit is better, then Waker::from() might
be better than adding a type name to let and keeping .into().
> > + let mut cx = Context::from_waker(&waker);
> > +
> > + let mut pinned_future = std::pin::pin!(future);
> > + loop {
> > + match pinned_future.as_mut().poll(&mut cx) {
> > + Poll::Ready(res) => return res,
>
> Alternatively, "break res" (matter of taste).
>
> > + Poll::Pending => unsafe {
> > + bindings::qemu_coroutine_yield();
> > + },
> > + }
> > + }
> > +}
> > +/// Wrapper around [`qemu_co_run_future`] that can be called from C.
> > +///
> > +/// # Safety
> > +///
> > +/// `future` must be a valid pointer to an owned `F` (it will be freed in this function). `output`
> > +/// must be a valid pointer representing a mutable reference to an `F::Output` where the result can
> > +/// be stored.
> > +unsafe extern "C" fn rust_co_run_future<F: Future>(
> > + future: *mut bindings::RustBoxedFuture,
> > + output: *mut c_void,
> > +) {
> > + let future = unsafe { Box::from_raw(future.cast::<F>()) };
> > + let output = output.cast::<F::Output>();
> > + let ret = qemu_co_run_future(*future);
> > + unsafe {
> > + *output = ret;
>
> This should use output.write(ret), to ensure that the output is written
> without dropping the previous value.
Oops, thanks. I need to learn that unsafe Rust still isn't C. :-)
> Also, would qemu_co_run_future() and qemu_run_future() become methods on an
> Executor later? Maybe it make sense to have already something like
>
> pub trait QemuExecutor {
> fn run_until<F: Future>(future: F) -> F::Output;
> }
>
> pub struct Executor;
> pub struct CoExecutor;
>
> and pass an executor to Rust functions (&Executor for no_coroutine_fn,
> &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would that
> be premature in your opinion?
If we could get bindgen to actually do that for the C interface, then
this could be interesting, though for most functions it also would
remain unused boilerplate. If we have to get the executor manually on
the Rust side for each function, that's probably the same function that
will want to execute the future - in which case it just can directly
call the correct function.
The long term idea should be anyway that C coroutines disappear from the
path and we integrate an executor into the QEMU main loop. But as long
as the block layer core is written in C and uses coroutines and we want
Rust futures to be able to call into coroutine_fns, that's still a long
way to go.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
2025-02-12 12:47 ` Kevin Wolf
@ 2025-02-12 13:22 ` Paolo Bonzini
2025-02-18 17:25 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 13:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On Wed, Feb 12, 2025 at 1:47 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > > +pub fn qemu_co_run_future<F: Future>(future: F) -> F::Output {
> > > + let waker = Arc::new(RunFutureWaker {
> > > + co: unsafe { bindings::qemu_coroutine_self() },
> > > + })
> > > + .into();
> >
> > into what? :) Maybe you can add the type to the "let" for clarity.
>
> Into Waker. Do we have any idea yet how explicit we want to be with type
> annotations that aren't strictly necessary? I didn't think of making it
> explicit here because the only thing that is done with it is building a
> Context from it, so it seemed obvious enough.
>
> If you think that being explicit is better, then Waker::from() might
> be better than adding a type name to let and keeping .into().
I think this case threw me off because From<Arc<W: Wake>> is a bit
more generic than your
usual From implementation. Maybe it's obvious to anyone who has used
futures in Rust
(I haven't).
I agree, something like
let waker = Waker::from(waker);
let mut cx = Context::from_waker(&waker);
could be the best of both worlds.
> > Also, would qemu_co_run_future() and qemu_run_future() become methods on an
> > Executor later? Maybe it make sense to have already something like
> >
> > pub trait QemuExecutor {
> > fn run_until<F: Future>(future: F) -> F::Output;
> > }
> >
> > pub struct Executor;
> > pub struct CoExecutor;
> >
> > and pass an executor to Rust functions (&Executor for no_coroutine_fn,
> > &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would that
> > be premature in your opinion?
>
> If we could get bindgen to actually do that for the C interface, then
> this could be interesting, though for most functions it also would
> remain unused boilerplate. If we have to get the executor manually on
> the Rust side for each function, that's probably the same function that
> will want to execute the future - in which case it just can directly
> call the correct function.
The idea was that you don't call the correct function but the *only*
function :) i.e. exec.run_until(), and it will do the right thing for
coroutine vs. no coroutine.
But yeah, there would be boilerplate to pass it all the way down so
maybe it is not a great idea. I liked the concept that you just
*couldn't* get _co_ wrong... but perhaps it is not necessary once more
of "BlockDriver::open"
is lifted into bdrv_open<D: BlockDriver>.
Paolo
> The long term idea should be anyway that C coroutines disappear from the
> path and we integrate an executor into the QEMU main loop. But as long
> as the block layer core is written in C and uses coroutines and we want
> Rust futures to be able to call into coroutine_fns, that's still a long
> way to go.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
2025-02-12 13:22 ` Paolo Bonzini
@ 2025-02-18 17:25 ` Kevin Wolf
0 siblings, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2025-02-18 17:25 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 14:22 hat Paolo Bonzini geschrieben:
> > > Also, would qemu_co_run_future() and qemu_run_future() become methods on an
> > > Executor later? Maybe it make sense to have already something like
> > >
> > > pub trait QemuExecutor {
> > > fn run_until<F: Future>(future: F) -> F::Output;
> > > }
> > >
> > > pub struct Executor;
> > > pub struct CoExecutor;
> > >
> > > and pass an executor to Rust functions (&Executor for no_coroutine_fn,
> > > &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would that
> > > be premature in your opinion?
> >
> > If we could get bindgen to actually do that for the C interface, then
> > this could be interesting, though for most functions it also would
> > remain unused boilerplate. If we have to get the executor manually on
> > the Rust side for each function, that's probably the same function that
> > will want to execute the future - in which case it just can directly
> > call the correct function.
>
> The idea was that you don't call the correct function but the *only*
> function :) i.e. exec.run_until(), and it will do the right thing for
> coroutine vs. no coroutine.
>
> But yeah, there would be boilerplate to pass it all the way down so
> maybe it is not a great idea. I liked the concept that you just
> *couldn't* get _co_ wrong... but perhaps it is not necessary once more
> of "BlockDriver::open"
> is lifted into bdrv_open<D: BlockDriver>.
Yes, my assumption is that in the final state there is no "all the way
down" because the function wanting to run a future will be the outermost
Rust function. At any other level, the Rust function can just be async
itself.
That's why I said that calling the only function of the correct executor
isn't really any better than directly calling the correct function.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 05/11] rust/block: Add empty crate
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (3 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-11 21:43 ` [PATCH 06/11] rust/block: Add I/O buffer traits Kevin Wolf
` (5 subsequent siblings)
10 siblings, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/Cargo.lock | 7 +++++++
rust/Cargo.toml | 1 +
rust/block/Cargo.toml | 15 +++++++++++++++
rust/block/README.md | 3 +++
rust/block/meson.build | 19 +++++++++++++++++++
rust/block/src/lib.rs | 1 +
rust/meson.build | 1 +
7 files changed, 47 insertions(+)
create mode 100644 rust/block/Cargo.toml
create mode 100644 rust/block/README.md
create mode 100644 rust/block/meson.build
create mode 100644 rust/block/src/lib.rs
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index c0c6069247..b6af988a03 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -31,6 +31,13 @@ dependencies = [
"syn",
]
+[[package]]
+name = "block"
+version = "0.1.0"
+dependencies = [
+ "qemu_api",
+]
+
[[package]]
name = "either"
version = "1.12.0"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 5b0cb55928..777a2e9157 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -3,6 +3,7 @@ resolver = "2"
members = [
"qemu-api-macros",
"qemu-api",
+ "block",
"hw/char/pl011",
]
diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
new file mode 100644
index 0000000000..70ee02f429
--- /dev/null
+++ b/rust/block/Cargo.toml
@@ -0,0 +1,15 @@
+[package]
+name = "block"
+version = "0.1.0"
+edition = "2021"
+authors = ["Kevin Wolf <kwolf@redhat.com>"]
+license = "GPL-2.0-or-later"
+readme = "README.md"
+description = "Block backends for QEMU"
+repository = "https://gitlab.com/qemu-project/qemu/"
+publish = false
+keywords = []
+categories = []
+
+[dependencies]
+qemu_api = { path = "../qemu-api" }
diff --git a/rust/block/README.md b/rust/block/README.md
new file mode 100644
index 0000000000..debcc9d815
--- /dev/null
+++ b/rust/block/README.md
@@ -0,0 +1,3 @@
+# QEMU block backends
+
+This library implements block drivers for QEMU.
diff --git a/rust/block/meson.build b/rust/block/meson.build
new file mode 100644
index 0000000000..b13c037b2f
--- /dev/null
+++ b/rust/block/meson.build
@@ -0,0 +1,19 @@
+_block_rs = static_library(
+ 'block',
+ files('src/lib.rs'),
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ dependencies: [
+ qemu_api_tools,
+ qemu_api_macros,
+ ],
+ rust_dependency_map: {'qemu_api_tools': 'qemu_api'},
+)
+
+rust_block_ss.add(if_true: [declare_dependency(
+ link_whole: [_block_rs],
+ # Putting proc macro crates in `dependencies` is necessary for Meson to find
+ # them when compiling the root per-target static rust lib.
+ dependencies: [qemu_api_macros],
+ variables: {'crate': 'block'},
+)])
diff --git a/rust/block/src/lib.rs b/rust/block/src/lib.rs
new file mode 100644
index 0000000000..8b13789179
--- /dev/null
+++ b/rust/block/src/lib.rs
@@ -0,0 +1 @@
+
diff --git a/rust/meson.build b/rust/meson.build
index 50eb23b072..d959809fda 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -1,6 +1,7 @@
subdir('qemu-api-macros')
subdir('qemu-api')
+subdir('block')
subdir('hw')
cargo = find_program('cargo', required: false)
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 06/11] rust/block: Add I/O buffer traits
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (4 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 05/11] rust/block: Add empty crate Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 16:48 ` Paolo Bonzini
2025-02-11 21:43 ` [PATCH 07/11] block: Add bdrv_open_blockdev_ref_file() Kevin Wolf
` (4 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
Types that implement IoBuffer can be used with safe I/O functions.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/block/src/iobuffer.rs | 94 ++++++++++++++++++++++++++++++++++++++
rust/block/src/lib.rs | 2 +
2 files changed, 96 insertions(+)
create mode 100644 rust/block/src/iobuffer.rs
diff --git a/rust/block/src/iobuffer.rs b/rust/block/src/iobuffer.rs
new file mode 100644
index 0000000000..d61370c961
--- /dev/null
+++ b/rust/block/src/iobuffer.rs
@@ -0,0 +1,94 @@
+// Copyright Red Hat Inc.
+// Author(s): Kevin Wolf <kwolf@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use std::mem::MaybeUninit;
+
+/// Types that implement IoBuffer can be used with safe I/O functions.
+///
+/// # Safety
+///
+/// `buffer_ptr()` and `buffer_mut_ptr()` must return pointers to the address of the same I/O
+/// buffer with the size returned by `buffer_len()` which remain valid for the lifetime of the
+/// object. It must be safe for the I/O buffer to contain any byte patterns.
+pub unsafe trait IoBuffer {
+ /// Returns a const pointer to be used as a raw I/O buffer
+ fn buffer_ptr(&self) -> *const u8;
+
+ /// Returns a mutable pointer to be used as a raw I/O buffer
+ fn buffer_mut_ptr(&mut self) -> *mut u8;
+
+ /// Returns the length in bytes for the raw I/O buffer returned by [`buffer_ptr`] and
+ /// [`buffer_mut_ptr`]
+ ///
+ /// [`buffer_ptr`]: IoBuffer::buffer_ptr
+ /// [`buffer_mut_ptr`]: IoBuffer::buffer_mut_ptr
+ fn buffer_len(&self) -> usize;
+}
+
+/// Implementing `SizedIoBuffer` provides an implementation for [`IoBuffer`] without having to
+/// implement any functions manually.
+///
+/// # Safety
+///
+/// Types implementing `SizedIoBuffer` guarantee that the whole object can be accessed as an I/O
+/// buffer that is safe to contain any byte patterns.
+pub unsafe trait SizedIoBuffer: Sized {
+ /// Safely converts a byte slice into a shared reference to the type implementing
+ /// `SizedIoBuffer`
+ fn from_byte_slice(buf: &[u8]) -> Option<&Self> {
+ if buf.len() < std::mem::size_of::<Self>() {
+ return None;
+ }
+
+ let ptr = buf.as_ptr() as *const Self;
+
+ // TODO Use ptr.is_aligned() when MSRV is updated to at least 1.79.0
+ if (ptr as usize) % std::mem::align_of::<Self>() != 0 {
+ return None;
+ }
+
+ // SAFETY: This function checked that the byte slice is large enough and aligned.
+ // Implementing SizedIoBuffer promises that any byte pattern is valid for the type.
+ Some(unsafe { &*ptr })
+ }
+}
+
+unsafe impl<T: SizedIoBuffer> IoBuffer for T {
+ fn buffer_ptr(&self) -> *const u8 {
+ self as *const Self as *const u8
+ }
+
+ fn buffer_mut_ptr(&mut self) -> *mut u8 {
+ self as *mut Self as *mut u8
+ }
+
+ fn buffer_len(&self) -> usize {
+ std::mem::size_of::<Self>()
+ }
+}
+
+unsafe impl<T: SizedIoBuffer> IoBuffer for [T] {
+ fn buffer_ptr(&self) -> *const u8 {
+ self.as_ptr() as *const u8
+ }
+
+ fn buffer_mut_ptr(&mut self) -> *mut u8 {
+ self.as_mut_ptr() as *mut u8
+ }
+
+ fn buffer_len(&self) -> usize {
+ std::mem::size_of_val(self)
+ }
+}
+
+unsafe impl<T: SizedIoBuffer> SizedIoBuffer for MaybeUninit<T> {}
+
+unsafe impl SizedIoBuffer for u8 {}
+unsafe impl SizedIoBuffer for u16 {}
+unsafe impl SizedIoBuffer for u32 {}
+unsafe impl SizedIoBuffer for u64 {}
+unsafe impl SizedIoBuffer for i8 {}
+unsafe impl SizedIoBuffer for i16 {}
+unsafe impl SizedIoBuffer for i32 {}
+unsafe impl SizedIoBuffer for i64 {}
diff --git a/rust/block/src/lib.rs b/rust/block/src/lib.rs
index 8b13789179..1c03549821 100644
--- a/rust/block/src/lib.rs
+++ b/rust/block/src/lib.rs
@@ -1 +1,3 @@
+mod iobuffer;
+pub use iobuffer::{IoBuffer, SizedIoBuffer};
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 06/11] rust/block: Add I/O buffer traits
2025-02-11 21:43 ` [PATCH 06/11] rust/block: Add I/O buffer traits Kevin Wolf
@ 2025-02-12 16:48 ` Paolo Bonzini
2025-02-12 17:22 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 16:48 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/11/25 22:43, Kevin Wolf wrote:
> +/// Implementing `SizedIoBuffer` provides an implementation for [`IoBuffer`] without having to
> +/// implement any functions manually.
> +///
> +/// # Safety
> +///
> +/// Types implementing `SizedIoBuffer` guarantee that the whole object can be accessed as an I/O
> +/// buffer that is safe to contain any byte patterns.
> +pub unsafe trait SizedIoBuffer: Sized {
This is similar to the ByteValued trait in rust-vmm. Can you name it
the same so that we can later consider replacing it?
> + fn from_byte_slice(buf: &[u8]) -> Option<&Self> {
> + if buf.len() < std::mem::size_of::<Self>() {
> + return None;
> + }
> +
> + let ptr = buf.as_ptr() as *const Self;
> +
> + // TODO Use ptr.is_aligned() when MSRV is updated to at least 1.79.0
> + if (ptr as usize) % std::mem::align_of::<Self>() != 0 {
> + return None;
> + }
> +
> + // SAFETY: This function checked that the byte slice is large enough and aligned.
> + // Implementing SizedIoBuffer promises that any byte pattern is valid for the type.
> + Some(unsafe { &*ptr })
If you want, the function can be written also
// SAFETY: implementing SizedIoBuffer promises that any byte pattern
// is valid for the type
match unsafe { buf.align_to::<Self>() } {
([], mid, _) => mid.get(0),
_ => None
}
(trick stolen from rust-vmm, in fact).
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 06/11] rust/block: Add I/O buffer traits
2025-02-12 16:48 ` Paolo Bonzini
@ 2025-02-12 17:22 ` Kevin Wolf
2025-02-12 17:41 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 17:22 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 17:48 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > +/// Implementing `SizedIoBuffer` provides an implementation for [`IoBuffer`] without having to
> > +/// implement any functions manually.
> > +///
> > +/// # Safety
> > +///
> > +/// Types implementing `SizedIoBuffer` guarantee that the whole object can be accessed as an I/O
> > +/// buffer that is safe to contain any byte patterns.
> > +pub unsafe trait SizedIoBuffer: Sized {
>
> This is similar to the ByteValued trait in rust-vmm. Can you name it
> the same so that we can later consider replacing it?
I'm not sure if it's the best name, but could be done, of course.
Though the more interesting thing to replace it with eventually might be
the zerocopy crate which has derive macros that check that the condition
is actually fulfilled. I just didn't feel like bringing in new external
dependencies in this first series.
> > + fn from_byte_slice(buf: &[u8]) -> Option<&Self> {
> > + if buf.len() < std::mem::size_of::<Self>() {
> > + return None;
> > + }
This is a semantic difference compared to ByteValued::from_slice(),
which requires the sizes to match exactly. For the probe function, I
actually make use of the relaxed requirement here to access a header
struct in a larger buffer passed from C.
> > + let ptr = buf.as_ptr() as *const Self;
> > +
> > + // TODO Use ptr.is_aligned() when MSRV is updated to at least 1.79.0
> > + if (ptr as usize) % std::mem::align_of::<Self>() != 0 {
> > + return None;
> > + }
> > +
> > + // SAFETY: This function checked that the byte slice is large enough and aligned.
> > + // Implementing SizedIoBuffer promises that any byte pattern is valid for the type.
> > + Some(unsafe { &*ptr })
>
> If you want, the function can be written also
>
> // SAFETY: implementing SizedIoBuffer promises that any byte pattern
> // is valid for the type
> match unsafe { buf.align_to::<Self>() } {
> ([], mid, _) => mid.get(0),
> _ => None
> }
>
> (trick stolen from rust-vmm, in fact).
Clever way to avoid ptr::is_aligned(), but I feel a bit harder to
understand than just open-coding it like above? (And probably less
efficient, but I don't know how relevant that is.)
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 06/11] rust/block: Add I/O buffer traits
2025-02-12 17:22 ` Kevin Wolf
@ 2025-02-12 17:41 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 17:41 UTC (permalink / raw)
To: Kevin Wolf
Cc: open list:Block layer core, Hanna Czenczek,
Emmanouil Pitsidianakis, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]
Il mer 12 feb 2025, 18:23 Kevin Wolf <kwolf@redhat.com> ha scritto:
> Am 12.02.2025 um 17:48 hat Paolo Bonzini geschrieben:
> > On 2/11/25 22:43, Kevin Wolf wrote:
> > > +/// Implementing `SizedIoBuffer` provides an implementation for
> [`IoBuffer`] without having to
> > > +/// implement any functions manually.
> > > +///
> > > +/// # Safety
> > > +///
> > > +/// Types implementing `SizedIoBuffer` guarantee that the whole
> object can be accessed as an I/O
> > > +/// buffer that is safe to contain any byte patterns.
> > > +pub unsafe trait SizedIoBuffer: Sized {
> >
> > This is similar to the ByteValued trait in rust-vmm. Can you name it
> > the same so that we can later consider replacing it?
>
> I'm not sure if it's the best name, but could be done, of course.
>
> Though the more interesting thing to replace it with eventually might be
> the zerocopy crate which has derive macros that check that the condition
> is actually fulfilled. I just didn't feel like bringing in new external
> dependencies in this first series.
>
Good idea though. zerocopy has no extra dependencies, and I agree that
sooner or later we're going to include it, so you might as well go for it.
The build.rs file is ludicrously overengineered, but converting it to meson
should be easy.
> > + fn from_byte_slice(buf: &[u8]) -> Option<&Self> {
> > > + if buf.len() < std::mem::size_of::<Self>() {
> > > + return None;
> > > + }
>
> This is a semantic difference compared to ByteValued::from_slice(),
> which requires the sizes to match exactly. For the probe function, I
> actually make use of the relaxed requirement here to access a header
> struct in a larger buffer passed from C.
>
Indeed it's similar but not the same. I haven't checked how you'd write it
with vm-memory (it could be hdr.as_bytes().read_obj(0), or maybe there's
something better), but it's something that could be added there too.
> If you want, the function can be written also
> >
> > // SAFETY: implementing SizedIoBuffer promises that any byte pattern
> > // is valid for the type
> > match unsafe { buf.align_to::<Self>() } {
> > ([], mid, _) => mid.get(0),
> > _ => None
> > }
> >
> > (trick stolen from rust-vmm, in fact).
>
> Clever way to avoid ptr::is_aligned(), but I feel a bit harder to
> understand than just open-coding it like above? (And probably less
> efficient, but I don't know how relevant that is.)
>
Probably not much and a lot of dead code elimination can happen, but either
way is fine of course.
Paolo
> Kevin
>
>
[-- Attachment #2: Type: text/html, Size: 4244 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 07/11] block: Add bdrv_open_blockdev_ref_file()
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (5 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 06/11] rust/block: Add I/O buffer traits Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 7:43 ` Philippe Mathieu-Daudé
2025-02-11 21:43 ` [PATCH 08/11] rust/block: Add driver module Kevin Wolf
` (3 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
This is the equivalent of bdrv_open_file_child() to be used in cases
where the caller is QAPI based and has a BlockdevRef rather than a
filename and an options QDict.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 4 ++++
block.c | 30 +++++++++++++++++++++++++++---
2 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 9be34b3c99..861e19cfe6 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -100,6 +100,10 @@ bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
BlockDriverState * coroutine_fn no_co_wrapper
bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+BlockDriverState * no_coroutine_fn
+bdrv_open_blockdev_ref_file(BlockdevRef *ref, BlockDriverState *parent,
+ Error **errp);
+
int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp);
int GRAPH_WRLOCK
diff --git a/block.c b/block.c
index 0ece805e41..6315be79d8 100644
--- a/block.c
+++ b/block.c
@@ -3847,7 +3847,11 @@ int bdrv_open_file_child(const char *filename,
* TODO Future callers may need to specify parent/child_class in order for
* option inheritance to work. Existing callers use it for the root node.
*/
-BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
+static BlockDriverState * no_coroutine_fn
+bdrv_open_blockdev_ref_common(BlockdevRef *ref, BlockDriverState *parent,
+ const BdrvChildClass *child_class,
+ BdrvChildRole child_role, bool parse_filename,
+ Error **errp)
{
BlockDriverState *bs = NULL;
QObject *obj = NULL;
@@ -3880,14 +3884,34 @@ BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
}
- bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, 0, false,
- errp);
+ bs = bdrv_open_inherit(NULL, reference, qdict, 0, parent, child_class,
+ child_role, parse_filename, errp);
obj = NULL;
qobject_unref(obj);
visit_free(v);
return bs;
}
+BlockDriverState *bdrv_open_blockdev_ref_file(BlockdevRef *ref,
+ BlockDriverState *parent,
+ Error **errp)
+{
+ BdrvChildRole role;
+
+ /* commit_top and mirror_top don't use this function */
+ assert(!parent->drv->filtered_child_is_backing);
+ role = parent->drv->is_filter ?
+ (BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY) : BDRV_CHILD_IMAGE;
+
+ return bdrv_open_blockdev_ref_common(ref, parent, &child_of_bds, role,
+ true, errp);
+}
+
+BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
+{
+ return bdrv_open_blockdev_ref_common(ref, NULL, NULL, 0, false, errp);
+}
+
static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
int flags,
QDict *snapshot_options,
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 07/11] block: Add bdrv_open_blockdev_ref_file()
2025-02-11 21:43 ` [PATCH 07/11] block: Add bdrv_open_blockdev_ref_file() Kevin Wolf
@ 2025-02-12 7:43 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 7:43 UTC (permalink / raw)
To: Kevin Wolf, qemu-block
Cc: hreitz, pbonzini, manos.pitsidianakis, qemu-devel, qemu-rust
On 11/2/25 22:43, Kevin Wolf wrote:
> This is the equivalent of bdrv_open_file_child() to be used in cases
> where the caller is QAPI based and has a BlockdevRef rather than a
> filename and an options QDict.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/block-global-state.h | 4 ++++
> block.c | 30 +++++++++++++++++++++++++++---
> 2 files changed, 31 insertions(+), 3 deletions(-)
> +BlockDriverState *bdrv_open_blockdev_ref_file(BlockdevRef *ref,
> + BlockDriverState *parent,
> + Error **errp)
> +{
> + BdrvChildRole role;
> +
> + /* commit_top and mirror_top don't use this function */
> + assert(!parent->drv->filtered_child_is_backing);
> + role = parent->drv->is_filter ?
> + (BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY) : BDRV_CHILD_IMAGE;
Nitpicking style:
role = parent->drv->is_filter
? (BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY)
: BDRV_CHILD_IMAGE;
> + return bdrv_open_blockdev_ref_common(ref, parent, &child_of_bds, role,
> + true, errp);
> +}
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 08/11] rust/block: Add driver module
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (6 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 07/11] block: Add bdrv_open_blockdev_ref_file() Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 16:43 ` Paolo Bonzini
2025-02-11 21:43 ` [PATCH 09/11] rust/block: Add read support for block drivers Kevin Wolf
` (2 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
This adds a barebones module for a block driver interface. Because there
is no native QAPI support for Rust yet, opening images takes a few
unsafe functions to call into C visitor functions. This should be
cleaned up later.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/block/src/driver.rs | 190 +++++++++++++++++++++++++++++++++++++++
rust/block/src/lib.rs | 1 +
2 files changed, 191 insertions(+)
create mode 100644 rust/block/src/driver.rs
diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
new file mode 100644
index 0000000000..5c7c46bfa0
--- /dev/null
+++ b/rust/block/src/driver.rs
@@ -0,0 +1,190 @@
+// Copyright Red Hat Inc.
+// Author(s): Kevin Wolf <kwolf@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+// All of this is unused until the first block driver is added
+#![allow(dead_code)]
+#![allow(unused_macros)]
+#![allow(unused_imports)]
+
+use crate::{IoBuffer, SizedIoBuffer};
+use qemu_api::bindings;
+use std::ffi::c_void;
+use std::io::{self, Error, ErrorKind};
+use std::mem::MaybeUninit;
+use std::ptr;
+
+/// A trait for writing block drivers.
+///
+/// Types that implement this trait can be registered as QEMU block drivers using the
+/// [`block_driver`] macro.
+pub trait BlockDriver {
+ /// The type that contains the block driver specific options for opening an image
+ type Options;
+
+ // TODO Native support for QAPI types and deserialization
+ unsafe fn parse_options(
+ v: &mut bindings::Visitor,
+ opts: &mut *mut Self::Options,
+ errp: *mut *mut bindings::Error,
+ );
+ unsafe fn free_options(opts: *mut Self::Options);
+ unsafe fn open(
+ bs: *mut bindings::BlockDriverState,
+ opts: &Self::Options,
+ errp: *mut *mut bindings::Error,
+ ) -> std::os::raw::c_int;
+
+ /// Returns the size of the image in bytes
+ fn size(&self) -> u64;
+}
+
+/// Represents the connection between a parent and its child node.
+///
+/// This is a wrapper around the `BdrvChild` type in C.
+pub struct BdrvChild {
+ child: *mut bindings::BdrvChild,
+}
+
+impl BdrvChild {
+ /// Creates a new child reference from a `BlockdevRef`.
+ pub unsafe fn new(
+ parent: *mut bindings::BlockDriverState,
+ bref: *mut bindings::BlockdevRef,
+ errp: *mut *mut bindings::Error,
+ ) -> Option<Self> {
+ unsafe {
+ let child_bs = bindings::bdrv_open_blockdev_ref_file(bref, parent, errp);
+ if child_bs.is_null() {
+ return None;
+ }
+
+ bindings::bdrv_graph_wrlock();
+ let child = bindings::bdrv_attach_child(
+ parent,
+ child_bs,
+ c"file".as_ptr(),
+ &bindings::child_of_bds as *const _,
+ bindings::BDRV_CHILD_IMAGE,
+ errp,
+ );
+ bindings::bdrv_graph_wrunlock();
+
+ if child.is_null() {
+ None
+ } else {
+ Some(BdrvChild { child })
+ }
+ }
+ }
+
+ /// Reads data from the child node into a linear byte buffer.
+ ///
+ /// # Safety
+ ///
+ /// `buf` must be a valid I/O buffer that can store at least `bytes` bytes.
+ pub async unsafe fn read_raw(&self, offset: u64, bytes: usize, buf: *mut u8) -> io::Result<()> {
+ let offset: i64 = offset
+ .try_into()
+ .map_err(|e| Error::new(ErrorKind::InvalidInput, e))?;
+ let bytes: i64 = bytes
+ .try_into()
+ .map_err(|e| Error::new(ErrorKind::InvalidInput, e))?;
+
+ let ret = unsafe { bindings::bdrv_pread(self.child, offset, bytes, buf as *mut c_void, 0) };
+ if ret < 0 {
+ Err(Error::from_raw_os_error(ret))
+ } else {
+ Ok(())
+ }
+ }
+
+ /// Reads data from the child node into a linear typed buffer.
+ pub async fn read<T: IoBuffer + ?Sized>(&self, offset: u64, buf: &mut T) -> io::Result<()> {
+ unsafe {
+ self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
+ .await
+ }
+ }
+
+ /// Reads data from the child node into a linear, potentially uninitialised typed buffer.
+ pub async fn read_uninit<T: SizedIoBuffer>(
+ &self,
+ offset: u64,
+ mut buf: MaybeUninit<T>,
+ ) -> io::Result<T> {
+ unsafe {
+ self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
+ .await?;
+ Ok(buf.assume_init())
+ }
+ }
+}
+
+#[doc(hidden)]
+pub unsafe extern "C" fn bdrv_open<D: BlockDriver>(
+ bs: *mut bindings::BlockDriverState,
+ options: *mut bindings::QDict,
+ _flags: std::os::raw::c_int,
+ errp: *mut *mut bindings::Error,
+) -> std::os::raw::c_int {
+ unsafe {
+ let v = match bindings::qobject_input_visitor_new_flat_confused(options, errp).as_mut() {
+ None => return -(bindings::EINVAL as std::os::raw::c_int),
+ Some(v) => v,
+ };
+
+ let mut opts: *mut D::Options = ptr::null_mut();
+ D::parse_options(v, &mut opts, errp);
+ bindings::visit_free(v);
+
+ let opts = match opts.as_mut() {
+ None => return -(bindings::EINVAL as std::os::raw::c_int),
+ Some(opts) => opts,
+ };
+
+ while let Some(e) = bindings::qdict_first(options).as_ref() {
+ bindings::qdict_del(options, e.key);
+ }
+
+ let ret = D::open(bs, opts, errp);
+ D::free_options(opts);
+ ret
+ }
+}
+
+#[doc(hidden)]
+pub unsafe extern "C" fn bdrv_close<D: BlockDriver>(bs: *mut bindings::BlockDriverState) {
+ unsafe {
+ let state = (*bs).opaque as *mut D;
+ ptr::drop_in_place(state);
+ }
+}
+
+/// Declare a format block driver. This macro is meant to be used at the top level.
+///
+/// `typ` is a type implementing the [`BlockDriver`] trait to handle the image format with the
+/// user-visible name `fmtname`.
+macro_rules! block_driver {
+ ($fmtname:expr, $typ:ty) => {
+ const _: () = {
+ static mut BLOCK_DRIVER: ::qemu_api::bindings::BlockDriver =
+ ::qemu_api::bindings::BlockDriver {
+ format_name: ::qemu_api::c_str!($fmtname).as_ptr(),
+ instance_size: ::std::mem::size_of::<$typ>() as i32,
+ bdrv_open: Some($crate::driver::bdrv_open::<$typ>),
+ bdrv_close: Some($crate::driver::bdrv_close::<$typ>),
+ bdrv_child_perm: Some(::qemu_api::bindings::bdrv_default_perms),
+ is_format: true,
+ ..::qemu_api::zeroable::Zeroable::ZERO
+ };
+
+ qemu_api::module_init! {
+ MODULE_INIT_BLOCK => unsafe {
+ ::qemu_api::bindings::bdrv_register(std::ptr::addr_of_mut!(BLOCK_DRIVER));
+ }
+ }
+ };
+ };
+}
+pub(crate) use block_driver;
diff --git a/rust/block/src/lib.rs b/rust/block/src/lib.rs
index 1c03549821..54ebd480ec 100644
--- a/rust/block/src/lib.rs
+++ b/rust/block/src/lib.rs
@@ -1,3 +1,4 @@
+mod driver;
mod iobuffer;
pub use iobuffer::{IoBuffer, SizedIoBuffer};
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 08/11] rust/block: Add driver module
2025-02-11 21:43 ` [PATCH 08/11] rust/block: Add driver module Kevin Wolf
@ 2025-02-12 16:43 ` Paolo Bonzini
2025-02-12 17:32 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 16:43 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/11/25 22:43, Kevin Wolf wrote:
> + /// Reads data from the child node into a linear typed buffer.
> + pub async fn read<T: IoBuffer + ?Sized>(&self, offset: u64, buf: &mut T) -> io::Result<()> {
> + unsafe {
> + self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
> + .await
> + }
> + }
> +
> + /// Reads data from the child node into a linear, potentially uninitialised typed buffer.
> + pub async fn read_uninit<T: SizedIoBuffer>(
> + &self,
> + offset: u64,
> + mut buf: MaybeUninit<T>,
I think Rust doesn't guarantee no copies here, so maybe this could be
pub async fn read_uninit<T: SizedIoBuffer>(
&self,
offset: u64,
buf: &mut MaybeUninit<T>,
) -> io::Result<&mut T>
using assume_init_mut().
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 08/11] rust/block: Add driver module
2025-02-12 16:43 ` Paolo Bonzini
@ 2025-02-12 17:32 ` Kevin Wolf
2025-02-12 18:17 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 17:32 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 17:43 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > + /// Reads data from the child node into a linear typed buffer.
> > + pub async fn read<T: IoBuffer + ?Sized>(&self, offset: u64, buf: &mut T) -> io::Result<()> {
> > + unsafe {
> > + self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
> > + .await
> > + }
> > + }
> > +
> > + /// Reads data from the child node into a linear, potentially uninitialised typed buffer.
> > + pub async fn read_uninit<T: SizedIoBuffer>(
> > + &self,
> > + offset: u64,
> > + mut buf: MaybeUninit<T>,
>
> I think Rust doesn't guarantee no copies here, so maybe this could be
Do you think that in practice the compiler won't optimise the copy away?
Or is this more of a theoretical concern?
> pub async fn read_uninit<T: SizedIoBuffer>(
> &self,
> offset: u64,
> buf: &mut MaybeUninit<T>,
> ) -> io::Result<&mut T>
>
> using assume_init_mut().
Are you sure that callers are ok with only getting a &mut T rather than
an owned T?
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 08/11] rust/block: Add driver module
2025-02-12 17:32 ` Kevin Wolf
@ 2025-02-12 18:17 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 18:17 UTC (permalink / raw)
To: Kevin Wolf
Cc: open list:Block layer core, Hanna Czenczek,
Emmanouil Pitsidianakis, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]
Il mer 12 feb 2025, 18:32 Kevin Wolf <kwolf@redhat.com> ha scritto:
> > > + mut buf: MaybeUninit<T>,
> >
> > I think Rust doesn't guarantee no copies here, so maybe this could be
>
> Do you think that in practice the compiler won't optimise the copy away?
>
It's possiblr that it does not, because it has to build the io::Result and
stick the result of assume_init() in there. It all depends on the amount of
inlining perhaps?
I think Box<MaybeUninit>> is the only way to guarantee no copies
(assume_init for Box was only stabilized recently but it can be emulated
with Box::into_raw and Box::from_raw).
> pub async fn read_uninit<T: SizedIoBuffer>(
> > &self,
> > offset: u64,
> > buf: &mut MaybeUninit<T>,
> > ) -> io::Result<&mut T>
> >
> > using assume_init_mut().
>
> Are you sure that callers are ok with only getting a &mut T rather than
> an owned T?
>
The one you have would need to be adjusted but it would work.
Another possibility by the way is to have "pub async fn read_obj<T:
SizedIoBuffer>(&self, offset: u64) -> io::Result<T>" and hide the usage of
MaybeUninit inside the function... That one doesn't even try to avoid
copies though.
Paolo
> Kevin
>
>
[-- Attachment #2: Type: text/html, Size: 2424 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 09/11] rust/block: Add read support for block drivers
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (7 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 08/11] rust/block: Add driver module Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 15:05 ` Paolo Bonzini
2025-02-11 21:43 ` [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust Kevin Wolf
2025-02-11 21:43 ` [PATCH 11/11] rust/block: Add format probing Kevin Wolf
10 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
This adds a map() function to the BlockDriver trait and makes use of it
to implement reading from an image.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/block/src/driver.rs | 100 +++++++++++++++++++++++++++++++++++++++
1 file changed, 100 insertions(+)
diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
index 5c7c46bfa0..2849ef6949 100644
--- a/rust/block/src/driver.rs
+++ b/rust/block/src/driver.rs
@@ -9,11 +9,46 @@
use crate::{IoBuffer, SizedIoBuffer};
use qemu_api::bindings;
+use qemu_api::futures::qemu_co_run_future;
+use std::cmp::min;
use std::ffi::c_void;
use std::io::{self, Error, ErrorKind};
use std::mem::MaybeUninit;
use std::ptr;
+/// A request to a block driver
+pub enum Request {
+ Read { offset: u64, len: u64 },
+}
+
+/// The target for a number of guest blocks, e.g. a location in a child node or the information
+/// that the described blocks are unmapped.
+// FIXME Actually use node
+#[allow(dead_code)]
+pub enum MappingTarget {
+ /// The described blocks are unallocated. Reading from them yields zeros.
+ Unmapped,
+
+ /// The described blocks are stored in a child node.
+ Data {
+ /// Child node in which the data is stored
+ node: (),
+
+ /// Offset in the child node at which the data is stored
+ offset: u64,
+ },
+}
+
+/// A mapping for a number of contiguous guest blocks
+pub struct Mapping {
+ /// Offset of the mapped blocks from the perspective of the guest
+ pub offset: u64,
+ /// Length of the mapping in bytes
+ pub len: u64,
+ /// Where the data for the described blocks is stored
+ pub target: MappingTarget,
+}
+
/// A trait for writing block drivers.
///
/// Types that implement this trait can be registered as QEMU block drivers using the
@@ -37,6 +72,11 @@ unsafe fn open(
/// Returns the size of the image in bytes
fn size(&self) -> u64;
+
+ /// Returns the mapping for the first part of `req`. If the returned mapping is shorter than
+ /// the request, the function can be called again with a shortened request to get the mapping
+ /// for the remaining part.
+ async fn map(&self, req: &Request) -> io::Result<Mapping>;
}
/// Represents the connection between a parent and its child node.
@@ -161,6 +201,65 @@ pub async fn read_uninit<T: SizedIoBuffer>(
}
}
+#[doc(hidden)]
+pub unsafe extern "C" fn bdrv_co_preadv_part<D: BlockDriver>(
+ bs: *mut bindings::BlockDriverState,
+ offset: i64,
+ bytes: i64,
+ qiov: *mut bindings::QEMUIOVector,
+ mut qiov_offset: usize,
+ flags: bindings::BdrvRequestFlags,
+) -> std::os::raw::c_int {
+ let s = unsafe { &mut *((*bs).opaque as *mut D) };
+
+ let mut offset = offset as u64;
+ let mut bytes = bytes as u64;
+
+ while bytes > 0 {
+ let req = Request::Read { offset, len: bytes };
+ let mapping = match qemu_co_run_future(s.map(&req)) {
+ Ok(mapping) => mapping,
+ Err(e) => {
+ return e
+ .raw_os_error()
+ .unwrap_or(-(bindings::EIO as std::os::raw::c_int))
+ }
+ };
+
+ let mapping_offset = offset - mapping.offset;
+ let cur_bytes = min(bytes, mapping.len - mapping_offset);
+
+ match mapping.target {
+ MappingTarget::Unmapped => unsafe {
+ bindings::qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes.try_into().unwrap());
+ },
+ MappingTarget::Data {
+ node: _,
+ offset: target_offset,
+ } => unsafe {
+ // TODO Support using child nodes other than bs->file
+ let ret = bindings::bdrv_co_preadv_part(
+ (*bs).file,
+ (target_offset + mapping_offset) as i64,
+ cur_bytes as i64,
+ qiov,
+ qiov_offset,
+ flags,
+ );
+ if ret < 0 {
+ return ret;
+ }
+ },
+ }
+
+ offset += cur_bytes;
+ qiov_offset += cur_bytes as usize;
+ bytes -= cur_bytes;
+ }
+
+ 0
+}
+
/// Declare a format block driver. This macro is meant to be used at the top level.
///
/// `typ` is a type implementing the [`BlockDriver`] trait to handle the image format with the
@@ -174,6 +273,7 @@ macro_rules! block_driver {
instance_size: ::std::mem::size_of::<$typ>() as i32,
bdrv_open: Some($crate::driver::bdrv_open::<$typ>),
bdrv_close: Some($crate::driver::bdrv_close::<$typ>),
+ bdrv_co_preadv_part: Some($crate::driver::bdrv_co_preadv_part::<$typ>),
bdrv_child_perm: Some(::qemu_api::bindings::bdrv_default_perms),
is_format: true,
..::qemu_api::zeroable::Zeroable::ZERO
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 09/11] rust/block: Add read support for block drivers
2025-02-11 21:43 ` [PATCH 09/11] rust/block: Add read support for block drivers Kevin Wolf
@ 2025-02-12 15:05 ` Paolo Bonzini
2025-02-12 20:52 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-12 15:05 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
On 2/11/25 22:43, Kevin Wolf wrote:
> +/// A request to a block driver
> +pub enum Request {
> + Read { offset: u64, len: u64 },
> +}
> +
Maybe add flags already?
> +#[allow(dead_code)]
> +pub enum MappingTarget {
> + /// The described blocks are unallocated. Reading from them yields zeros.
> + Unmapped,
> +
> + /// The described blocks are stored in a child node.
> + Data {
> + /// Child node in which the data is stored
> + node: (),
Make it already a *mut BlockDriverState, or *mut BdrvChild? Or are you worried of
irritating the borrow checker? :)
> + /// Offset in the child node at which the data is stored
> + offset: u64,
> + },
> +}
> +
> +/// A mapping for a number of contiguous guest blocks
> +pub struct Mapping {
> + /// Offset of the mapped blocks from the perspective of the guest
> + pub offset: u64,
> + /// Length of the mapping in bytes
> + pub len: u64,
> + /// Where the data for the described blocks is stored
> + pub target: MappingTarget,
> +}
> +
> /// A trait for writing block drivers.
> ///
> /// Types that implement this trait can be registered as QEMU block drivers using the
> @@ -37,6 +72,11 @@ unsafe fn open(
>
> /// Returns the size of the image in bytes
> fn size(&self) -> u64;
> +
> + /// Returns the mapping for the first part of `req`. If the returned mapping is shorter than
> + /// the request, the function can be called again with a shortened request to get the mapping
> + /// for the remaining part.
> + async fn map(&self, req: &Request) -> io::Result<Mapping>;
I am not sure I like the idea of making this the only way to do a read.
Maybe you can keep preadv_part in the trait, and add an utility function
like:
async fn bdrv_co_preadv_with_mappings<F: Future<Output = io::Result<()>>>(
mut offset: u64,
mut bytes: u64,
// IIRC qiov is not changed, but perhaps you still want to require &mut
// for it to _write_ memory?
qiov: &mut bindings::QEMUIOVector,
mut qiov_offset: usize,
flags: bindings::BdrvRequestFlags,
mut f: impl FnMut(Request) -> F) -> io::Result<()> {
while bytes > 0 {
let req = Request::Read { offset, len: bytes, flags };
let mapping = f(req).await?;
...
}
Ok(())
}
Then you can implement BochsImage's trait methods as:
async fn preadv_part(
&self,
offset: u64,
bytes: u64,
qiov: &mut bindings::QEMUIOVector,
qiov_offset: usize,
flags: bindings::BdrvRequestFlags,
) -> io::Result<()> {
bdrv_co_preadv_with_mappings(offset, bytes, qiov, qiov_offset, flags,
|req| self.map(req)).await
}
BochsImage::map() is a private function and the outer bdrv_co_preadv_part()
only handles qemu_co_run_future() and converting the io::Result.
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 09/11] rust/block: Add read support for block drivers
2025-02-12 15:05 ` Paolo Bonzini
@ 2025-02-12 20:52 ` Kevin Wolf
0 siblings, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 20:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, hreitz, manos.pitsidianakis, qemu-devel, qemu-rust
Am 12.02.2025 um 16:05 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > +/// A request to a block driver
> > +pub enum Request {
> > + Read { offset: u64, len: u64 },
> > +}
> > +
>
> Maybe add flags already?
> > +#[allow(dead_code)]
> > +pub enum MappingTarget {
> > + /// The described blocks are unallocated. Reading from them yields zeros.
> > + Unmapped,
> > +
> > + /// The described blocks are stored in a child node.
> > + Data {
> > + /// Child node in which the data is stored
> > + node: (),
>
> Make it already a *mut BlockDriverState, or *mut BdrvChild? Or are you worried of
> irritating the borrow checker? :)
Mostly I just didn't need it yet and was too lazy to think about the
details.
The right type would probably be Arc<driver::BdrvChild> (which contains
the raw *mut pointer). But I haven't thought much about how this plays
together with graph changes.
> > + /// Offset in the child node at which the data is stored
> > + offset: u64,
> > + },
> > +}
> > +
> > +/// A mapping for a number of contiguous guest blocks
> > +pub struct Mapping {
> > + /// Offset of the mapped blocks from the perspective of the guest
> > + pub offset: u64,
> > + /// Length of the mapping in bytes
> > + pub len: u64,
> > + /// Where the data for the described blocks is stored
> > + pub target: MappingTarget,
> > +}
> > +
> > /// A trait for writing block drivers.
> > ///
> > /// Types that implement this trait can be registered as QEMU block drivers using the
> > @@ -37,6 +72,11 @@ unsafe fn open(
> > /// Returns the size of the image in bytes
> > fn size(&self) -> u64;
> > +
> > + /// Returns the mapping for the first part of `req`. If the returned mapping is shorter than
> > + /// the request, the function can be called again with a shortened request to get the mapping
> > + /// for the remaining part.
> > + async fn map(&self, req: &Request) -> io::Result<Mapping>;
>
> I am not sure I like the idea of making this the only way to do a read.
We'll clearly need a way for drivers to have explicit functions when the
blocks aren't just mapped to somewhere else, e.g. with compression or
encryption. (My idea for that was that it would be another MappingTarget
branch.)
But using map() as the primary interface is intentional as it enables a
few things, even though this isn't visible in the code yet. Of course,
that basically every block driver duplicates the same loop is a reason
to unify it, but as you say there are other ways to do that.
One thing map() can do in the common case is provide the caller just a
list of mappings to a file descriptor and an offset. This may in the
future allow some block exports like ublk or fuse to use zero copy
operations. You can't do that if the block driver already does an
explicit read into a buffer.
You can cache mappings outside of the block driver and even of QEMU.
Block exports could tell the kernel that it shouldn't even bother with
going to userspace if a request can be completed using a cached mapping.
Two hosts with shared storage could access the same qcow2 image without
one of them sending all data across the network (like we did in KubeSAN
with NBD), but it's enough if metadata (= mappings) are synchronised.
So I think this is a useful interface to have, even if we don't know
which of the possibilities we'll actually make use of. And it should be
the primary interface, because if you want to do caching, then cache
invalidation must be done properly by block drivers, which is more
likely to go wrong if it's just an additional interface that isn't
normally used.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (8 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 09/11] rust/block: Add read support for block drivers Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
2025-02-12 7:45 ` Philippe Mathieu-Daudé
2025-02-12 9:14 ` Daniel P. Berrangé
2025-02-11 21:43 ` [PATCH 11/11] rust/block: Add format probing Kevin Wolf
10 siblings, 2 replies; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
This adds a separate block driver for the bochs image format called
'bochs-rs' so that for the moment both the C implementation and the Rust
implementation can be present in the same build. The intention is to
remove the C implementation eventually and rename this one into 'bochs'.
This can only happen once Rust can be a hard build dependency for QEMU.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/block/Cargo.toml | 2 +-
rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
rust/block/src/driver.rs | 5 -
rust/block/src/lib.rs | 1 +
4 files changed, 298 insertions(+), 6 deletions(-)
create mode 100644 rust/block/src/bochs.rs
diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
index 70ee02f429..1c06f3a00c 100644
--- a/rust/block/Cargo.toml
+++ b/rust/block/Cargo.toml
@@ -3,7 +3,7 @@ name = "block"
version = "0.1.0"
edition = "2021"
authors = ["Kevin Wolf <kwolf@redhat.com>"]
-license = "GPL-2.0-or-later"
+license = "GPL-2.0-or-later AND MIT"
readme = "README.md"
description = "Block backends for QEMU"
repository = "https://gitlab.com/qemu-project/qemu/"
diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
new file mode 100644
index 0000000000..388ac5ef03
--- /dev/null
+++ b/rust/block/src/bochs.rs
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Block driver for the various disk image formats used by Bochs
+ * Currently only for "growing" type in read-only mode
+ *
+ * Copyright (c) 2005 Alex Beregszaszi
+ * Copyright (c) 2024 Red Hat
+ *
+ * Authors:
+ * Alex Beregszaszi
+ * Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+use crate::driver::{block_driver, BdrvChild, BlockDriver, Mapping, MappingTarget, Request};
+use crate::SizedIoBuffer;
+use qemu_api::bindings;
+use qemu_api::futures::qemu_run_future;
+use std::cmp::min;
+use std::io::{self, Error, ErrorKind};
+use std::mem::MaybeUninit;
+use std::ptr;
+
+const BDRV_SECTOR_SIZE: u64 = 512;
+
+const HEADER_MAGIC: [u8; 32] = *b"Bochs Virtual HD Image\0\0\0\0\0\0\0\0\0\0";
+const HEADER_VERSION: u32 = 0x00020000;
+const HEADER_V1: u32 = 0x00010000;
+const HEADER_SIZE: usize = 512;
+
+const HEADER_TYPE_REDOLOG: [u8; 16] = *b"Redolog\0\0\0\0\0\0\0\0\0";
+const HEADER_SUBTYPE_GROWING: [u8; 16] = *b"Growing\0\0\0\0\0\0\0\0\0";
+
+// TODO Use u64.div_ceil() when MSRV is updated to at least 1.73.0
+fn div_ceil(a: u64, b: u64) -> u64 {
+ (a + b - 1) / b
+}
+
+// TODO Use little endian enforcing type for integers
+
+#[repr(C, packed)]
+struct BochsHeader {
+ pub magic: [u8; 32],
+ pub imgtype: [u8; 16],
+ pub subtype: [u8; 16],
+ pub version: u32,
+ pub header_size: u32,
+ pub catalog_entries: u32,
+ pub bitmap_size: u32,
+ pub extent_size: u32,
+ pub extra: BochsHeaderExtra,
+}
+unsafe impl SizedIoBuffer for BochsHeader {}
+
+#[repr(C, packed)]
+union BochsHeaderExtra {
+ v2: BochsHeaderExtraRedolog,
+ v1: BochsHeaderExtraRedologV1,
+ padding: [u8; HEADER_SIZE - 84],
+}
+
+#[repr(C, packed)]
+#[derive(Clone, Copy)]
+struct BochsHeaderExtraRedolog {
+ pub timestamp: u32,
+ pub disk_size: u64,
+}
+
+#[repr(C, packed)]
+#[derive(Clone, Copy)]
+struct BochsHeaderExtraRedologV1 {
+ pub disk_size: u64,
+}
+
+pub struct BochsImage {
+ file: BdrvChild,
+ size: u64,
+ data_offset: u64,
+ bitmap_blocks: u64,
+ extent_size: u64,
+ extent_blocks: u64,
+ catalog_bitmap: Vec<u32>, // TODO Rename
+}
+
+impl BochsImage {
+ pub async fn new(file: BdrvChild) -> io::Result<Self> {
+ let header = file
+ .read_uninit(0, MaybeUninit::<BochsHeader>::uninit())
+ .await?;
+
+ if header.magic != HEADER_MAGIC
+ || header.imgtype != HEADER_TYPE_REDOLOG
+ || header.subtype != HEADER_SUBTYPE_GROWING
+ {
+ return Err(Error::new(
+ ErrorKind::InvalidInput,
+ "Image not in Bochs format",
+ ));
+ }
+
+ let size = match u32::from_le(header.version) {
+ HEADER_VERSION => unsafe { header.extra.v2.disk_size.to_le() },
+ HEADER_V1 => unsafe { header.extra.v1.disk_size.to_le() },
+ _ => return Err(Error::new(ErrorKind::InvalidInput, "Version not supported")),
+ };
+
+ let header_size: u64 = header.header_size.to_le().into();
+ let extent_size: u64 = header.extent_size.to_le().into();
+
+ if extent_size < BDRV_SECTOR_SIZE {
+ // bximage actually never creates extents smaller than 4k
+ return Err(Error::new(
+ ErrorKind::InvalidInput,
+ "Extent size must be at least 512",
+ ));
+ } else if !extent_size.is_power_of_two() {
+ return Err(Error::new(
+ ErrorKind::InvalidInput,
+ format!("Extent size {extent_size} is not a power of two"),
+ ));
+ } else if extent_size > 0x800000 {
+ return Err(Error::new(
+ ErrorKind::InvalidInput,
+ format!("Extent size {extent_size} is too large"),
+ ));
+ }
+
+ // Limit to 1M entries to avoid unbounded allocation. This is what is
+ // needed for the largest image that bximage can create (~8 TB).
+ let catalog_entries: usize = header
+ .catalog_entries
+ .to_le()
+ .try_into()
+ .map_err(|e| Error::new(ErrorKind::InvalidInput, e))?;
+ if catalog_entries > 0x100000 {
+ return Err(Error::new(ErrorKind::Other, "Catalog size is too large"));
+ } else if (catalog_entries as u64) < div_ceil(size, extent_size) {
+ return Err(Error::new(
+ ErrorKind::InvalidInput,
+ "Catalog size is too small for this disk size",
+ ));
+ }
+
+ // FIXME This was g_try_malloc() in C
+ let mut catalog_bitmap = vec![0u32; catalog_entries];
+ file.read(header_size, catalog_bitmap.as_mut_slice())
+ .await?;
+
+ for entry in &mut catalog_bitmap {
+ *entry = entry.to_le();
+ }
+
+ let data_offset = header_size + (catalog_entries as u64 * 4);
+ let bitmap_blocks = (1 + (header.bitmap_size.to_le() - 1) / 512).into();
+ let extent_blocks = 1 + (extent_size - 1) / 512;
+
+ Ok(Self {
+ file,
+ size,
+ data_offset,
+ bitmap_blocks,
+ extent_size,
+ extent_blocks,
+ catalog_bitmap,
+ })
+ }
+}
+
+impl BlockDriver for BochsImage {
+ type Options = bindings::BlockdevOptionsGenericFormat;
+
+ unsafe fn parse_options(
+ v: &mut bindings::Visitor,
+ opts: &mut *mut Self::Options,
+ errp: *mut *mut bindings::Error,
+ ) {
+ unsafe {
+ bindings::visit_type_BlockdevOptionsGenericFormat(v, ptr::null(), opts as *mut _, errp);
+ }
+ }
+
+ unsafe fn free_options(opts: *mut Self::Options) {
+ unsafe {
+ bindings::qapi_free_BlockdevOptionsGenericFormat(opts);
+ }
+ }
+
+ unsafe fn open(
+ bs: *mut bindings::BlockDriverState,
+ opts: &Self::Options,
+ errp: *mut *mut bindings::Error,
+ ) -> std::os::raw::c_int {
+ let file_child;
+ unsafe {
+ /* No write support yet */
+ bindings::bdrv_graph_rdlock_main_loop();
+ let ret = bindings::bdrv_apply_auto_read_only(bs, ptr::null(), errp);
+ bindings::bdrv_graph_rdunlock_main_loop();
+ if ret < 0 {
+ return ret;
+ }
+
+ file_child = match BdrvChild::new(bs, opts.file, errp) {
+ Some(c) => c,
+ None => return -(bindings::EINVAL as std::os::raw::c_int),
+ };
+ }
+
+ qemu_run_future(async {
+ match BochsImage::new(file_child).await {
+ Ok(bdrv) => unsafe {
+ (*bs).total_sectors =
+ div_ceil(bdrv.size(), BDRV_SECTOR_SIZE).try_into().unwrap();
+ let state = (*bs).opaque as *mut BochsImage;
+ *state = bdrv;
+ 0
+ },
+ // FIXME This is not a good default error code
+ Err(e) => e.raw_os_error().unwrap_or(-1),
+ }
+ })
+ }
+
+ fn size(&self) -> u64 {
+ self.size
+ }
+
+ async fn map(&self, req: &Request) -> io::Result<Mapping> {
+ let (offset, len) = match *req {
+ Request::Read { offset, len } => (offset, len),
+ };
+
+ let extent_index: usize = (offset / self.extent_size).try_into().unwrap();
+ let extent_offset = (offset % self.extent_size) / 512;
+
+ if self.catalog_bitmap[extent_index] == 0xffffffff {
+ return Ok(Mapping {
+ offset: (extent_index as u64) * self.extent_size,
+ len: self.extent_size,
+ target: MappingTarget::Unmapped,
+ });
+ }
+
+ let bitmap_offset = self.data_offset
+ + (512
+ * (self.catalog_bitmap[extent_index] as u64)
+ * (self.extent_blocks + self.bitmap_blocks));
+
+ // Read in bitmap for current extent
+ // TODO This should be cached
+ let mut bitmap_entry = 0x8;
+ self.file
+ .read(bitmap_offset + (extent_offset / 8), &mut bitmap_entry)
+ .await?;
+
+ // We checked only a single sector
+ let offset = offset & !511;
+ let len = min(len, 512);
+
+ if (bitmap_entry >> (extent_offset % 8)) & 1 == 0 {
+ return Ok(Mapping {
+ offset,
+ len,
+ target: MappingTarget::Unmapped,
+ });
+ }
+
+ Ok(Mapping {
+ offset,
+ len,
+ target: MappingTarget::Data {
+ node: (),
+ offset: bitmap_offset + (512 * (self.bitmap_blocks + extent_offset)),
+ },
+ })
+ }
+}
+
+block_driver!("bochs-rs", BochsImage);
diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
index 2849ef6949..740e9266aa 100644
--- a/rust/block/src/driver.rs
+++ b/rust/block/src/driver.rs
@@ -2,11 +2,6 @@
// Author(s): Kevin Wolf <kwolf@redhat.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-// All of this is unused until the first block driver is added
-#![allow(dead_code)]
-#![allow(unused_macros)]
-#![allow(unused_imports)]
-
use crate::{IoBuffer, SizedIoBuffer};
use qemu_api::bindings;
use qemu_api::futures::qemu_co_run_future;
diff --git a/rust/block/src/lib.rs b/rust/block/src/lib.rs
index 54ebd480ec..ff528609bc 100644
--- a/rust/block/src/lib.rs
+++ b/rust/block/src/lib.rs
@@ -1,3 +1,4 @@
+mod bochs;
mod driver;
mod iobuffer;
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-11 21:43 ` [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust Kevin Wolf
@ 2025-02-12 7:45 ` Philippe Mathieu-Daudé
2025-02-12 12:59 ` Kevin Wolf
2025-02-12 9:14 ` Daniel P. Berrangé
1 sibling, 1 reply; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 7:45 UTC (permalink / raw)
To: Kevin Wolf, qemu-block
Cc: hreitz, pbonzini, manos.pitsidianakis, qemu-devel, qemu-rust
On 11/2/25 22:43, Kevin Wolf wrote:
> This adds a separate block driver for the bochs image format called
> 'bochs-rs' so that for the moment both the C implementation and the Rust
> implementation can be present in the same build. The intention is to
> remove the C implementation eventually and rename this one into 'bochs'.
> This can only happen once Rust can be a hard build dependency for QEMU.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> rust/block/Cargo.toml | 2 +-
> rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
> rust/block/src/driver.rs | 5 -
> rust/block/src/lib.rs | 1 +
> 4 files changed, 298 insertions(+), 6 deletions(-)
> create mode 100644 rust/block/src/bochs.rs
> diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
> new file mode 100644
> index 0000000000..388ac5ef03
> --- /dev/null
> +++ b/rust/block/src/bochs.rs
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Block driver for the various disk image formats used by Bochs
> + * Currently only for "growing" type in read-only mode
> + *
> + * Copyright (c) 2005 Alex Beregszaszi
> + * Copyright (c) 2024 Red Hat
> + *
> + * Authors:
> + * Alex Beregszaszi
> + * Kevin Wolf <kwolf@redhat.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
As an addition, we don't have to add the full license boilerplate IMO...
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-12 7:45 ` Philippe Mathieu-Daudé
@ 2025-02-12 12:59 ` Kevin Wolf
2025-02-12 13:52 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 12:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-block, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
Am 12.02.2025 um 08:45 hat Philippe Mathieu-Daudé geschrieben:
> On 11/2/25 22:43, Kevin Wolf wrote:
> > This adds a separate block driver for the bochs image format called
> > 'bochs-rs' so that for the moment both the C implementation and the Rust
> > implementation can be present in the same build. The intention is to
> > remove the C implementation eventually and rename this one into 'bochs'.
> > This can only happen once Rust can be a hard build dependency for QEMU.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > rust/block/Cargo.toml | 2 +-
> > rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
> > rust/block/src/driver.rs | 5 -
> > rust/block/src/lib.rs | 1 +
> > 4 files changed, 298 insertions(+), 6 deletions(-)
> > create mode 100644 rust/block/src/bochs.rs
>
>
> > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
> > new file mode 100644
> > index 0000000000..388ac5ef03
> > --- /dev/null
> > +++ b/rust/block/src/bochs.rs
> > @@ -0,0 +1,296 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Block driver for the various disk image formats used by Bochs
> > + * Currently only for "growing" type in read-only mode
> > + *
> > + * Copyright (c) 2005 Alex Beregszaszi
> > + * Copyright (c) 2024 Red Hat
> > + *
> > + * Authors:
> > + * Alex Beregszaszi
> > + * Kevin Wolf <kwolf@redhat.com>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
>
> As an addition, we don't have to add the full license boilerplate IMO...
IANAL, but the license says "The above copyright notice and this
permission notice shall be included in all copies or substantial
portions of the Software.", so keeping it feels like the safe option.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-12 12:59 ` Kevin Wolf
@ 2025-02-12 13:52 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 13:52 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
On 12/2/25 13:59, Kevin Wolf wrote:
> Am 12.02.2025 um 08:45 hat Philippe Mathieu-Daudé geschrieben:
>> On 11/2/25 22:43, Kevin Wolf wrote:
>>> This adds a separate block driver for the bochs image format called
>>> 'bochs-rs' so that for the moment both the C implementation and the Rust
>>> implementation can be present in the same build. The intention is to
>>> remove the C implementation eventually and rename this one into 'bochs'.
>>> This can only happen once Rust can be a hard build dependency for QEMU.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> rust/block/Cargo.toml | 2 +-
>>> rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
>>> rust/block/src/driver.rs | 5 -
>>> rust/block/src/lib.rs | 1 +
>>> 4 files changed, 298 insertions(+), 6 deletions(-)
>>> create mode 100644 rust/block/src/bochs.rs
>>
>>
>>> diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
>>> new file mode 100644
>>> index 0000000000..388ac5ef03
>>> --- /dev/null
>>> +++ b/rust/block/src/bochs.rs
>>> @@ -0,0 +1,296 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Block driver for the various disk image formats used by Bochs
>>> + * Currently only for "growing" type in read-only mode
>>> + *
>>> + * Copyright (c) 2005 Alex Beregszaszi
>>> + * Copyright (c) 2024 Red Hat
>>> + *
>>> + * Authors:
>>> + * Alex Beregszaszi
>>> + * Kevin Wolf <kwolf@redhat.com>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>> + */
>>
>> As an addition, we don't have to add the full license boilerplate IMO...
>
> IANAL, but the license says "The above copyright notice and this
> permission notice shall be included in all copies or substantial
> portions of the Software.", so keeping it feels like the safe option.
OK (and as Daniel clarified, you consider your work a derivative).
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-11 21:43 ` [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust Kevin Wolf
2025-02-12 7:45 ` Philippe Mathieu-Daudé
@ 2025-02-12 9:14 ` Daniel P. Berrangé
2025-02-12 9:41 ` Daniel P. Berrangé
1 sibling, 1 reply; 43+ messages in thread
From: Daniel P. Berrangé @ 2025-02-12 9:14 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote:
> This adds a separate block driver for the bochs image format called
> 'bochs-rs' so that for the moment both the C implementation and the Rust
> implementation can be present in the same build. The intention is to
> remove the C implementation eventually and rename this one into 'bochs'.
> This can only happen once Rust can be a hard build dependency for QEMU.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> rust/block/Cargo.toml | 2 +-
> rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
> rust/block/src/driver.rs | 5 -
> rust/block/src/lib.rs | 1 +
> 4 files changed, 298 insertions(+), 6 deletions(-)
> create mode 100644 rust/block/src/bochs.rs
>
> diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
> index 70ee02f429..1c06f3a00c 100644
> --- a/rust/block/Cargo.toml
> +++ b/rust/block/Cargo.toml
> @@ -3,7 +3,7 @@ name = "block"
> version = "0.1.0"
> edition = "2021"
> authors = ["Kevin Wolf <kwolf@redhat.com>"]
> -license = "GPL-2.0-or-later"
> +license = "GPL-2.0-or-later AND MIT"
> readme = "README.md"
> description = "Block backends for QEMU"
> repository = "https://gitlab.com/qemu-project/qemu/"
> diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
> new file mode 100644
> index 0000000000..388ac5ef03
> --- /dev/null
> +++ b/rust/block/src/bochs.rs
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: MIT
Why MIT instead of our normal GPL-2.0-or-later.
Using Rust conversion to eliminate GPL usage for permissive licenses
like MIT is not something I'd like to see us doing.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-12 9:14 ` Daniel P. Berrangé
@ 2025-02-12 9:41 ` Daniel P. Berrangé
2025-02-12 12:58 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Daniel P. Berrangé @ 2025-02-12 9:41 UTC (permalink / raw)
To: Kevin Wolf, qemu-block, hreitz, pbonzini, manos.pitsidianakis,
qemu-devel, qemu-rust
On Wed, Feb 12, 2025 at 09:14:57AM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote:
> > This adds a separate block driver for the bochs image format called
> > 'bochs-rs' so that for the moment both the C implementation and the Rust
> > implementation can be present in the same build. The intention is to
> > remove the C implementation eventually and rename this one into 'bochs'.
> > This can only happen once Rust can be a hard build dependency for QEMU.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > rust/block/Cargo.toml | 2 +-
> > rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
> > rust/block/src/driver.rs | 5 -
> > rust/block/src/lib.rs | 1 +
> > 4 files changed, 298 insertions(+), 6 deletions(-)
> > create mode 100644 rust/block/src/bochs.rs
> >
> > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
> > index 70ee02f429..1c06f3a00c 100644
> > --- a/rust/block/Cargo.toml
> > +++ b/rust/block/Cargo.toml
> > @@ -3,7 +3,7 @@ name = "block"
> > version = "0.1.0"
> > edition = "2021"
> > authors = ["Kevin Wolf <kwolf@redhat.com>"]
> > -license = "GPL-2.0-or-later"
> > +license = "GPL-2.0-or-later AND MIT"
> > readme = "README.md"
> > description = "Block backends for QEMU"
> > repository = "https://gitlab.com/qemu-project/qemu/"
> > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
> > new file mode 100644
> > index 0000000000..388ac5ef03
> > --- /dev/null
> > +++ b/rust/block/src/bochs.rs
> > @@ -0,0 +1,296 @@
> > +// SPDX-License-Identifier: MIT
>
> Why MIT instead of our normal GPL-2.0-or-later.
>
> Using Rust conversion to eliminate GPL usage for permissive licenses
> like MIT is not something I'd like to see us doing.
My bad. I should have noticed that the original bochs.c was also MIT,
so I presume you're considering this Rust impl to be a derived work.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-12 9:41 ` Daniel P. Berrangé
@ 2025-02-12 12:58 ` Kevin Wolf
2025-02-12 13:07 ` Daniel P. Berrangé
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2025-02-12 12:58 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-block, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
Am 12.02.2025 um 10:41 hat Daniel P. Berrangé geschrieben:
> On Wed, Feb 12, 2025 at 09:14:57AM +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote:
> > > This adds a separate block driver for the bochs image format called
> > > 'bochs-rs' so that for the moment both the C implementation and the Rust
> > > implementation can be present in the same build. The intention is to
> > > remove the C implementation eventually and rename this one into 'bochs'.
> > > This can only happen once Rust can be a hard build dependency for QEMU.
> > >
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > > rust/block/Cargo.toml | 2 +-
> > > rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
> > > rust/block/src/driver.rs | 5 -
> > > rust/block/src/lib.rs | 1 +
> > > 4 files changed, 298 insertions(+), 6 deletions(-)
> > > create mode 100644 rust/block/src/bochs.rs
> > >
> > > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
> > > index 70ee02f429..1c06f3a00c 100644
> > > --- a/rust/block/Cargo.toml
> > > +++ b/rust/block/Cargo.toml
> > > @@ -3,7 +3,7 @@ name = "block"
> > > version = "0.1.0"
> > > edition = "2021"
> > > authors = ["Kevin Wolf <kwolf@redhat.com>"]
> > > -license = "GPL-2.0-or-later"
> > > +license = "GPL-2.0-or-later AND MIT"
> > > readme = "README.md"
> > > description = "Block backends for QEMU"
> > > repository = "https://gitlab.com/qemu-project/qemu/"
> > > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
> > > new file mode 100644
> > > index 0000000000..388ac5ef03
> > > --- /dev/null
> > > +++ b/rust/block/src/bochs.rs
> > > @@ -0,0 +1,296 @@
> > > +// SPDX-License-Identifier: MIT
> >
> > Why MIT instead of our normal GPL-2.0-or-later.
> >
> > Using Rust conversion to eliminate GPL usage for permissive licenses
> > like MIT is not something I'd like to see us doing.
>
> My bad. I should have noticed that the original bochs.c was also MIT,
> so I presume you're considering this Rust impl to be a derived work.
Yes, this is essentially a translation of the existing C bochs driver,
which is also why I kept the original copyright notice and the full
license text.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
2025-02-12 12:58 ` Kevin Wolf
@ 2025-02-12 13:07 ` Daniel P. Berrangé
0 siblings, 0 replies; 43+ messages in thread
From: Daniel P. Berrangé @ 2025-02-12 13:07 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
On Wed, Feb 12, 2025 at 01:58:15PM +0100, Kevin Wolf wrote:
> Am 12.02.2025 um 10:41 hat Daniel P. Berrangé geschrieben:
> > On Wed, Feb 12, 2025 at 09:14:57AM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote:
> > > > This adds a separate block driver for the bochs image format called
> > > > 'bochs-rs' so that for the moment both the C implementation and the Rust
> > > > implementation can be present in the same build. The intention is to
> > > > remove the C implementation eventually and rename this one into 'bochs'.
> > > > This can only happen once Rust can be a hard build dependency for QEMU.
> > > >
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > > ---
> > > > rust/block/Cargo.toml | 2 +-
> > > > rust/block/src/bochs.rs | 296 +++++++++++++++++++++++++++++++++++++++
> > > > rust/block/src/driver.rs | 5 -
> > > > rust/block/src/lib.rs | 1 +
> > > > 4 files changed, 298 insertions(+), 6 deletions(-)
> > > > create mode 100644 rust/block/src/bochs.rs
> > > >
> > > > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
> > > > index 70ee02f429..1c06f3a00c 100644
> > > > --- a/rust/block/Cargo.toml
> > > > +++ b/rust/block/Cargo.toml
> > > > @@ -3,7 +3,7 @@ name = "block"
> > > > version = "0.1.0"
> > > > edition = "2021"
> > > > authors = ["Kevin Wolf <kwolf@redhat.com>"]
> > > > -license = "GPL-2.0-or-later"
> > > > +license = "GPL-2.0-or-later AND MIT"
> > > > readme = "README.md"
> > > > description = "Block backends for QEMU"
> > > > repository = "https://gitlab.com/qemu-project/qemu/"
> > > > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
> > > > new file mode 100644
> > > > index 0000000000..388ac5ef03
> > > > --- /dev/null
> > > > +++ b/rust/block/src/bochs.rs
> > > > @@ -0,0 +1,296 @@
> > > > +// SPDX-License-Identifier: MIT
> > >
> > > Why MIT instead of our normal GPL-2.0-or-later.
> > >
> > > Using Rust conversion to eliminate GPL usage for permissive licenses
> > > like MIT is not something I'd like to see us doing.
> >
> > My bad. I should have noticed that the original bochs.c was also MIT,
> > so I presume you're considering this Rust impl to be a derived work.
>
> Yes, this is essentially a translation of the existing C bochs driver,
> which is also why I kept the original copyright notice and the full
> license text.
Agreed, I think including both is the right thing in this particular
case, especially given that we don't include the MIT text at all in
a LICENSE file at the top level.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 11/11] rust/block: Add format probing
2025-02-11 21:43 [PATCH 00/11] rust/block: Add minimal block driver bindings Kevin Wolf
` (9 preceding siblings ...)
2025-02-11 21:43 ` [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust Kevin Wolf
@ 2025-02-11 21:43 ` Kevin Wolf
10 siblings, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2025-02-11 21:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, hreitz, pbonzini, manos.pitsidianakis, qemu-devel,
qemu-rust
This adds format probing both to the BlockDriver trait and the bochs-rs
block driver. With this, bochs-rs achieves feature parity with its C
counterpart. Its probe function returns a higher priority so that it is
preferred when both drivers are available.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
rust/block/src/bochs.rs | 20 ++++++++++++++++++++
rust/block/src/driver.rs | 26 +++++++++++++++++++++++++-
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs
index 388ac5ef03..bccc55ead9 100644
--- a/rust/block/src/bochs.rs
+++ b/rust/block/src/bochs.rs
@@ -186,6 +186,26 @@ pub async fn new(file: BdrvChild) -> io::Result<Self> {
impl BlockDriver for BochsImage {
type Options = bindings::BlockdevOptionsGenericFormat;
+ fn probe(buf: &[u8], _filename: &str) -> u16 {
+ let header = match BochsHeader::from_byte_slice(buf) {
+ Some(header) => header,
+ None => return 0,
+ };
+
+ if header.magic != HEADER_MAGIC
+ || header.imgtype != HEADER_TYPE_REDOLOG
+ || header.subtype != HEADER_SUBTYPE_GROWING
+ {
+ return 0;
+ }
+
+ // This driver is better than the C one which returns 100, give it priority
+ match header.version {
+ HEADER_VERSION | HEADER_V1 => 200,
+ _ => 0,
+ }
+ }
+
unsafe fn parse_options(
v: &mut bindings::Visitor,
opts: &mut *mut Self::Options,
diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
index 740e9266aa..727d13b751 100644
--- a/rust/block/src/driver.rs
+++ b/rust/block/src/driver.rs
@@ -6,7 +6,7 @@
use qemu_api::bindings;
use qemu_api::futures::qemu_co_run_future;
use std::cmp::min;
-use std::ffi::c_void;
+use std::ffi::{c_void, CStr};
use std::io::{self, Error, ErrorKind};
use std::mem::MaybeUninit;
use std::ptr;
@@ -65,6 +65,16 @@ unsafe fn open(
errp: *mut *mut bindings::Error,
) -> std::os::raw::c_int;
+ /// Returns the image format probing priority of this block driver for disk images starting
+ /// with the byte sequence in `buf`. Probing selects the driver that returns the highest
+ /// number.
+ ///
+ /// If the driver doesn't support images starting with `buf`, 0 is returned.
+ fn probe(buf: &[u8], filename: &str) -> u16 {
+ let _ = (buf, filename);
+ 0
+ }
+
/// Returns the size of the image in bytes
fn size(&self) -> u64;
@@ -156,6 +166,19 @@ pub async fn read_uninit<T: SizedIoBuffer>(
}
}
+#[doc(hidden)]
+pub unsafe extern "C" fn bdrv_probe<D: BlockDriver>(
+ buf: *const u8,
+ buf_size: std::os::raw::c_int,
+ filename: *const std::os::raw::c_char,
+) -> std::os::raw::c_int {
+ let buf = unsafe { std::slice::from_raw_parts(buf, buf_size as usize) };
+ match unsafe { CStr::from_ptr(filename) }.to_str() {
+ Ok(filename) => D::probe(buf, filename).into(),
+ Err(_) => 0,
+ }
+}
+
#[doc(hidden)]
pub unsafe extern "C" fn bdrv_open<D: BlockDriver>(
bs: *mut bindings::BlockDriverState,
@@ -266,6 +289,7 @@ macro_rules! block_driver {
::qemu_api::bindings::BlockDriver {
format_name: ::qemu_api::c_str!($fmtname).as_ptr(),
instance_size: ::std::mem::size_of::<$typ>() as i32,
+ bdrv_probe: Some($crate::driver::bdrv_probe::<$typ>),
bdrv_open: Some($crate::driver::bdrv_open::<$typ>),
bdrv_close: Some($crate::driver::bdrv_close::<$typ>),
bdrv_co_preadv_part: Some($crate::driver::bdrv_co_preadv_part::<$typ>),
--
2.48.1
^ permalink raw reply related [flat|nested] 43+ messages in thread