[PATCH 0/3] Small Rust memory API improvements

[PATCH 0/3] Small Rust memory API improvements

[Manos Pitsidianakis]

This series adds minor improvements to the memory API:

- Use an enum to define bit width for memory accesses instead of an
  integer
- Add *_with_attrs methods to MemoryRegionOps

The *_with_attrs methods are not currently used by any device, and that
patch can be dropped without issue.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Manos Pitsidianakis (3):
      rust/memory: replace size arg with Bits enum
      memattrs.h: make MemTxAttrs into enum
      rust: add *_with_attrs methods to MemoryRegionOps

 include/exec/memattrs.h          | 11 ++---
 rust/hw/char/pl011/src/device.rs |  8 ++--
 rust/hw/timer/hpet/src/device.rs | 14 +++----
 rust/qemu-api/meson.build        |  1 +
 rust/qemu-api/src/memory.rs      | 86 +++++++++++++++++++++++++++++++++++-----
 5 files changed, 93 insertions(+), 27 deletions(-)
---
base-commit: c77283dd5d79149f4e7e9edd00f65416c648ee59
change-id: 20250703-rust-mem-api-7faf5644cda2

--
γαῖα πυρί μιχθήτω

[PATCH 1/3] rust/memory: replace size arg with Bits enum

[Manos Pitsidianakis]

We have the ability to make memory accesses use a typesafe access width
type in Rust, which the C API currently lacks as it does not use a
newtype wrapper for specifying the amount of bytes a memory access has;
it uses a plain 32-bit integer value instead.

Replace use of u32 size arguments with a Bits enum that has only the
allowed byte sizes as variants and has a u32 representation so that it
can be fed back into C as well.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 rust/hw/char/pl011/src/device.rs |  8 ++++----
 rust/hw/timer/hpet/src/device.rs | 14 +++++++-------
 rust/qemu-api/src/memory.rs      | 34 ++++++++++++++++++++++++----------
 3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 5b53f2649f161287f40f79075afba47db6d9315c..0c146821fbec4d310963264b90bb2bf2d30b901d 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -10,7 +10,7 @@
     irq::{IRQState, InterruptSource},
     log::Log,
     log_mask_ln,
-    memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
+    memory::{hwaddr, Bits, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
     prelude::*,
     qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
     qom::{ObjectImpl, Owned, ParentField, ParentInit},
@@ -501,7 +501,7 @@ unsafe fn init(mut this: ParentInit<Self>) {
             .read(&PL011State::read)
             .write(&PL011State::write)
             .native_endian()
-            .impl_sizes(4, 4)
+            .impl_sizes(Bits::_32, Bits::_32)
             .build();
 
         // SAFETY: this and this.iomem are guaranteed to be valid at this point
@@ -534,7 +534,7 @@ fn post_init(&self) {
         }
     }
 
-    fn read(&self, offset: hwaddr, _size: u32) -> u64 {
+    fn read(&self, offset: hwaddr, _size: Bits) -> u64 {
         match RegisterOffset::try_from(offset) {
             Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
                 let device_id = self.get_class().device_id;
@@ -555,7 +555,7 @@ fn read(&self, offset: hwaddr, _size: u32) -> u64 {
         }
     }
 
-    fn write(&self, offset: hwaddr, value: u64, _size: u32) {
+    fn write(&self, offset: hwaddr, value: u64, _size: Bits) {
         let mut update_irq = false;
         if let Ok(field) = RegisterOffset::try_from(offset) {
             // qemu_chr_fe_write_all() calls into the can_receive
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index acf7251029e912f18a5690b0d6cf04ea8151c5e1..a94e128e302a57df709ef3643694308833791859 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -18,7 +18,7 @@
     cell::{BqlCell, BqlRefCell},
     irq::InterruptSource,
     memory::{
-        hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder, MEMTXATTRS_UNSPECIFIED,
+        hwaddr, Bits, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder, MEMTXATTRS_UNSPECIFIED,
     },
     prelude::*,
     qdev::{DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
@@ -703,8 +703,8 @@ unsafe fn init(mut this: ParentInit<Self>) {
                 .read(&HPETState::read)
                 .write(&HPETState::write)
                 .native_endian()
-                .valid_sizes(4, 8)
-                .impl_sizes(4, 8)
+                .valid_sizes(Bits::_32, Bits::_64)
+                .impl_sizes(Bits::_32, Bits::_64)
                 .build();
 
         MemoryRegion::init_io(
@@ -771,9 +771,9 @@ fn reset_hold(&self, _type: ResetType) {
         self.rtc_irq_level.set(0);
     }
 
-    fn decode(&self, mut addr: hwaddr, size: u32) -> HPETAddrDecode<'_> {
+    fn decode(&self, mut addr: hwaddr, size: Bits) -> HPETAddrDecode<'_> {
         let shift = ((addr & 4) * 8) as u32;
-        let len = std::cmp::min(size * 8, 64 - shift);
+        let len = std::cmp::min(size as u32 * 8, 64 - shift);
 
         addr &= !4;
         let reg = if (0..=0xff).contains(&addr) {
@@ -796,7 +796,7 @@ fn decode(&self, mut addr: hwaddr, size: u32) -> HPETAddrDecode<'_> {
         HPETAddrDecode { shift, len, reg }
     }
 
-    fn read(&self, addr: hwaddr, size: u32) -> u64 {
+    fn read(&self, addr: hwaddr, size: Bits) -> u64 {
         // TODO: Add trace point - trace_hpet_ram_read(addr)
         let HPETAddrDecode { shift, reg, .. } = self.decode(addr, size);
 
@@ -823,7 +823,7 @@ fn read(&self, addr: hwaddr, size: u32) -> u64 {
         }) >> shift
     }
 
-    fn write(&self, addr: hwaddr, value: u64, size: u32) {
+    fn write(&self, addr: hwaddr, value: u64, size: Bits) {
         let HPETAddrDecode { shift, len, reg } = self.decode(addr, size);
 
         // TODO: Add trace point - trace_hpet_ram_write(addr, value)
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index e40fad6cf19ea7971daf78afdf9ac886308ef5c3..b1907aa01300a3fac8e1f3b69c5d50da631a556d 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -20,6 +20,15 @@
     zeroable::Zeroable,
 };
 
+#[derive(Copy, Clone, Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
+#[repr(u32)]
+pub enum Bits {
+    _8 = 1,
+    _16 = 2,
+    _32 = 4,
+    _64 = 8,
+}
+
 pub struct MemoryRegionOps<T>(
     bindings::MemoryRegionOps,
     // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
@@ -41,32 +50,37 @@ unsafe impl<T: Sync> Sync for MemoryRegionOps<T> {}
 #[derive(Clone)]
 pub struct MemoryRegionOpsBuilder<T>(bindings::MemoryRegionOps, PhantomData<fn(&T)>);
 
-unsafe extern "C" fn memory_region_ops_read_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, u32), u64>>(
+unsafe extern "C" fn memory_region_ops_read_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, Bits), u64>>(
     opaque: *mut c_void,
     addr: hwaddr,
     size: c_uint,
 ) -> u64 {
+    let size = Bits::try_from(size).expect("invalid size argument");
     F::call((unsafe { &*(opaque.cast::<T>()) }, addr, size))
 }
 
-unsafe extern "C" fn memory_region_ops_write_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, u64, u32)>>(
+unsafe extern "C" fn memory_region_ops_write_cb<
+    T,
+    F: for<'a> FnCall<(&'a T, hwaddr, u64, Bits)>,
+>(
     opaque: *mut c_void,
     addr: hwaddr,
     data: u64,
     size: c_uint,
 ) {
+    let size = Bits::try_from(size).expect("invalid size argument");
     F::call((unsafe { &*(opaque.cast::<T>()) }, addr, data, size))
 }
 
 impl<T> MemoryRegionOpsBuilder<T> {
     #[must_use]
-    pub const fn read<F: for<'a> FnCall<(&'a T, hwaddr, u32), u64>>(mut self, _f: &F) -> Self {
+    pub const fn read<F: for<'a> FnCall<(&'a T, hwaddr, Bits), u64>>(mut self, _f: &F) -> Self {
         self.0.read = Some(memory_region_ops_read_cb::<T, F>);
         self
     }
 
     #[must_use]
-    pub const fn write<F: for<'a> FnCall<(&'a T, hwaddr, u64, u32)>>(mut self, _f: &F) -> Self {
+    pub const fn write<F: for<'a> FnCall<(&'a T, hwaddr, u64, Bits)>>(mut self, _f: &F) -> Self {
         self.0.write = Some(memory_region_ops_write_cb::<T, F>);
         self
     }
@@ -90,9 +104,9 @@ pub const fn native_endian(mut self) -> Self {
     }
 
     #[must_use]
-    pub const fn valid_sizes(mut self, min: u32, max: u32) -> Self {
-        self.0.valid.min_access_size = min;
-        self.0.valid.max_access_size = max;
+    pub const fn valid_sizes(mut self, min: Bits, max: Bits) -> Self {
+        self.0.valid.min_access_size = min as u32;
+        self.0.valid.max_access_size = max as u32;
         self
     }
 
@@ -103,9 +117,9 @@ pub const fn valid_unaligned(mut self) -> Self {
     }
 
     #[must_use]
-    pub const fn impl_sizes(mut self, min: u32, max: u32) -> Self {
-        self.0.impl_.min_access_size = min;
-        self.0.impl_.max_access_size = max;
+    pub const fn impl_sizes(mut self, min: Bits, max: Bits) -> Self {
+        self.0.impl_.min_access_size = min as u32;
+        self.0.impl_.max_access_size = max as u32;
         self
     }
 

-- 
2.47.2

Re: [PATCH 1/3] rust/memory: replace size arg with Bits enum

[Zhao Liu]

On Thu, Jul 03, 2025 at 04:58:11PM +0300, Manos Pitsidianakis wrote:
> Date: Thu, 03 Jul 2025 16:58:11 +0300
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Subject: [PATCH 1/3] rust/memory: replace size arg with Bits enum
> X-Mailer: b4 0.14.2
> 
> We have the ability to make memory accesses use a typesafe access width
> type in Rust, which the C API currently lacks as it does not use a
> newtype wrapper for specifying the amount of bytes a memory access has;
> it uses a plain 32-bit integer value instead.
> 
> Replace use of u32 size arguments with a Bits enum that has only the
> allowed byte sizes as variants and has a u32 representation so that it
> can be fed back into C as well.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  rust/hw/char/pl011/src/device.rs |  8 ++++----
>  rust/hw/timer/hpet/src/device.rs | 14 +++++++-------
>  rust/qemu-api/src/memory.rs      | 34 ++++++++++++++++++++++++----------
>  3 files changed, 35 insertions(+), 21 deletions(-)

LGTM,

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

Re: [PATCH 1/3] rust/memory: replace size arg with Bits enum

[Paolo Bonzini]

> We have the ability to make memory accesses use a typesafe access width
> type in Rust, which the C API currently lacks as it does not use a
> newtype wrapper for specifying the amount of bytes a memory access has;
> it uses a plain 32-bit integer value instead.

I find this both verbose and (ok, that's subjective) ugly due to the 
extra import, the underscore.

There are two parts on the patches:

1) the extra checking on impl_sizes and valid_sizes.  That's valuable, 
what about just adding something like this:

         assert!(min == 1 || min == 2 || min == 4 || min == 8);
         assert!(max == 1 || max == 2 || max == 4 || max == 8);
         assert!(max >= min);

It can be validated at compile time anyway, since the functions are 
pretty much always used in const context (in fact, for C code there's a 
scripts/checkpatch.pl check that they are declared as const).


2) Passing Bits to the read and write callbacks.  The argument is 
ignored for pl011, and converted with "as u32" for HPET.  I find this to 
be worse than before, because it's very unobvious that _32 is defined to 
4 rather than 32.

The main effect on generated code is to add an assert! to 
memory_region_ops_read_cb() and memory_region_ops_write_cb() that's 
similar to the above.  I'm not sure of its value, either: if the size is 
not 1/2/4/8, memory.c/physmem.c must have screwed up big.  It's not a 
safety concern, either, since the size is not used in any unsafe code.

Thanks,

Paolo

Re: [PATCH 1/3] rust/memory: replace size arg with Bits enum

[Manos Pitsidianakis]

On Tue, Jul 8, 2025 at 11:35 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> > We have the ability to make memory accesses use a typesafe access width
> > type in Rust, which the C API currently lacks as it does not use a
> > newtype wrapper for specifying the amount of bytes a memory access has;
> > it uses a plain 32-bit integer value instead.
>
> I find this both verbose and (ok, that's subjective) ugly due to the
> extra import, the underscore.

Yep, I agree on that, but I wasn't sure what name would be better.
Whatever type-level improvement this patch brings, it's small, though
nice-to-have. We can drop it, no problem.

>
> There are two parts on the patches:
>
> 1) the extra checking on impl_sizes and valid_sizes.  That's valuable,
> what about just adding something like this:
>
>          assert!(min == 1 || min == 2 || min == 4 || min == 8);
>          assert!(max == 1 || max == 2 || max == 4 || max == 8);
>          assert!(max >= min);
>
> It can be validated at compile time anyway, since the functions are
> pretty much always used in const context (in fact, for C code there's a
> scripts/checkpatch.pl check that they are declared as const).

Yes sounds good!

>
>
> 2) Passing Bits to the read and write callbacks.  The argument is
> ignored for pl011, and converted with "as u32" for HPET.  I find this to
> be worse than before, because it's very unobvious that _32 is defined to
> 4 rather than 32.

Maybe do a `Size` enum instead that has variants: 1 instead of 8, 2
instead of 16, 4 instead of 32, etc.?

>
> The main effect on generated code is to add an assert! to
> memory_region_ops_read_cb() and memory_region_ops_write_cb() that's
> similar to the above.  I'm not sure of its value, either: if the size is
> not 1/2/4/8, memory.c/physmem.c must have screwed up big.  It's not a
> safety concern, either, since the size is not used in any unsafe code.

Yep it's more of a guard rail since we can't have refined integer types.

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

Re: [PATCH 1/3] rust/memory: replace size arg with Bits enum

[Paolo Bonzini]

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

Il mar 8 lug 2025, 11:11 Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
ha scritto:

> > I find this both verbose and (ok, that's subjective) ugly due to the
> > extra import, the underscore.
>
> Yep, I agree on that, but I wasn't sure what name would be better.
> Whatever type-level improvement this patch brings, it's small, though
> nice-to-have. We can drop it, no problem.
>

You did identify a small but real problem, which can be fixed with
assertions.

And also another issue in the API, which is that some code likes to use bit
sizes and other likes to use byte sizes. And it's not clear whether you
have to do *8 or /8.

This is the one that is where an enum does work great, but without the "as"
(because it reintroduces the *8 or /8 problem) and with names that don't
have digits (because of the underscore, but also because it makes the
result of "as" confusing).

Byte/Word/Long/Quad would work for me, as it matches the C stb/stw/stl/stq
APIs. Also please add the enum to the prelude. Still verbose but readable.
I would also add methods like nbits() and nbytes() that can be used instead
of "as".

Of course if you go with the enum there is no need for assertions in the
memory API. Except for the >= one, so perhaps it would also make sense to
implement Cmp in addition to Eq?

> The main effect on generated code is to add an assert! to
> > memory_region_ops_read_cb() and memory_region_ops_write_cb() that's
> > similar to the above.  I'm not sure of its value, either: if the size is
> > not 1/2/4/8, memory.c/physmem.c must have screwed up big.  It's not a
> > safety concern, either, since the size is not used in any unsafe code.
>
> Yep it's more of a guard rail since we can't have refined integer types.
>

It's not a lot in the grand scheme of things, but adding a match around all
reads and writes isn't great. :(

The only way to make an enum from an integer of the same repr is via
transmute, right? So probably there could be an unsafe method
Size::from_unchecked(u32) too?

Paolo


> --
> Manos Pitsidianakis
> Emulation and Virtualization Engineer at Linaro Ltd
>
>

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

Re: [PATCH 1/3] rust/memory: replace size arg with Bits enum

[Zhao Liu]

On Thu, Jul 03, 2025 at 04:58:11PM +0300, Manos Pitsidianakis wrote:
> Date: Thu, 03 Jul 2025 16:58:11 +0300
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Subject: [PATCH 1/3] rust/memory: replace size arg with Bits enum
> X-Mailer: b4 0.14.2
> 
> We have the ability to make memory accesses use a typesafe access width
> type in Rust, which the C API currently lacks as it does not use a
> newtype wrapper for specifying the amount of bytes a memory access has;
> it uses a plain 32-bit integer value instead.
> 
> Replace use of u32 size arguments with a Bits enum that has only the
> allowed byte sizes as variants and has a u32 representation so that it
> can be fed back into C as well.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  rust/hw/char/pl011/src/device.rs |  8 ++++----
>  rust/hw/timer/hpet/src/device.rs | 14 +++++++-------
>  rust/qemu-api/src/memory.rs      | 34 ++++++++++++++++++++++++----------
>  3 files changed, 35 insertions(+), 21 deletions(-)

LGTM,

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

[PATCH 2/3] memattrs.h: make MemTxAttrs into enum

[Manos Pitsidianakis]

Convert MemTxResult defines into an enum. This will allow bindgen to
generate a bitflag using the enum variants as its domain of values.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 include/exec/memattrs.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 8db1d3046479c21a32928086ea8804a66a5d5ffa..dc99b456edb0d99eced6f6513c4e1c8013e84c3e 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -81,10 +81,11 @@ QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) > 8);
  * of some kind. The memory subsystem will bitwise-OR together results
  * if it is synthesizing an operation from multiple smaller accesses.
  */
-#define MEMTX_OK 0
-#define MEMTX_ERROR             (1U << 0) /* device returned an error */
-#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
-#define MEMTX_ACCESS_ERROR      (1U << 2) /* access denied */
-typedef uint32_t MemTxResult;
+typedef enum MemTxResult {
+    MEMTX_OK           = 0,
+    MEMTX_ERROR        = (1U << 0), /* device returned an error */
+    MEMTX_DECODE_ERROR = (1U << 1), /* nothing at that address */
+    MEMTX_ACCESS_ERROR = (1U << 2), /* access denied */
+} MemTxResult;
 
 #endif

-- 
2.47.2

Re: [PATCH 2/3] memattrs.h: make MemTxAttrs into enum

[Philippe Mathieu-Daudé]

On 3/7/25 15:58, Manos Pitsidianakis wrote:
> Convert MemTxResult defines into an enum. This will allow bindgen to
> generate a bitflag using the enum variants as its domain of values.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   include/exec/memattrs.h | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/3] memattrs.h: make MemTxAttrs into enum

[Zhao Liu]

On Thu, Jul 03, 2025 at 04:58:12PM +0300, Manos Pitsidianakis wrote:
> Date: Thu, 03 Jul 2025 16:58:12 +0300
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Subject: [PATCH 2/3] memattrs.h: make MemTxAttrs into enum
> X-Mailer: b4 0.14.2
> 
> Convert MemTxResult defines into an enum. This will allow bindgen to
> generate a bitflag using the enum variants as its domain of values.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  include/exec/memattrs.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)>

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

Re: [PATCH 2/3] memattrs.h: make MemTxAttrs into enum

[Zhao Liu]

On Thu, Jul 03, 2025 at 04:58:12PM +0300, Manos Pitsidianakis wrote:
> Date: Thu, 03 Jul 2025 16:58:12 +0300
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Subject: [PATCH 2/3] memattrs.h: make MemTxAttrs into enum
> X-Mailer: b4 0.14.2
> 
> Convert MemTxResult defines into an enum. This will allow bindgen to
> generate a bitflag using the enum variants as its domain of values.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  include/exec/memattrs.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

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

[PATCH 3/3] rust: add *_with_attrs methods to MemoryRegionOps

[Manos Pitsidianakis]

MemoryRegionOps (and its builder type) only support read/write
callbacks.

Add the ability to define {read,write}_with_attrs callbacks for devices
that need them.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 rust/qemu-api/meson.build   |  1 +
 rust/qemu-api/src/memory.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index a090297c458b1a282211b9e241c0e447dd594167..95c097ec8f8dd31a863339d58eaa8bfc4d4dea18 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -26,6 +26,7 @@ foreach enum : c_enums
 endforeach
 c_bitfields = [
   'ClockEvent',
+  'MemTxResult',
   'VMStateFlags',
 ]
 foreach enum : c_bitfields
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index b1907aa01300a3fac8e1f3b69c5d50da631a556d..98d1e116147aba2c9034a61159492382b1ee0e5e 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -9,7 +9,7 @@
     marker::PhantomData,
 };
 
-pub use bindings::{hwaddr, MemTxAttrs};
+pub use bindings::{hwaddr, MemTxAttrs, MemTxResult};
 
 use crate::{
     bindings::{self, device_endian, memory_region_init_io},
@@ -59,6 +59,20 @@ unsafe impl<T: Sync> Sync for MemoryRegionOps<T> {}
     F::call((unsafe { &*(opaque.cast::<T>()) }, addr, size))
 }
 
+unsafe extern "C" fn memory_region_ops_read_with_attrs_cb<
+    T,
+    F: for<'a> FnCall<(&'a T, hwaddr, *mut u64, Bits, MemTxAttrs), MemTxResult>,
+>(
+    opaque: *mut c_void,
+    addr: hwaddr,
+    data: *mut u64,
+    size: c_uint,
+    attrs: MemTxAttrs,
+) -> MemTxResult {
+    let size = Bits::try_from(size).expect("invalid size argument");
+    F::call((unsafe { &*(opaque.cast::<T>()) }, addr, data, size, attrs))
+}
+
 unsafe extern "C" fn memory_region_ops_write_cb<
     T,
     F: for<'a> FnCall<(&'a T, hwaddr, u64, Bits)>,
@@ -72,6 +86,20 @@ unsafe impl<T: Sync> Sync for MemoryRegionOps<T> {}
     F::call((unsafe { &*(opaque.cast::<T>()) }, addr, data, size))
 }
 
+unsafe extern "C" fn memory_region_ops_write_with_attrs_cb<
+    T,
+    F: for<'a> FnCall<(&'a T, hwaddr, u64, Bits, MemTxAttrs), MemTxResult>,
+>(
+    opaque: *mut c_void,
+    addr: hwaddr,
+    data: u64,
+    size: c_uint,
+    attrs: MemTxAttrs,
+) -> MemTxResult {
+    let size = Bits::try_from(size).expect("invalid size argument");
+    F::call((unsafe { &*(opaque.cast::<T>()) }, addr, data, size, attrs))
+}
+
 impl<T> MemoryRegionOpsBuilder<T> {
     #[must_use]
     pub const fn read<F: for<'a> FnCall<(&'a T, hwaddr, Bits), u64>>(mut self, _f: &F) -> Self {
@@ -86,6 +114,28 @@ pub const fn write<F: for<'a> FnCall<(&'a T, hwaddr, u64, Bits)>>(mut self, _f:
     }
 
     #[must_use]
+    pub const fn read_with_attrs<
+        F: for<'a> FnCall<(&'a T, hwaddr, *mut u64, Bits, MemTxAttrs), MemTxResult>,
+    >(
+        mut self,
+        _f: &F,
+    ) -> Self {
+        self.0.read_with_attrs = Some(memory_region_ops_read_with_attrs_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn write_with_attrs<
+        F: for<'a> FnCall<(&'a T, hwaddr, u64, Bits, MemTxAttrs), MemTxResult>,
+    >(
+        mut self,
+        _f: &F,
+    ) -> Self {
+        self.0.write_with_attrs = Some(memory_region_ops_write_with_attrs_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
     pub const fn big_endian(mut self) -> Self {
         self.0.endianness = device_endian::DEVICE_BIG_ENDIAN;
         self

-- 
2.47.2

Re: [PATCH 3/3] rust: add *_with_attrs methods to MemoryRegionOps

[Zhao Liu]

On Thu, Jul 03, 2025 at 04:58:13PM +0300, Manos Pitsidianakis wrote:
> Date: Thu, 03 Jul 2025 16:58:13 +0300
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Subject: [PATCH 3/3] rust: add *_with_attrs methods to MemoryRegionOps
> X-Mailer: b4 0.14.2
> 
> MemoryRegionOps (and its builder type) only support read/write
> callbacks.
> 
> Add the ability to define {read,write}_with_attrs callbacks for devices
> that need them.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  rust/qemu-api/meson.build   |  1 +
>  rust/qemu-api/src/memory.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 52 insertions(+), 1 deletion(-)

Fine for me,

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