[PATCH v2 00/11] rust/block: Add minimal block driver bindings

[PATCH v2 00/11] rust/block: Add minimal block driver bindings

[Kevin Wolf]

This series adds minimal bindings for writing block drivers in Rust and
converts the bochs block driver as an example. Some parts (such as the
open functions) are still essentially C written in Rust syntax, while
other parts already try to provide somewhat idiomatic abstractions.

The main purpose of this series is just to add a starting point for
incremental improvements to the bindings; it's clearly not very polished
yet and ignores some important points like enforcing the graph lock.

Therefore, the main focus in the immediate future after this series
should be cleaning up the bochs driver and the bindings it uses rather
than adding more drivers.

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

v2:
- Rebased on top of current git master with qemu_api::errno patches
  applied
- Changed 'node' in MappingTarget from a dummy () to Arc<BdrvChild>
- Changed BdrvChild::read_uninit() to use Box<MaybeUninit<T>>
- Use libc crate instead of letting bindgen output EINVAL/EIO
- Fixed errno translation logic, in parts by using qemu_api::errno
- Changed two instances of incorrect *foo = ... into foo.write(...)
- Added rust/block/ to MAINTAINERS
- Some style and readability improvements

Kevin Wolf (11):
  rust: Build separate qemu_api_tools and qemu_api_system
  meson: Add rust_block_ss and link tools with it
  rust: Add some block layer bindings
  rust/qemu-api: Add wrappers to run futures in QEMU
  rust/block: Add empty crate
  rust/block: Add I/O buffer traits
  block: Add bdrv_open_blockdev_ref_file()
  rust/block: Add driver module
  rust/block: Add read support for block drivers
  bochs-rs: Add bochs block driver reimplementation in Rust
  rust/block: Add format probing

 include/block/block-global-state.h |   4 +
 include/qemu/coroutine-rust.h      |  24 +++
 rust/wrapper-system.h              |  46 +++++
 rust/wrapper.h                     |  16 +-
 block.c                            |  31 ++-
 util/qemu-co-rust-async.c          |  55 +++++
 MAINTAINERS                        |   1 +
 meson.build                        |  47 ++++-
 rust/Cargo.lock                    |   8 +
 rust/Cargo.toml                    |   1 +
 rust/block/Cargo.toml              |  16 ++
 rust/block/README.md               |   3 +
 rust/block/meson.build             |  20 ++
 rust/block/src/bochs.rs            | 317 +++++++++++++++++++++++++++++
 rust/block/src/driver.rs           | 309 ++++++++++++++++++++++++++++
 rust/block/src/iobuffer.rs         |  94 +++++++++
 rust/block/src/lib.rs              |   5 +
 rust/hw/char/pl011/meson.build     |   3 +-
 rust/hw/timer/hpet/meson.build     |   3 +-
 rust/meson.build                   |  12 +-
 rust/qemu-api/Cargo.toml           |   1 +
 rust/qemu-api/build.rs             |  10 +-
 rust/qemu-api/meson.build          |  80 +++++---
 rust/qemu-api/src/bindings.rs      |  52 +++--
 rust/qemu-api/src/futures.rs       |  77 +++++++
 rust/qemu-api/src/lib.rs           |   6 +
 rust/qemu-api/src/prelude.rs       |   3 +
 rust/qemu-api/src/zeroable.rs      |  34 ++--
 storage-daemon/meson.build         |   2 +-
 util/meson.build                   |   3 +
 30 files changed, 1188 insertions(+), 95 deletions(-)
 create mode 100644 include/qemu/coroutine-rust.h
 create mode 100644 rust/wrapper-system.h
 create mode 100644 util/qemu-co-rust-async.c
 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/bochs.rs
 create mode 100644 rust/block/src/driver.rs
 create mode 100644 rust/block/src/iobuffer.rs
 create mode 100644 rust/block/src/lib.rs
 create mode 100644 rust/qemu-api/src/futures.rs

-- 
2.48.1

[PATCH v2 01/11] rust: Build separate qemu_api_tools and qemu_api_system

[Kevin Wolf]

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          | 46 ++++++++++++++++++++
 rust/wrapper.h                 | 11 -----
 meson.build                    | 11 ++++-
 rust/hw/char/pl011/meson.build |  3 +-
 rust/hw/timer/hpet/meson.build |  3 +-
 rust/meson.build               | 11 ++---
 rust/qemu-api/Cargo.toml       |  1 +
 rust/qemu-api/build.rs         | 10 ++++-
 rust/qemu-api/meson.build      | 79 +++++++++++++++++++++-------------
 rust/qemu-api/src/bindings.rs  | 52 ++++++++++++----------
 rust/qemu-api/src/lib.rs       |  5 +++
 rust/qemu-api/src/prelude.rs   |  3 ++
 rust/qemu-api/src/zeroable.rs  | 35 ++++++++-------
 13 files changed, 182 insertions(+), 88 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..06f4c5279a
--- /dev/null
+++ b/rust/wrapper-system.h
@@ -0,0 +1,46 @@
+/*
+ * 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 "exec/address-spaces.h"
+#include "exec/memattrs.h"
+#include "exec/memory.h"
+#include "hw/clock.h"
+#include "hw/irq.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "system/system.h"
diff --git a/rust/wrapper.h b/rust/wrapper.h
index d927ad6799..f203fd13ac 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -50,18 +50,7 @@ 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"
-#include "exec/memattrs.h"
 #include "qemu/timer.h"
-#include "exec/address-spaces.h"
diff --git a/meson.build b/meson.build
index 8ed10b6624..6ee1757828 100644
--- a/meson.build
+++ b/meson.build
@@ -4102,10 +4102,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/hw/timer/hpet/meson.build b/rust/hw/timer/hpet/meson.build
index c2d7c0532c..94b256ae2f 100644
--- a/rust/hw/timer/hpet/meson.build
+++ b/rust/hw/timer/hpet/meson.build
@@ -4,9 +4,10 @@ _libhpet_rs = static_library(
   override_options: ['rust_std=2021', 'build.rust_std=2021'],
   rust_abi: 'rust',
   dependencies: [
-    qemu_api,
+    qemu_api_system,
     qemu_api_macros,
   ],
+  rust_dependency_map: {'qemu_api_system': 'qemu_api'},
 )
 
 rust_devices_ss.add(when: 'CONFIG_X_HPET_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 57747bc934..bc0393add4 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -25,6 +25,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 bcf1cf780f..e0a3052c79 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -12,44 +12,64 @@ 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/chardev.rs',
+  'src/c_str.rs',
+  'src/errno.rs',
+  'src/module.rs',
+  'src/offset_of.rs',
+  'src/prelude.rs',
+  'src/qom.rs',
+  'src/timer.rs',
+  'src/zeroable.rs',
+]
+sources_system = sources_core + [
+  'src/irq.rs',
+  'src/memory.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/chardev.rs',
-      'src/c_str.rs',
-      'src/errno.rs',
-      'src/irq.rs',
-      'src/memory.rs',
-      'src/module.rs',
-      'src/offset_of.rs',
-      'src/prelude.rs',
-      'src/qdev.rs',
-      'src/qom.rs',
-      'src/sysbus.rs',
-      'src/timer.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,
   dependencies: libc_dep,
 )
+_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"'],
+  dependencies: libc_dep,
+)
 
-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,
 )
 
@@ -66,7 +86,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 d2868639ff..d93848c80c 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -17,11 +17,15 @@
 
 //! `bindgen`-generated declarations.
 
-#[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"));
 
 // SAFETY: these are implemented in C; the bindings need to assert that the
 // BQL is taken, either directly or via `BqlCell` and `BqlRefCell`.
@@ -49,29 +53,33 @@ unsafe impl Sync for ObjectClass {}
 unsafe impl Send for Object {}
 unsafe impl Sync for Object {}
 
-unsafe impl Send for SysBusDevice {}
-unsafe impl Sync for SysBusDevice {}
-
-// SAFETY: this is a pure data struct
-unsafe impl Send for CoalescedMemoryRange {}
-unsafe impl Sync for CoalescedMemoryRange {}
-
-// SAFETY: these are constants and vtables; the Send and Sync requirements
-// are deferred to the unsafe callbacks that they contain
-unsafe impl Send for MemoryRegionOps {}
-unsafe impl Sync for MemoryRegionOps {}
-
 unsafe impl Send for Property {}
 unsafe impl Sync for Property {}
 
 unsafe impl Send for TypeInfo {}
 unsafe impl Sync for TypeInfo {}
 
-unsafe impl Send for VMStateDescription {}
-unsafe impl Sync for VMStateDescription {}
+#[cfg(feature="system")]
+mod system_impls {
+    use super::*;
+
+    unsafe impl Send for SysBusDevice {}
+    unsafe impl Sync for SysBusDevice {}
+
+    // SAFETY: this is a pure data struct
+    unsafe impl Send for CoalescedMemoryRange {}
+    unsafe impl Sync for CoalescedMemoryRange {}
+
+    // SAFETY: these are constants and vtables; the Send and Sync requirements
+    // are deferred to the unsafe callbacks that they contain
+    unsafe impl Send for MemoryRegionOps {}
+    unsafe impl Sync for MemoryRegionOps {}
+
+    unsafe impl Send for VMStateDescription {}
+    unsafe impl Sync for VMStateDescription {}
 
-unsafe impl Send for VMStateField {}
-unsafe impl Sync for VMStateField {}
+    unsafe impl Send for VMStateField {}
+    unsafe impl Sync for VMStateField {}
 
-unsafe impl Send for VMStateInfo {}
-unsafe impl Sync for VMStateInfo {}
+    unsafe impl Send for VMStateInfo {}
+}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 05f38b51d3..825443abde 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -20,14 +20,19 @@
 pub mod cell;
 pub mod chardev;
 pub mod errno;
+#[cfg(feature = "system")]
 pub mod irq;
+#[cfg(feature = "system")]
 pub mod memory;
 pub mod module;
 pub mod offset_of;
+#[cfg(feature = "system")]
 pub mod qdev;
 pub mod qom;
+#[cfg(feature = "system")]
 pub mod sysbus;
 pub mod timer;
+#[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 634acf37a8..94738869d1 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -11,6 +11,7 @@
 
 pub use crate::errno;
 
+#[cfg(feature="system")]
 pub use crate::qdev::DeviceMethods;
 
 pub use crate::qom::InterfaceType;
@@ -25,6 +26,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 47b6977828..bec5093366 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -58,6 +58,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_export]
 macro_rules! const_zero {
     // This macro to produce a type-generic zero constant is taken from the
@@ -80,6 +81,7 @@ union TypeAsBytes {
 }
 
 /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
+#[allow(unused)]
 #[macro_export]
 macro_rules! impl_zeroable {
     ($type:ty) => {
@@ -89,20 +91,23 @@ unsafe impl $crate::zeroable::Zeroable for $type {
     };
 }
 
-// bindgen does not derive Default here
-#[allow(clippy::derivable_impls)]
-impl Default for crate::bindings::VMStateFlags {
-    fn default() -> Self {
-        Self(0)
+#[cfg(feature = "system")]
+mod system_impls {
+    // bindgen does not derive Default here
+    #[allow(clippy::derivable_impls)]
+    impl Default for crate::bindings::VMStateFlags {
+        fn default() -> Self {
+            Self(0)
+        }
     }
-}
 
-impl_zeroable!(crate::bindings::Property__bindgen_ty_1);
-impl_zeroable!(crate::bindings::Property);
-impl_zeroable!(crate::bindings::VMStateFlags);
-impl_zeroable!(crate::bindings::VMStateField);
-impl_zeroable!(crate::bindings::VMStateDescription);
-impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
-impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
-impl_zeroable!(crate::bindings::MemoryRegionOps);
-impl_zeroable!(crate::bindings::MemTxAttrs);
+    impl_zeroable!(crate::bindings::Property__bindgen_ty_1);
+    impl_zeroable!(crate::bindings::Property);
+    impl_zeroable!(crate::bindings::VMStateFlags);
+    impl_zeroable!(crate::bindings::VMStateField);
+    impl_zeroable!(crate::bindings::VMStateDescription);
+    impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
+    impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
+    impl_zeroable!(crate::bindings::MemoryRegionOps);
+    impl_zeroable!(crate::bindings::MemTxAttrs);
+}
-- 
2.48.1

[PATCH v2 02/11] meson: Add rust_block_ss and link tools with it

[Kevin Wolf]

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 6ee1757828..a08d029032 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()
@@ -4217,7 +4218,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')
@@ -4357,15 +4362,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

[PATCH v2 03/11] rust: Add some block layer bindings

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 rust/wrapper.h                | 4 ++++
 rust/qemu-api/src/zeroable.rs | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/rust/wrapper.h b/rust/wrapper.h
index f203fd13ac..303d7bba7f 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -54,3 +54,7 @@ typedef enum memory_order {
 #include "qapi/error.h"
 #include "chardev/char-serial.h"
 #include "qemu/timer.h"
+#include "block/block.h"
+#include "block/block_int.h"
+#include "block/qdict.h"
+#include "qapi/qapi-visit-block-core.h"
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index bec5093366..4e1e54cf57 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -58,7 +58,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_export]
 macro_rules! const_zero {
     // This macro to produce a type-generic zero constant is taken from the
@@ -81,7 +80,6 @@ union TypeAsBytes {
 }
 
 /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
-#[allow(unused)]
 #[macro_export]
 macro_rules! impl_zeroable {
     ($type:ty) => {
@@ -111,3 +109,6 @@ fn default() -> Self {
     impl_zeroable!(crate::bindings::MemoryRegionOps);
     impl_zeroable!(crate::bindings::MemTxAttrs);
 }
+
+impl_zeroable!(crate::bindings::BlockDriver);
+impl_zeroable!(crate::bindings::BlockDriver__bindgen_ty_1);
-- 
2.48.1

[PATCH v2 04/11] rust/qemu-api: Add wrappers to run futures in QEMU

[Kevin Wolf]

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  | 77 +++++++++++++++++++++++++++++++++++
 rust/qemu-api/src/lib.rs      |  1 +
 util/meson.build              |  3 ++
 7 files changed, 162 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 303d7bba7f..3dc385e256 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -58,3 +58,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 e0a3052c79..44fd34e193 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -22,6 +22,7 @@ sources_core = [
   'src/chardev.rs',
   'src/c_str.rs',
   'src/errno.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..cd307a1d62
--- /dev/null
+++ b/rust/qemu-api/src/futures.rs
@@ -0,0 +1,77 @@
+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, Waker};
+
+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 = Waker::from(Arc::new(RunFutureWaker {
+        co: unsafe { bindings::qemu_coroutine_self() },
+    }));
+    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.write(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 825443abde..84928905f1 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -20,6 +20,7 @@
 pub mod cell;
 pub mod chardev;
 pub mod errno;
+pub mod futures;
 #[cfg(feature = "system")]
 pub mod irq;
 #[cfg(feature = "system")]
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

[PATCH v2 05/11] rust/block: Add empty crate

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 MAINTAINERS            |  1 +
 rust/Cargo.lock        |  8 ++++++++
 rust/Cargo.toml        |  1 +
 rust/block/Cargo.toml  | 16 ++++++++++++++++
 rust/block/README.md   |  3 +++
 rust/block/meson.build | 20 ++++++++++++++++++++
 rust/block/src/lib.rs  |  1 +
 rust/meson.build       |  1 +
 8 files changed, 51 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/MAINTAINERS b/MAINTAINERS
index 3848d37a38..97cc04ae32 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2874,6 +2874,7 @@ S: Supported
 F: block*
 F: block/
 F: hw/block/
+F: rust/block/
 F: qapi/block*.json
 F: qapi/transaction.json
 F: include/block/
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 2ebf0a11ea..a2525052dd 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -31,6 +31,14 @@ dependencies = [
  "syn",
 ]
 
+[[package]]
+name = "block"
+version = "0.1.0"
+dependencies = [
+ "libc",
+ "qemu_api",
+]
+
 [[package]]
 name = "either"
 version = "1.12.0"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 5041d6291f..2135273dfb 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",
     "hw/timer/hpet",
 ]
diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
new file mode 100644
index 0000000000..fbc2f2d6ef
--- /dev/null
+++ b/rust/block/Cargo.toml
@@ -0,0 +1,16 @@
+[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" }
+libc = "0.2.162"
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..ca93afd939
--- /dev/null
+++ b/rust/block/meson.build
@@ -0,0 +1,20 @@
+_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,
+    libc_dep,
+  ],
+  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

[PATCH v2 06/11] rust/block: Add I/O buffer traits

[Kevin Wolf]

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

[PATCH v2 07/11] block: Add bdrv_open_blockdev_ref_file()

[Kevin Wolf]

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                            | 31 +++++++++++++++++++++++++++---
 2 files changed, 32 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..cd53c84f84 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,35 @@ 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

[PATCH v2 08/11] rust/block: Add driver module

[Kevin Wolf]

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 | 195 +++++++++++++++++++++++++++++++++++++++
 rust/block/src/lib.rs    |   1 +
 2 files changed, 196 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..fe19f4b88f
--- /dev/null
+++ b/rust/block/src/driver.rs
@@ -0,0 +1,195 @@
+// 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,
+}
+
+// TODO Represent the graph lock and let the compiler verify it's held when accessing child
+unsafe impl Send for BdrvChild {}
+unsafe impl Sync for 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: Box<MaybeUninit<T>>,
+    ) -> io::Result<Box<T>> {
+        unsafe {
+            self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
+                .await?;
+            let ptr = Box::into_raw(buf) as *mut T;
+            Ok(Box::from_raw(ptr))
+        }
+    }
+}
+
+#[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 -libc::EINVAL,
+            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 -libc::EINVAL,
+            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

[PATCH v2 09/11] rust/block: Add read support for block drivers

[Kevin Wolf]

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 | 95 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
index fe19f4b88f..022d50ffbc 100644
--- a/rust/block/src/driver.rs
+++ b/rust/block/src/driver.rs
@@ -9,10 +9,45 @@
 
 use crate::{IoBuffer, SizedIoBuffer};
 use qemu_api::bindings;
+use qemu_api::errno::Errno;
+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;
+use std::sync::Arc;
+
+/// 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.
+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: Arc<BdrvChild>,
+
+        /// 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.
 ///
@@ -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.
@@ -166,6 +206,60 @@ 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 -i32::from(Errno::from(e).0),
+        };
+
+        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 {
+                let ret = bindings::bdrv_co_preadv_part(
+                    node.child,
+                    (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
@@ -179,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

[PATCH v2 10/11] bochs-rs: Add bochs block driver reimplementation in Rust

[Kevin Wolf]

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  | 297 +++++++++++++++++++++++++++++++++++++++
 rust/block/src/driver.rs |   5 -
 rust/block/src/lib.rs    |   1 +
 4 files changed, 299 insertions(+), 6 deletions(-)
 create mode 100644 rust/block/src/bochs.rs

diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
index fbc2f2d6ef..b91483aed1 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..9dd84446e1
--- /dev/null
+++ b/rust/block/src/bochs.rs
@@ -0,0 +1,297 @@
+// 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::errno::Errno;
+use qemu_api::futures::qemu_run_future;
+use std::cmp::min;
+use std::io::{self, Error, ErrorKind};
+use std::mem::MaybeUninit;
+use std::ptr;
+use std::sync::Arc;
+
+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: Arc<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, Box::new(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: Arc::new(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 -libc::EINVAL,
+            };
+        }
+
+        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.write(bdrv);
+                    0
+                },
+                Err(e) => -i32::from(Errno::from(e).0),
+            }
+        })
+    }
+
+    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: self.file.clone(),
+                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 022d50ffbc..baeaf47eda 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::errno::Errno;
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

[PATCH v2 11/11] rust/block: Add format probing

[Kevin Wolf]

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 9dd84446e1..b1247a6bd5 100644
--- a/rust/block/src/bochs.rs
+++ b/rust/block/src/bochs.rs
@@ -188,6 +188,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 baeaf47eda..1b132bc8de 100644
--- a/rust/block/src/driver.rs
+++ b/rust/block/src/driver.rs
@@ -7,7 +7,7 @@
 use qemu_api::errno::Errno;
 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;
 
@@ -161,6 +171,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

Re: [PATCH v2 09/11] rust/block: Add read support for block drivers

[Paolo Bonzini]

On 2/18/25 19:20, Kevin Wolf wrote:
> +    /// The described blocks are stored in a child node.
> +    Data {
> +        /// Child node in which the data is stored
> +        node: Arc<BdrvChild>,

Having Arc<> here shouldn't be necessary, since the BdrvChild is already 
reference counted.  Since the code is called under the bdrv_graph_rdlock 
there's no risk of the BdrvChild going away, and you can just make it a 
&BdrvChild.

Likewise, even BochsImage should not need a standard Rust 
Arc<BdrvChild>.  However you need to add your own block::Arc<BdrvChild> 
and map Clone/Drop to bdrv_ref/bdrv_unref.  Then BochsImage can use 
block::Arc<BdrvChild>; this makes it even clearer that Mapping should 
not use the Arc<> wrapper, because bdrv_ref is GLOBAL_STATE_CODE() and 
would abort if run from a non-main thread.

That said, I'm not sure how to include "block graph lock must be taken" 
into the types, yet.  That has to be taken into account too, sooner or 
later.  You probably have a lot of items like this one so it'd be nice 
to have TODO comments as much as you can.

(This boundary is where you get an unholy mix of C and Rust concepts. 
It takes a while to get used to, and it teaches you a lot of the parts 
of Rust that you usually take for granted.  So while it's not hard, it's 
unusual and it does feel like water and oil in the beginning).

> +) -> std::os::raw::c_int {
> +    let s = unsafe { &mut *((*bs).opaque as *mut D) };

&mut is not safe here (don't worry, we went through the same thing for 
devices :)).  You can only get an & unless you go through an UnsafeCell 
(or something that contains one).  You'll need to split the mutable and 
immutable parts of BochsImage in separate structs, and embed the former 
into the latter.  Long term you there should be a 
qemu_api::coroutine::CoMutex<>, but for the short term you can just use 
a BqlRefCell<> or a standard Rust RefCell<>.  You can see how 
PL011Registers is included into PL011State in 
rust/hw/char/pl011/src/device.rs, and a small intro is also present in 
docs/devel/rust.rst.

Anyway, the BdrvChild needs to remain in BochsImage, so that it is 
accessible outside the CoMutex critical section and can be placed into 
the Mapping.

> +    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 -i32::from(Errno::from(e).0),

This is indeed not great, but it's partly so because you're doing a lot 
(for some definition of "a lot") in the function.  While it would be 
possible to use a trait, I wrote the API thinking of minimal glue code 
that only does the C<->Rust conversion.

In this case, because you have a lot more code than just a call into the 
BlockDriver trait, you'd have something like

fn bdrv_co_preadv_part(
     bs: &dyn BlockDriver,
     offset: i64,
     bytes: i64,
     qiov: &bindings::QEMUIOVector,
     mut qiov_offset: usize,
     flags: bindings::BdrvRequestFlags) -> io::Result<()>

and then a wrapper (e.g. rust_co_preadv_part?) that only does

    let s = unsafe { &mut *((*bs).opaque as *mut D) };
    let qiov = unsafe { &*qiov };
    let result = bdrv_co_preadv_part(s, offset, bytes,
          qiov, qiov_offset, flags);
    errno::into_negative_errno(result)

This by the way has also code size benefits because &dyn, unlike 
generics, does not need to result in duplicated code.

For now, I'd rather keep into_negative_errno() this way, to keep an eye 
on other cases where you have an io::Error<()>.  Since Rust rarely has 
Error objects that aren't part of a Result, it stands to reason that the 
same is true of QEMU code, but if I'm wrong then it can be changed.

Paolo

Re: [PATCH v2 05/11] rust/block: Add empty crate

[Zhao Liu]

> diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
> new file mode 100644
> index 0000000000..fbc2f2d6ef
> --- /dev/null
> +++ b/rust/block/Cargo.toml
> @@ -0,0 +1,16 @@
> +[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 = []

Per the commit f26137893b98 ("rust: remove unnecessary Cargo.toml
metadata"), readme & repository are not necessary. :-)

And rust-version is needed (since the commit 669fab6a1f3e).

> +[dependencies]
> +qemu_api = { path = "../qemu-api" }
> +libc = "0.2.162"

I think it's necessary add the lints section to apply workspace's lint
configurations:

[lints]
workspace = true

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

And doc can be updated in docs/devel/rust.rst, like Paolo's patch:

https://lore.kernel.org/qemu-devel/20250218080455.426383-1-pbonzini@redhat.com/

> diff --git a/rust/block/meson.build b/rust/block/meson.build
> new file mode 100644
> index 0000000000..ca93afd939
> --- /dev/null
> +++ b/rust/block/meson.build
> @@ -0,0 +1,20 @@
> +_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,

I'm unsure without testing, the dependencies here looks different with
the Cargo.toml?

Regards,
Zhao

Re: [PATCH v2 09/11] rust/block: Add read support for block drivers

[Kevin Wolf]

Am 19.02.2025 um 07:11 hat Paolo Bonzini geschrieben:
> On 2/18/25 19:20, Kevin Wolf wrote:
> > +    /// The described blocks are stored in a child node.
> > +    Data {
> > +        /// Child node in which the data is stored
> > +        node: Arc<BdrvChild>,
> 
> Having Arc<> here shouldn't be necessary, since the BdrvChild is already
> reference counted.  Since the code is called under the bdrv_graph_rdlock
> there's no risk of the BdrvChild going away, and you can just make it a
> &BdrvChild.

That would mean that you need keep the BlockDriver borrowed as long as
you're using the mapping. It would work today, but as soon as I want to
cache mappings, it won't any more.

> Likewise, even BochsImage should not need a standard Rust Arc<BdrvChild>.
> However you need to add your own block::Arc<BdrvChild> and map Clone/Drop to
> bdrv_ref/bdrv_unref.  Then BochsImage can use block::Arc<BdrvChild>; this
> makes it even clearer that Mapping should not use the Arc<> wrapper, because
> bdrv_ref is GLOBAL_STATE_CODE() and would abort if run from a non-main
> thread.

It's not BdrvChild that is refcounted on the C side, but
BlockDriverState. We definitely don't bdrv_ref()/unref() for each
request on the C side and we shouldn't on the Rust side either. The
refcount only changes when you modify the graph.

I'm not entirely sure how your block::Arc<T> is supposed to work. It
would be tied to one specific type (BlockDriverState), not generic.
Which probably means that it can't be a separate pointer type, but
BlockDriverState itself should just implement Clone with bdrv_ref().

Though that doesn't help here, obviously, because we have a BdrvChild.

> That said, I'm not sure how to include "block graph lock must be taken" into
> the types, yet.  That has to be taken into account too, sooner or later.
> You probably have a lot of items like this one so it'd be nice to have TODO
> comments as much as you can.

Actually, I'm not aware of that many items. But yes, there is a TODO
item for the graph lock.

I think I'll have something like:

    pub struct BdrvChild {
        child: GraphLock<*mut bindings::BdrvChild>,
    }

where you can access the inner object either by calling a lock function,
or passing another graph lock guard that you already own. And for the
FFI boundary unsafe functions like "I promise I already own the lock".

> (This boundary is where you get an unholy mix of C and Rust concepts. It
> takes a while to get used to, and it teaches you a lot of the parts of Rust
> that you usually take for granted.  So while it's not hard, it's unusual and
> it does feel like water and oil in the beginning).
> 
> > +) -> std::os::raw::c_int {
> > +    let s = unsafe { &mut *((*bs).opaque as *mut D) };
> 
> &mut is not safe here (don't worry, we went through the same thing for
> devices :)).  You can only get an & unless you go through an UnsafeCell (or
> something that contains one).

Right, we can have multiple requests in flight.

The fix is easy here: Even though bindgen gives us a *mut, we only want
a immutable reference.

> You'll need to split the mutable and immutable parts of BochsImage in
> separate structs, and embed the former into the latter.  Long term you
> there should be a qemu_api::coroutine::CoMutex<>, but for the short
> term you can just use a BqlRefCell<> or a standard Rust RefCell<>.
> You can see how PL011Registers is included into PL011State in
> rust/hw/char/pl011/src/device.rs, and a small intro is also present in
> docs/devel/rust.rst.

There is no mutable part in BochsImage, which makes this easy. The only
thing is the *mut bindings::BdrvChild, but we never dereference that in
Rust. It is also essentially interior mutability protected by the graph
lock, even though this isn't explicit yet.

But if we were to introduce a mutable part (I think we will add write
support to it sooner or later), then BqlRefCell or RefCell are
definitely not right. They would only turn the UB into a safe panic when
you have more than one request in flight. (Or actually, BqlRefCell
should already panic with just one request from an iothread, because we
don't actually hold the BQL.)

> Anyway, the BdrvChild needs to remain in BochsImage, so that it is
> accessible outside the CoMutex critical section and can be placed into
> the Mapping.
> 
> > +    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 -i32::from(Errno::from(e).0),
> 
> This is indeed not great, but it's partly so because you're doing a
> lot (for some definition of "a lot") in the function.  While it would
> be possible to use a trait, I wrote the API thinking of minimal glue
> code that only does the C<->Rust conversion.
> 
> In this case, because you have a lot more code than just a call into
> the BlockDriver trait, you'd have something like
> 
> fn bdrv_co_preadv_part(
>     bs: &dyn BlockDriver,
>     offset: i64,
>     bytes: i64,
>     qiov: &bindings::QEMUIOVector,
>     mut qiov_offset: usize,
>     flags: bindings::BdrvRequestFlags) -> io::Result<()>
> 
> and then a wrapper (e.g. rust_co_preadv_part?) that only does
> 
>    let s = unsafe { &mut *((*bs).opaque as *mut D) };
>    let qiov = unsafe { &*qiov };
>    let result = bdrv_co_preadv_part(s, offset, bytes,
>          qiov, qiov_offset, flags);
>    errno::into_negative_errno(result)
> 
> This by the way has also code size benefits because &dyn, unlike
> generics, does not need to result in duplicated code.

I don't really like the aesthetics of having two functions on the
Rust side for each C function, but I guess ugliness is expected in
bindings...

For the errno conversion functions, I'm still not sure that they should
only support trivial wrappers with no early returns. I reluctantly buy
your &dyn argument (though in C block drivers this is generally
duplicated code, too), but that's unrelated to error handling.

> For now, I'd rather keep into_negative_errno() this way, to keep an
> eye on other cases where you have an io::Error<()>.  Since Rust rarely
> has Error objects that aren't part of a Result, it stands to reason
> that the same is true of QEMU code, but if I'm wrong then it can be
> changed.

This one is part of a Result, too. But not a result that is directly
returned in both success and error cases, but where the error leads to
an early return. That is, an equivalent for the ubiquitous pattern:

    ret = foo();
    if (ret < 0) {
        return ret;
    }

    /* Do something with the successful result of foo() */

Kevin

Re: [PATCH v2 09/11] rust/block: Add read support for block drivers

[Paolo Bonzini]

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

Il mer 19 feb 2025, 14:02 Kevin Wolf <kwolf@redhat.com> ha scritto:

> > Likewise, even BochsImage should not need a standard Rust Arc<BdrvChild>.
> > However you need to add your own block::Arc<BdrvChild> and map
> Clone/Drop to
> > bdrv_ref/bdrv_unref.  Then BochsImage can use block::Arc<BdrvChild>; this
> > makes it even clearer that Mapping should not use the Arc<> wrapper,
> because
> > bdrv_ref is GLOBAL_STATE_CODE() and would abort if run from a non-main
> > thread.
>
> It's not BdrvChild that is refcounted on the C side, but
> BlockDriverState. We definitely don't bdrv_ref()/unref() for each
> request on the C side and we shouldn't on the Rust side either. The
> refcount only changes when you modify the graph.
>

I keep confusing BdrvChild and BlockDriverState, sorry.

Talking in general about Rust and not QEMU, if you wanted to be able to
optionally store the mapping, and not always modify the refcount, you'd
probably want an &Arc<T>. Then you only clone it if needed for the cache
(for example if you cached the *content* rather than the mapping, you
wouldn't need to clone).

However, Arc doesn't work here. In order to globally invalidate all cached
mappings, you would need the cache to store something similar to a Weak<T>.
But even that doesn't quite work, because if you passed an &Arc the caller
could clone it as it wished. So using weak clones to do cache invalidation
shows as the hack that it is.

Going back to QEMU, the Mapping needs some smart pointer (something that
implements Deref<Target = bindings;:BdrvChild>), which does implement some
reference counting unlike the C version but, unlike &Arc<BdrvChild>, cannot
be cloned willy nilly. Instead it can be 1) weakly-cloned under the graph
rdlock 2) used from a weak reference but only for the duration of an rdlock
critical section (no way to make it strong for an arbitrary lifetime!) 3)
invalidated under the graph wrlock.

The nice thing here is that, even if you have a good way to invalidate the
cache, that's orthogonal to how you make the Rust API memory safe. Cache
invalidation becomes just a way to quickly free the BdrvChild—the invalid
entry would be detected anyway later when trying to use the weak reference.

More practical/immediate suggestion below...

I'm not entirely sure how your block::Arc<T> is supposed to work. It
> would be tied to one specific type (BlockDriverState), not generic.
> Which probably means that it can't be a separate pointer type, but
> BlockDriverState itself should just implement Clone with bdrv_ref().
>

You're right, I always forget the new BdrvChild world.

> That said, I'm not sure how to include "block graph lock must be taken"
> into
> > the types, yet.  That has to be taken into account too, sooner or later.
> > You probably have a lot of items like this one so it'd be nice to have
> TODO
> > comments as much as you can.
>
> Actually, I'm not aware of that many items.

But yes, there is a TODO
> item for the graph lock.
>
> I think I'll have something like:
>
>     pub struct BdrvChild {
>         child: GraphLock<*mut bindings::BdrvChild>,
>     }
>

Arc<BdrvChild> poses another problem then, in that graph changes would
invalidate the raw pointer even if the Arc is still alive. Something like
the aforementioned smart pointer would prevent the cache from accessing a
dead pointer (at the cost of adding a refcount field to BdrvChild that C
doesn't use).

But for now maybe you can just rename the *mut-wrapping BdrvChild to
BdrvChildRef, get rid of Arc, and store an &BdrvChildRef into Mapping? Then
if you ever decide to go with the refcounting plan you can implement Deref.

> > +) -> std::os::raw::c_int {
> > > +    let s = unsafe { &mut *((*bs).opaque as *mut D) };
> >
> > &mut is not safe here (don't worry, we went through the same thing for
> > devices :)).  You can only get an & unless you go through an UnsafeCell
> (or
> > something that contains one).
>
> Right, we can have multiple requests in flight.
> The fix is easy here: Even though bindgen gives us a *mut, we only want
> a immutable reference.
>

Right.

There is no mutable part in BochsImage, which makes this easy. [...] But if
> we were to introduce a mutable part (I think we will add write

support to it sooner or later), then BqlRefCell or RefCell are
> definitely not right. They would only turn the UB into a safe panic when
> you have more than one request in flight. (Or actually, BqlRefCell
> should already panic with just one request from an iothread, because we
> don't actually hold the BQL.)
>

Yes, I mentioned RefCell because of the iothread case but I agree it also
isn't right. It wouldn't panic when you have more than one request in
flight however, as long as only map() needs to borrow_mut(). If instead you
need slightly more complex locking, for example similar to the vbox driver,
you need CoMutex/CoRwLock bindings.

> > +    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 -i32::from(Errno::from(e).0),
> >
> > This is indeed not great, but it's partly so because you're doing a
> > lot (for some definition of "a lot") in the function.  While it would
> > be possible to use a trait, I wrote the API thinking of minimal glue
> > code that only does the C<->Rust conversion.
> >
> > In this case, because you have a lot more code than just a call into
> > the BlockDriver trait, you'd have something like
> >
> > fn bdrv_co_preadv_part(
> >     bs: &dyn BlockDriver,
> >     offset: i64,
> >     bytes: i64,
> >     qiov: &bindings::QEMUIOVector,
> >     mut qiov_offset: usize,
> >     flags: bindings::BdrvRequestFlags) -> io::Result<()>
> >
> > and then a wrapper (e.g. rust_co_preadv_part?) that only does
> >
> >    let s = unsafe { &mut *((*bs).opaque as *mut D) };
> >    let qiov = unsafe { &*qiov };
> >    let result = bdrv_co_preadv_part(s, offset, bytes,
> >          qiov, qiov_offset, flags);
> >    errno::into_negative_errno(result)
> >
> > This by the way has also code size benefits because &dyn, unlike
> > generics, does not need to result in duplicated code.
>
> I don't really like the aesthetics of having two functions on the
> Rust side for each C function, but I guess ugliness is expected in
> bindings...
>

Well, you don't *have* to have two. In this case I suggested two just
because the C and Rust APIs are completely different. But also...

> For now, I'd rather keep into_negative_errno() this way, to keep an
> > eye on other cases where you have an io::Error<()>.  Since Rust rarely
> > has Error objects that aren't part of a Result, it stands to reason
> > that the same is true of QEMU code, but if I'm wrong then it can be
> > changed.
>
> This one is part of a Result, too. But not a result that is directly
> returned in both success and error cases, but where the error leads to
> an early return. That is, an equivalent for the ubiquitous pattern:
>
>     ret = foo();
>     if (ret < 0) {
>         return ret;
>     }
>

... this is just "?" in Rust and it's the C/Rust boundary that complicates
things, because "?" assumes that you return another Result. So the
two-function idea helps because the inner function can just use "?", and
the outer one qemu_api::errno. It let's you use the language more
effectively. &dyn is just an addition on top.

If needed your inner function could return Result<T, Errno> instead of
Result<T, io::Error>, too. That works nicely because "?" would also convert
io::Error into Errno as needed.

>
But if you don't want the two functions, why wouldn't you just do "e =>
..." in the match statement instead of Err(e)?

Paolo

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

Re: [PATCH v2 04/11] rust/qemu-api: Add wrappers to run futures in QEMU

[Zhao Liu]

> +/// 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 = Waker::from(Arc::new(RunFutureWaker {
> +        co: unsafe { bindings::qemu_coroutine_self() },
> +    }));
> +    let mut cx = Context::from_waker(&waker);
> +
> +    let mut pinned_future = std::pin::pin!(future);

pin macro stabilized in v1.68.0, but currently the minimum rustc
supported by QEMU is v1.63.

I found there's a workaround [*], so we can add a temporary pin.rs in
qemu_api until QEMU bumps up rustc to >= v1.68?

[*]: https://github.com/rust-lang/rust/issues/93178#issuecomment-1386177439

> +    loop {
> +        match pinned_future.as_mut().poll(&mut cx) {
> +            Poll::Ready(res) => return res,
> +            Poll::Pending => unsafe {
> +                bindings::qemu_coroutine_yield();
> +            },
> +        }
> +    }
> +}
> +

Re: [PATCH v2 08/11] rust/block: Add driver module

[Zhao Liu]

> +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(),

c literal starts from v1.77, and there's a workaround provided by
c_str.rs:

c_str!("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 })
> +            }
> +        }
> +    }
> +

Re: [PATCH v2 10/11] bochs-rs: Add bochs block driver reimplementation in Rust

[Zhao Liu]

> +use crate::driver::{block_driver, BdrvChild, BlockDriver, Mapping, MappingTarget, Request};
> +use crate::SizedIoBuffer;
> +use qemu_api::bindings;

it's better to list all items from bindings here, which helps in understanding
which parts will need a wrapper added later.

> +use qemu_api::errno::Errno;
> +use qemu_api::futures::qemu_run_future;
> +use std::cmp::min;
> +use std::io::{self, Error, ErrorKind};
> +use std::mem::MaybeUninit;
> +use std::ptr;
> +use std::sync::Arc;

nit: we can fix style by "cargo +nightly fmt" with latest toolchain.

> +impl BlockDriver for BochsImage {

...

> +    async fn map(&self, req: &Request) -> io::Result<Mapping> {

v1.63 doesn't support async fn in trait, do we have to involve
async_trait crate?

Regards,
Zhao

Re: [PATCH v2 01/11] rust: Build separate qemu_api_tools and qemu_api_system

[Zhao Liu]

> diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
> index 57747bc934..bc0393add4 100644
> --- a/rust/qemu-api/Cargo.toml
> +++ b/rust/qemu-api/Cargo.toml
> @@ -25,6 +25,7 @@ version_check = "~0.9"
>  default = ["debug_cell"]
>  allocator = []
>  debug_cell = []
> +system= []

With this new feature, we also need to declear the this feature flag to
pl011 & hpet's Cargo.toml:

qemu_api = { path = "../../../qemu-api", features = ["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"
> +    };
> +

Missing to update rust/qemu-api/.gitignore :-)

(I'm still learning your series and currently just trying out the compilation.)

Thanks,
Zhao

Re: [PATCH v2 04/11] rust/qemu-api: Add wrappers to run futures in QEMU

[Kevin Wolf]

Am 20.02.2025 um 07:35 hat Zhao Liu geschrieben:
> > +/// 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 = Waker::from(Arc::new(RunFutureWaker {
> > +        co: unsafe { bindings::qemu_coroutine_self() },
> > +    }));
> > +    let mut cx = Context::from_waker(&waker);
> > +
> > +    let mut pinned_future = std::pin::pin!(future);
> 
> pin macro stabilized in v1.68.0, but currently the minimum rustc
> supported by QEMU is v1.63.

Can we check this automatically somehow? I actually seem to remember
that I got errors for too new things before. Is the problem here that
it's a macro?

> I found there's a workaround [*], so we can add a temporary pin.rs in
> qemu_api until QEMU bumps up rustc to >= v1.68?
> 
> [*]: https://github.com/rust-lang/rust/issues/93178#issuecomment-1386177439

I don't think we'll need this anywhere else, so I can just open-code
what the macro does:

    // TODO Use std::pin::pin! when MSRV is updated to at least 1.68.0
    // SAFETY: `future` is not used any more after this and dropped at the end of the function.
    let mut pinned_future = unsafe { std::pin::Pin::new_unchecked(&mut future)};

Kevin

Re: [PATCH v2 04/11] rust/qemu-api: Add wrappers to run futures in QEMU

[Stefan Hajnoczi]

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

On Tue, Feb 18, 2025 at 07:20:12PM +0100, Kevin Wolf wrote:
> This adds helper functions that allow running Rust futures to completion
> using QEMU's event loops.

This commit is a cliff-hanger. I'm intrigued to find out how timer, fd,
etc event loop integration will work :).

> 
> 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  | 77 +++++++++++++++++++++++++++++++++++
>  rust/qemu-api/src/lib.rs      |  1 +
>  util/meson.build              |  3 ++
>  7 files changed, 162 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);

This adds a blocking (aio_poll()-style) API. The more blocking APIs we
add, the more points are created where QEMU hangs when the async
operation doesn't complete in a reasonable amount of time. It would be
best to avoid introducing new blocking APIs, but sometimes it is
unavoidable.

rust_run_future() is very generic and I think the downsides should be
pointed out to discourage people from using it when not absolutely
necessary. Can you document when it's appropriate to use this API?

> +
> +#endif
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 303d7bba7f..3dc385e256 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -58,3 +58,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 e0a3052c79..44fd34e193 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -22,6 +22,7 @@ sources_core = [
>    'src/chardev.rs',
>    'src/c_str.rs',
>    'src/errno.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..cd307a1d62
> --- /dev/null
> +++ b/rust/qemu-api/src/futures.rs
> @@ -0,0 +1,77 @@
> +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, Waker};
> +
> +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 = Waker::from(Arc::new(RunFutureWaker {
> +        co: unsafe { bindings::qemu_coroutine_self() },
> +    }));
> +    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.write(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

rust_run_future() has GLOBAL_STATE_CODE() so qemu_run_future() needs to
run not just outside coroutine context, but also under the BQL. Should
this be mentioned?

> +/// 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 _),

This line is hard to follow. I think it's casting to the C equivalent
type:

 void coroutine_fn (*)(RustBoxedFuture *future, void *opaque)

I wonder if there's a clearer way of writing this. Maybe being explicit
rather than implicit here would be helpful.

If not, it's not a big deal, but I spent some time trying to figure out
what this does and others might too.

> +            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 825443abde..84928905f1 100644
> --- a/rust/qemu-api/src/lib.rs
> +++ b/rust/qemu-api/src/lib.rs
> @@ -20,6 +20,7 @@
>  pub mod cell;
>  pub mod chardev;
>  pub mod errno;
> +pub mod futures;
>  #[cfg(feature = "system")]
>  pub mod irq;
>  #[cfg(feature = "system")]
> 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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 08/11] rust/block: Add driver module

[Stefan Hajnoczi]

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

On Tue, Feb 18, 2025 at 07:20:16PM +0100, Kevin Wolf wrote:
> 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 | 195 +++++++++++++++++++++++++++++++++++++++
>  rust/block/src/lib.rs    |   1 +
>  2 files changed, 196 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..fe19f4b88f
> --- /dev/null
> +++ b/rust/block/src/driver.rs
> @@ -0,0 +1,195 @@
> +// 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;

I wonder if it makes sense to separate different parts of the
BlockDriver API into multiple traits:
1. Pre-open APIs (open(), probe()?, etc)
2. I/O path - Future or async-based and can run in any thread
3. Control path (size(), close(), reopen()?, etc)

That way the type system guarantees that sub-APIs that are only valid in
a specific state (e.g. when the BlockDriverState is open) cannot be
called.

size() is an example of a method that would benefit from more
fine-grained traits that keep states separated. size() -> u64 doesn't
make sense before open().

What do you think?

> +}
> +
> +/// 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,
> +}
> +
> +// TODO Represent the graph lock and let the compiler verify it's held when accessing child
> +unsafe impl Send for BdrvChild {}
> +unsafe impl Sync for 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: Box<MaybeUninit<T>>,
> +    ) -> io::Result<Box<T>> {
> +        unsafe {
> +            self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
> +                .await?;
> +            let ptr = Box::into_raw(buf) as *mut T;
> +            Ok(Box::from_raw(ptr))
> +        }
> +    }
> +}
> +
> +#[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 -libc::EINVAL,
> +            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 -libc::EINVAL,
> +            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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 09/11] rust/block: Add read support for block drivers

[Stefan Hajnoczi]

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

On Tue, Feb 18, 2025 at 07:20:17PM +0100, Kevin Wolf wrote:
> 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 | 95 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
> index fe19f4b88f..022d50ffbc 100644
> --- a/rust/block/src/driver.rs
> +++ b/rust/block/src/driver.rs
> @@ -9,10 +9,45 @@
>  
>  use crate::{IoBuffer, SizedIoBuffer};
>  use qemu_api::bindings;
> +use qemu_api::errno::Errno;
> +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;
> +use std::sync::Arc;
> +
> +/// 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.
> +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: Arc<BdrvChild>,
> +
> +        /// 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.
>  ///
> @@ -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>;

What are the constraints on the lifetime of returned mappings? I don't
mean a Rust lifetimes, but how long a returned mapping remains valid. I
guess at the moment returned mappings stay valid forever (i.e. the
BlockDriver cannot move data once it has been mapped)?

This becomes more interesting once write or discard requests are
supported or truncate() is implemented, but it would be worth spelling
out the lifetime of a mappings from the start and extending that model
later because it's an important assumption.

>  }
>  
>  /// Represents the connection between a parent and its child node.
> @@ -166,6 +206,60 @@ 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 -i32::from(Errno::from(e).0),
> +        };
> +
> +        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 {
> +                let ret = bindings::bdrv_co_preadv_part(
> +                    node.child,
> +                    (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
> @@ -179,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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 09/11] rust/block: Add read support for block drivers

[Stefan Hajnoczi]

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

On Tue, Feb 18, 2025 at 07:20:17PM +0100, Kevin Wolf wrote:
> +/// 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.
> +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: Arc<BdrvChild>,
> +
> +        /// 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.
>  ///
> @@ -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>;

Here are my thoughts about how the interface could evolve as more
functionality is added (beyond the scope of this patch). I'm sure you
have your own ideas that aren't in this patch series, so take it for
what it's worth.

The main point:
The current interface has Request, which tells the BlockDriver that this
is a Read. I think this interface is still based on I/O requests rather
than being about mappings. The caller should handle
read/write/discard/etc request logic themselves based on the mapping
information from the BlockDriver. Then the BlockDriver just worries
about mapping blocks rather than how to treat different types of
requests.

A sketch of the interface:
The interface consists of fetch(), alloc(), and update().

1. Fetch mappings located around a given range:
   fetch(offset, len) -> [Mapping]

   The returned array of mappings must cover <offset, len>, but it can
   also be larger. For example, it could return the entire extent that
   includes <offset, len>. Or it could even contain the <offset, len>
   mapping along with a bunch of other mappings.

   If the BlockDriver pays the cost of a metadata read operation to load
   mappings, then let's report all mappings even if they precede or
   follow the <offset, len> range. The exact amount of extra mappings
   reported depends on the BlockDriver implementation and the metadata
   layout, but in qcow2 it might be an L2 table, for example. The idea
   is that if the BlockDriver volunteers extra mapping information, then
   the caller can perform future operations without calling into the
   BlockDriver again.

   If it turns out that certain BlockDriver implementations don't
   benefit, that's okay, but let's include the flexibility to volunteer
   more mapping information than just <offset, len> in the interface.

2. Allocate new unmapped blocks:
   alloc(num_blocks) -> Result<(BdrvChild, Offset)>

   (A choice needs to be made whether the blocks are contiguous or not,
   I'm showing the contiguous function prototype here because it's
   simpler, but it could also support non-contiguous allocations.)

   These blocks are not mapped yet and the caller is expected to write
   to them eventually, populating them with data.

   I'm assuming the qcow2 crash consistency model where leaking clusters
   is acceptable here. Not sure whether this API makes sense for all
   other image formats, but please bear with me for the final part of
   the interface where everything fits together.

3. Update mapping:
   update(offset, len, MappingTarget) -> Result<()>

   This adds or changes mappings. It's the most complicated function and
   there are a lot of details about which inputs should be
   allowed/forbidden (e.g. creating completely new mappings, splitting
   existing mappings, toggling MappingTarget on an existing mapping).
   Perhaps in practice only a few valid combinations of inputs will be
   allowed rather than full ability to manipulate mappings.

   Anyway, here is how it can be used:
   - Allocating write. After blocks allocated with alloc() have been
     written with data, call update() to establish a mapping.
   - Discard. Change the MappingTarget of an existing mapping to unmap
     blocks. This frees blocks unless they should be anchored (aka
     pre-allocated).
   - Write to anchored blocks. If the mapping had blocks allocated but
     they were not mapped, then update() should be called after data has
     been written to the blocks to restore the mapping from anchored to
     normal mapped blocks.

The overall idea is that I/O requests are not part of the interface,
only mappings. The caller decides how to perform I/O requests and calls
the appropriate combination of fetch(), alloc(), and update() APIs to
ensure the necessary mapping state is present.

It will probably be necessary to add additional mapping information
indicating permissions, compression/encryption info, etc so that the
caller can process I/O requests rather than sending them into the
BlockDriver.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 10/11] bochs-rs: Add bochs block driver reimplementation in Rust

[Stefan Hajnoczi]

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

On Tue, Feb 18, 2025 at 07:20:18PM +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  | 297 +++++++++++++++++++++++++++++++++++++++
>  rust/block/src/driver.rs |   5 -
>  rust/block/src/lib.rs    |   1 +
>  4 files changed, 299 insertions(+), 6 deletions(-)
>  create mode 100644 rust/block/src/bochs.rs
> 
> diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml
> index fbc2f2d6ef..b91483aed1 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..9dd84446e1
> --- /dev/null
> +++ b/rust/block/src/bochs.rs
> @@ -0,0 +1,297 @@
> +// 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::errno::Errno;
> +use qemu_api::futures::qemu_run_future;
> +use std::cmp::min;
> +use std::io::{self, Error, ErrorKind};
> +use std::mem::MaybeUninit;
> +use std::ptr;
> +use std::sync::Arc;
> +
> +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

I just wanted to check if anyone knows whether the Rust standard library
has an endian enforcing type? It seems like an omission of a basic
standard library primitive. I've seen crates re-inventing this again and
again because the standard library only supports endianness conversion
but not a type that enforces safe conversion. Would be awesome if Rust
had this in the standard library and I may have missed recent
developments.

> +
> +#[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],

A compile-time check that BochsHeader == HEADER_SIZE would be nice. It
feels a little unsafe to have HEADER_SIZE and this explicit padding size
calculation without a check. And to be honest it took me a few moments
to verify the it's correct while reading the patch :).

> +}
> +
> +#[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: Arc<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, Box::new(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() },

I don't understand why the names are HEADER_VERSION vs HEADER_V1. Does
it make sense to rename them to HEADER_V1 and HEADER_V2?

> +            _ => 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: Arc::new(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 -libc::EINVAL,
> +            };
> +        }
> +
> +        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.write(bdrv);
> +                    0
> +                },
> +                Err(e) => -i32::from(Errno::from(e).0),
> +            }
> +        })
> +    }
> +
> +    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;

Why is bitmap_entry initialized to 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: self.file.clone(),
> +                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 022d50ffbc..baeaf47eda 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::errno::Errno;
> 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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 00/11] rust/block: Add minimal block driver bindings

[Stefan Hajnoczi]

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

On Tue, Feb 18, 2025 at 07:20:08PM +0100, Kevin Wolf wrote:
> This series adds minimal bindings for writing block drivers in Rust and
> converts the bochs block driver as an example. Some parts (such as the
> open functions) are still essentially C written in Rust syntax, while
> other parts already try to provide somewhat idiomatic abstractions.
> 
> The main purpose of this series is just to add a starting point for
> incremental improvements to the bindings; it's clearly not very polished
> yet and ignores some important points like enforcing the graph lock.
> 
> Therefore, the main focus in the immediate future after this series
> should be cleaning up the bochs driver and the bindings it uses rather
> than adding more drivers.
> 
> Based-on: <20250213143216.3910163-1-pbonzini@redhat.com>

Hi Kevin,
This is very cool: both the ability to write block drivers in Rust and
the new API design focussing on mappings instead of I/O request
processing.

I wonder whether BlockDriver should be called ImageFormatDriver instead
to differentiate it from protocol drivers (which are block drivers) as
they cannot be implemented with the mappings-based interface.

Stefan

> 
> v2:
> - Rebased on top of current git master with qemu_api::errno patches
>   applied
> - Changed 'node' in MappingTarget from a dummy () to Arc<BdrvChild>
> - Changed BdrvChild::read_uninit() to use Box<MaybeUninit<T>>
> - Use libc crate instead of letting bindgen output EINVAL/EIO
> - Fixed errno translation logic, in parts by using qemu_api::errno
> - Changed two instances of incorrect *foo = ... into foo.write(...)
> - Added rust/block/ to MAINTAINERS
> - Some style and readability improvements
> 
> Kevin Wolf (11):
>   rust: Build separate qemu_api_tools and qemu_api_system
>   meson: Add rust_block_ss and link tools with it
>   rust: Add some block layer bindings
>   rust/qemu-api: Add wrappers to run futures in QEMU
>   rust/block: Add empty crate
>   rust/block: Add I/O buffer traits
>   block: Add bdrv_open_blockdev_ref_file()
>   rust/block: Add driver module
>   rust/block: Add read support for block drivers
>   bochs-rs: Add bochs block driver reimplementation in Rust
>   rust/block: Add format probing
> 
>  include/block/block-global-state.h |   4 +
>  include/qemu/coroutine-rust.h      |  24 +++
>  rust/wrapper-system.h              |  46 +++++
>  rust/wrapper.h                     |  16 +-
>  block.c                            |  31 ++-
>  util/qemu-co-rust-async.c          |  55 +++++
>  MAINTAINERS                        |   1 +
>  meson.build                        |  47 ++++-
>  rust/Cargo.lock                    |   8 +
>  rust/Cargo.toml                    |   1 +
>  rust/block/Cargo.toml              |  16 ++
>  rust/block/README.md               |   3 +
>  rust/block/meson.build             |  20 ++
>  rust/block/src/bochs.rs            | 317 +++++++++++++++++++++++++++++
>  rust/block/src/driver.rs           | 309 ++++++++++++++++++++++++++++
>  rust/block/src/iobuffer.rs         |  94 +++++++++
>  rust/block/src/lib.rs              |   5 +
>  rust/hw/char/pl011/meson.build     |   3 +-
>  rust/hw/timer/hpet/meson.build     |   3 +-
>  rust/meson.build                   |  12 +-
>  rust/qemu-api/Cargo.toml           |   1 +
>  rust/qemu-api/build.rs             |  10 +-
>  rust/qemu-api/meson.build          |  80 +++++---
>  rust/qemu-api/src/bindings.rs      |  52 +++--
>  rust/qemu-api/src/futures.rs       |  77 +++++++
>  rust/qemu-api/src/lib.rs           |   6 +
>  rust/qemu-api/src/prelude.rs       |   3 +
>  rust/qemu-api/src/zeroable.rs      |  34 ++--
>  storage-daemon/meson.build         |   2 +-
>  util/meson.build                   |   3 +
>  30 files changed, 1188 insertions(+), 95 deletions(-)
>  create mode 100644 include/qemu/coroutine-rust.h
>  create mode 100644 rust/wrapper-system.h
>  create mode 100644 util/qemu-co-rust-async.c
>  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/bochs.rs
>  create mode 100644 rust/block/src/driver.rs
>  create mode 100644 rust/block/src/iobuffer.rs
>  create mode 100644 rust/block/src/lib.rs
>  create mode 100644 rust/qemu-api/src/futures.rs
> 
> -- 
> 2.48.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]