The Rust vmstate macros lack the type-safety of their C equivalents (so
safe, much abstraction), and therefore they were predictably wrong.
The registers have already been changed to 32-bits in the previous patch,
but read_pos/read_count/read_trigger also have to be u32 instead of usize.
The easiest way to do so is to let the FIFO use u32 indices instead
of usize.
My plan for making VMStateField typesafe is to have a trait to retrieve
a basic VMStateField; for example something like vmstate_uint32 would
become an implementation of the VMState trait on u32. Then you'd write
something like "vmstate_of!(Type, field).with_version_id(2)". That is,
vmstate_of retrieves the basic VMStateField and fills in the offset,
and then more changes can be applied on top.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 38 ++++++++++++++++++++++----
rust/hw/char/pl011/src/device_class.rs | 8 +++---
rust/qemu-api/src/vmstate.rs | 22 ---------------
3 files changed, 36 insertions(+), 32 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 332e0a31a82..cfe2734703e 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -31,7 +31,7 @@
const FBRD_MASK: u32 = 0x3f;
/// QEMU sourced constant.
-pub const PL011_FIFO_DEPTH: usize = 16_usize;
+pub const PL011_FIFO_DEPTH: u32 = 16;
#[derive(Clone, Copy)]
struct DeviceId(&'static [u8; 8]);
@@ -49,6 +49,32 @@ impl DeviceId {
const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
}
+// FIFOs use 32-bit indices instead of usize, for compatibility with
+// the migration stream produced by the C version of this device.
+#[repr(transparent)]
+#[derive(Debug, Default)]
+pub struct Fifo([registers::Data; PL011_FIFO_DEPTH as usize]);
+
+impl Fifo {
+ const fn len(&self) -> u32 {
+ self.0.len() as u32
+ }
+}
+
+impl std::ops::IndexMut<u32> for Fifo {
+ fn index_mut(&mut self, idx: u32) -> &mut Self::Output {
+ &mut self.0[idx as usize]
+ }
+}
+
+impl std::ops::Index<u32> for Fifo {
+ type Output = registers::Data;
+
+ fn index(&self, idx: u32) -> &Self::Output {
+ &self.0[idx as usize]
+ }
+}
+
#[repr(C)]
#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
/// PL011 Device Model in QEMU
@@ -66,14 +92,14 @@ pub struct PL011State {
pub dmacr: u32,
pub int_enabled: u32,
pub int_level: u32,
- pub read_fifo: [registers::Data; PL011_FIFO_DEPTH],
+ pub read_fifo: Fifo,
pub ilpr: u32,
pub ibrd: u32,
pub fbrd: u32,
pub ifl: u32,
- pub read_pos: usize,
- pub read_count: usize,
- pub read_trigger: usize,
+ pub read_pos: u32,
+ pub read_count: u32,
+ pub read_trigger: u32,
#[doc(alias = "chr")]
pub char_backend: CharBackend,
/// QEMU interrupts
@@ -485,7 +511,7 @@ pub fn loopback_enabled(&self) -> bool {
}
#[inline]
- pub fn fifo_depth(&self) -> usize {
+ pub fn fifo_depth(&self) -> u32 {
// Note: FIFO depth is expected to be power-of-2
if self.fifo_enabled() {
return PL011_FIFO_DEPTH;
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 975c3d42be7..759c521a99e 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -6,7 +6,7 @@
use std::os::raw::{c_int, c_void};
use qemu_api::{
- bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections,
+ bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_subsections,
vmstate_uint32, vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
};
@@ -64,9 +64,9 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
vmstate_uint32!(ibrd, PL011State),
vmstate_uint32!(fbrd, PL011State),
vmstate_uint32!(ifl, PL011State),
- vmstate_int32!(read_pos, PL011State),
- vmstate_int32!(read_count, PL011State),
- vmstate_int32!(read_trigger, PL011State),
+ vmstate_uint32!(read_pos, PL011State),
+ vmstate_uint32!(read_count, PL011State),
+ vmstate_uint32!(read_trigger, PL011State),
},
subsections: vmstate_subsections! {
VMSTATE_PL011_CLOCK
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 25c68b703ea..63c897abcdf 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -106,28 +106,6 @@ macro_rules! vmstate_uint32 {
}};
}
-#[doc(alias = "VMSTATE_INT32_V")]
-#[macro_export]
-macro_rules! vmstate_int32_v {
- ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
- $crate::vmstate_single!(
- $field_name,
- $struct_name,
- $version_id,
- ::core::ptr::addr_of!($crate::bindings::vmstate_info_int32),
- ::core::mem::size_of::<i32>()
- )
- }};
-}
-
-#[doc(alias = "VMSTATE_INT32")]
-#[macro_export]
-macro_rules! vmstate_int32 {
- ($field_name:ident, $struct_name:ty) => {{
- $crate::vmstate_int32_v!($field_name, $struct_name, 0)
- }};
-}
-
#[doc(alias = "VMSTATE_ARRAY")]
#[macro_export]
macro_rules! vmstate_array {
--
2.47.1On Thu, Dec 12, 2024 at 06:22:03PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:22:03 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 6/7] rust: pl011: fix migration stream
> X-Mailer: git-send-email 2.47.1
>
> The Rust vmstate macros lack the type-safety of their C equivalents (so
> safe, much abstraction), and therefore they were predictably wrong.
Yes, this makes Rust more unsafe than C code...
> The registers have already been changed to 32-bits in the previous patch,
> but read_pos/read_count/read_trigger also have to be u32 instead of usize.
> The easiest way to do so is to let the FIFO use u32 indices instead
> of usize.
>
> My plan for making VMStateField typesafe is to have a trait to retrieve
> a basic VMStateField; for example something like vmstate_uint32 would
> become an implementation of the VMState trait on u32. Then you'd write
> something like "vmstate_of!(Type, field).with_version_id(2)". That is,
> vmstate_of retrieves the basic VMStateField and fills in the offset,
> and then more changes can be applied on top.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 38 ++++++++++++++++++++++----
> rust/hw/char/pl011/src/device_class.rs | 8 +++---
> rust/qemu-api/src/vmstate.rs | 22 ---------------
> 3 files changed, 36 insertions(+), 32 deletions(-)
>
> @@ -64,9 +64,9 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
> vmstate_uint32!(ibrd, PL011State),
> vmstate_uint32!(fbrd, PL011State),
> vmstate_uint32!(ifl, PL011State),
> - vmstate_int32!(read_pos, PL011State),
> - vmstate_int32!(read_count, PL011State),
> - vmstate_int32!(read_trigger, PL011State),
> + vmstate_uint32!(read_pos, PL011State),
> + vmstate_uint32!(read_count, PL011State),
> + vmstate_uint32!(read_trigger, PL011State),
uint32 and int32 types both use `qemu_put_be32s` and `qemu_get_be32s`
to save and store vmstate, so I think it's safe to convert
vmstate_int32! to vmstate_uint32! here.
> },
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
LGTM, still happily deferring to Alex =)
On 12/12/24 18:22, Paolo Bonzini wrote:
> The Rust vmstate macros lack the type-safety of their C equivalents (so
> safe, much abstraction), and therefore they were predictably wrong.
>
> The registers have already been changed to 32-bits in the previous patch,
> but read_pos/read_count/read_trigger also have to be u32 instead of usize.
> The easiest way to do so is to let the FIFO use u32 indices instead
> of usize.
>
> My plan for making VMStateField typesafe is to have a trait to retrieve
> a basic VMStateField; for example something like vmstate_uint32 would
> become an implementation of the VMState trait on u32. Then you'd write
> something like "vmstate_of!(Type, field).with_version_id(2)". That is,
> vmstate_of retrieves the basic VMStateField and fills in the offset,
> and then more changes can be applied on top.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 38 ++++++++++++++++++++++----
> rust/hw/char/pl011/src/device_class.rs | 8 +++---
> rust/qemu-api/src/vmstate.rs | 22 ---------------
> 3 files changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 332e0a31a82..cfe2734703e 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -31,7 +31,7 @@
> const FBRD_MASK: u32 = 0x3f;
>
> /// QEMU sourced constant.
> -pub const PL011_FIFO_DEPTH: usize = 16_usize;
> +pub const PL011_FIFO_DEPTH: u32 = 16;
>
> #[derive(Clone, Copy)]
> struct DeviceId(&'static [u8; 8]);
> @@ -49,6 +49,32 @@ impl DeviceId {
> const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
> }
>
> +// FIFOs use 32-bit indices instead of usize, for compatibility with
> +// the migration stream produced by the C version of this device.
> +#[repr(transparent)]
> +#[derive(Debug, Default)]
> +pub struct Fifo([registers::Data; PL011_FIFO_DEPTH as usize]);
> +
> +impl Fifo {
> + const fn len(&self) -> u32 {
> + self.0.len() as u32
> + }
> +}
> +
> +impl std::ops::IndexMut<u32> for Fifo {
> + fn index_mut(&mut self, idx: u32) -> &mut Self::Output {
> + &mut self.0[idx as usize]
> + }
> +}
> +
> +impl std::ops::Index<u32> for Fifo {
> + type Output = registers::Data;
> +
> + fn index(&self, idx: u32) -> &Self::Output {
> + &self.0[idx as usize]
> + }
> +}
> +
> #[repr(C)]
> #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
> /// PL011 Device Model in QEMU
> @@ -66,14 +92,14 @@ pub struct PL011State {
> pub dmacr: u32,
> pub int_enabled: u32,
> pub int_level: u32,
> - pub read_fifo: [registers::Data; PL011_FIFO_DEPTH],
> + pub read_fifo: Fifo,
> pub ilpr: u32,
> pub ibrd: u32,
> pub fbrd: u32,
> pub ifl: u32,
> - pub read_pos: usize,
> - pub read_count: usize,
> - pub read_trigger: usize,
> + pub read_pos: u32,
> + pub read_count: u32,
> + pub read_trigger: u32,
> #[doc(alias = "chr")]
> pub char_backend: CharBackend,
> /// QEMU interrupts
> @@ -485,7 +511,7 @@ pub fn loopback_enabled(&self) -> bool {
> }
>
> #[inline]
> - pub fn fifo_depth(&self) -> usize {
> + pub fn fifo_depth(&self) -> u32 {
> // Note: FIFO depth is expected to be power-of-2
> if self.fifo_enabled() {
> return PL011_FIFO_DEPTH;
> diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
> index 975c3d42be7..759c521a99e 100644
> --- a/rust/hw/char/pl011/src/device_class.rs
> +++ b/rust/hw/char/pl011/src/device_class.rs
> @@ -6,7 +6,7 @@
> use std::os::raw::{c_int, c_void};
>
> use qemu_api::{
> - bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections,
> + bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_subsections,
> vmstate_uint32, vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
> };
>
> @@ -64,9 +64,9 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
> vmstate_uint32!(ibrd, PL011State),
> vmstate_uint32!(fbrd, PL011State),
> vmstate_uint32!(ifl, PL011State),
> - vmstate_int32!(read_pos, PL011State),
> - vmstate_int32!(read_count, PL011State),
> - vmstate_int32!(read_trigger, PL011State),
> + vmstate_uint32!(read_pos, PL011State),
> + vmstate_uint32!(read_count, PL011State),
> + vmstate_uint32!(read_trigger, PL011State),
> },
> subsections: vmstate_subsections! {
> VMSTATE_PL011_CLOCK
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index 25c68b703ea..63c897abcdf 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -106,28 +106,6 @@ macro_rules! vmstate_uint32 {
> }};
> }
>
> -#[doc(alias = "VMSTATE_INT32_V")]
> -#[macro_export]
> -macro_rules! vmstate_int32_v {
> - ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
> - $crate::vmstate_single!(
> - $field_name,
> - $struct_name,
> - $version_id,
> - ::core::ptr::addr_of!($crate::bindings::vmstate_info_int32),
> - ::core::mem::size_of::<i32>()
> - )
> - }};
> -}
> -
> -#[doc(alias = "VMSTATE_INT32")]
> -#[macro_export]
> -macro_rules! vmstate_int32 {
> - ($field_name:ident, $struct_name:ty) => {{
> - $crate::vmstate_int32_v!($field_name, $struct_name, 0)
> - }};
> -}
> -
> #[doc(alias = "VMSTATE_ARRAY")]
> #[macro_export]
> macro_rules! vmstate_array {
© 2016 - 2025 Red Hat, Inc.