| Message ID | 20250320133248.1679485-4-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Fix PL011State size mismatch assert | expand |
> -use std::{ffi::CStr, ptr::addr_of_mut}; > +use std::{ffi::CStr, mem, ptr::addr_of_mut}; maybe mem::size_of (since there're 2 use cases :-))? > > use qemu_api::{ > + bindings, > chardev::{CharBackend, Chardev, Event}, > + static_assert, This one looks like it breaks the alphabetical ordering (this nit can be checked and fixed by "cargo +nightly fmt" under rust directory, which is like checkpatch.pl). > impl_vmstate_forward, > irq::{IRQState, InterruptSource}, > memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder}, > @@ -124,6 +126,12 @@ pub struct PL011State { > pub migrate_clock: bool, > } > > +// Some C users of this device embed its state struct into their own > +// structs, so the size of the Rust version must not be any larger > +// than the size of the C one. If this assert triggers you need to > +// expand the padding_for_rust[] array in the C PL011State struct. > +static_assert!(mem::size_of::<PL011State>() <= mem::size_of::<bindings::PL011State>()); > + maybe use qemu_api::bindings::PL011State directly? Because bindings contains native C structures/functions and their use should not be encouraged, I think it's better to 'hide' bindings (not list it at the beginning of the file). Otherwise, Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
On Thu, 20 Mar 2025 at 15:25, Zhao Liu <zhao1.liu@intel.com> wrote: > > > -use std::{ffi::CStr, ptr::addr_of_mut}; > > +use std::{ffi::CStr, mem, ptr::addr_of_mut}; > > maybe mem::size_of (since there're 2 use cases :-))? > > > > > use qemu_api::{ > > + bindings, > > chardev::{CharBackend, Chardev, Event}, > > + static_assert, > > This one looks like it breaks the alphabetical ordering (this nit can be > checked and fixed by "cargo +nightly fmt" under rust directory, which is > like checkpatch.pl). Yep; I put it here because I started with Paolo's v1 patch which called the macro "const_assert", and then when v2 changed the macro name I forgot to move it later in the use {}. > > impl_vmstate_forward, > > irq::{IRQState, InterruptSource}, > > memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder}, > > @@ -124,6 +126,12 @@ pub struct PL011State { > > pub migrate_clock: bool, > > } > > > > +// Some C users of this device embed its state struct into their own > > +// structs, so the size of the Rust version must not be any larger > > +// than the size of the C one. If this assert triggers you need to > > +// expand the padding_for_rust[] array in the C PL011State struct. > > +static_assert!(mem::size_of::<PL011State>() <= mem::size_of::<bindings::PL011State>()); > > + > > maybe use qemu_api::bindings::PL011State directly? Because bindings > contains native C structures/functions and their use should not be > encouraged, I think it's better to 'hide' bindings (not list it at the > beginning of the file). Yeah, I wasn't sure what our preferred style approach is here regarding what we "use" and what we just directly reference (and the same in the other direction for mem::size_of vs size_of). Is there a "normal" pattern to follow here ? Speaking of size_of, I noticed that Rust provides both core::mem::size_of and std::mem::size_of, and in rust/ at the moment we have uses of both. What's the difference? thanks -- PMM
> > > +// Some C users of this device embed its state struct into their own > > > +// structs, so the size of the Rust version must not be any larger > > > +// than the size of the C one. If this assert triggers you need to > > > +// expand the padding_for_rust[] array in the C PL011State struct. > > > +static_assert!(mem::size_of::<PL011State>() <= mem::size_of::<bindings::PL011State>()); > > > + > > > > maybe use qemu_api::bindings::PL011State directly? Because bindings > > contains native C structures/functions and their use should not be > > encouraged, I think it's better to 'hide' bindings (not list it at the > > beginning of the file). > > Yeah, I wasn't sure what our preferred style approach is here > regarding what we "use" and what we just directly reference > (and the same in the other direction for mem::size_of vs > size_of). Is there a "normal" pattern to follow here ? There seems no clear doc on when to list use statements, but it's common to list as clearly as possible to make it easier to sort out dependencies. About bindings, I think it's better to clearly point out the specific members in bindings, so ‘use qemu_api::bindings’ looks vague. Alternatively, the qemu_api::bindings::PL011State could also be listed at the beginning of the file, similar to a previous clean up: 06a1cfb5550a ("rust/pl011: Avoid bindings::*") and another patch [1]. [1]: https://lore.kernel.org/qemu-devel/20250318130219.1799170-16-zhao1.liu@intel.com/ > Speaking of size_of, I noticed that Rust provides both > core::mem::size_of and std::mem::size_of, and in rust/ at > the moment we have uses of both. What's the difference? They're the same (a simple proof of this is that the "source" option of the std::mem page [2] points to the core::mem repo). `core` is self-contained without OS dependency, and `std` is the superset of `core` with extra OS dependency. And there's a previous cleanup to consolidate `std::ptr` (commit c48700e86d, "rust: prefer importing std::ptr over core::ptr"). So I think we can prefer std::mem as well. [2]: https://doc.rust-lang.org/std/mem/index.html Regards, Zhao
diff --git a/rust/wrapper.h b/rust/wrapper.h index d927ad6799d..d4fec546571 100644 --- a/rust/wrapper.h +++ b/rust/wrapper.h @@ -65,3 +65,4 @@ typedef enum memory_order { #include "exec/memattrs.h" #include "qemu/timer.h" #include "exec/address-spaces.h" +#include "hw/char/pl011.h" diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index f137b49feaf..f9b1c307289 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -2,10 +2,12 @@ // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> // SPDX-License-Identifier: GPL-2.0-or-later -use std::{ffi::CStr, ptr::addr_of_mut}; +use std::{ffi::CStr, mem, ptr::addr_of_mut}; use qemu_api::{ + bindings, chardev::{CharBackend, Chardev, Event}, + static_assert, impl_vmstate_forward, irq::{IRQState, InterruptSource}, memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder}, @@ -124,6 +126,12 @@ pub struct PL011State { pub migrate_clock: bool, } +// Some C users of this device embed its state struct into their own +// structs, so the size of the Rust version must not be any larger +// than the size of the C one. If this assert triggers you need to +// expand the padding_for_rust[] array in the C PL011State struct. +static_assert!(mem::size_of::<PL011State>() <= mem::size_of::<bindings::PL011State>()); + qom_isa!(PL011State : SysBusDevice, DeviceState, Object); #[repr(C)]
The PL011 device's C implementation exposes its PL011State struct to users of the device, and one common usage pattern is to embed that struct into the user's own state struct. (The internals of the struct are technically visible to the C user of the device, but in practice are treated as implementation details.) This means that the Rust version of the state struct must not be larger than the C version's struct; otherwise it will trip a runtime assertion in object_initialize_type() when the C user attempts to in-place initialize the type. Add a compile-time assertion on the Rust side, so that if we accidentally make the Rust device state larger we know immediately that we need to expand the padding in the C version of the struct. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- rust/wrapper.h | 1 + rust/hw/char/pl011/src/device.rs | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-)