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.2On 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>
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>
> 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
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
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 > >
© 2016 - 2025 Red Hat, Inc.