[PATCH 0/3] rust: Fix PL011State size mismatch assert

[PATCH 0/3] rust: Fix PL011State size mismatch assert

[Peter Maydell]

We have some users of the PL011 struct which embed it directly into
their own state structs. This means that the Rust version of the
device must have a state struct that is the same size or smaller
than the C struct.

In commit 9b642097d6b7 ("rust: pl011: switch to safe chardev operation")
the Rust PL011 state struct changed from having a bindings::CharBackend
to a chardev::CharBackend, which made it grow larger than the C
version. This results in an assertion at startup when QEMU was
built with Rust enabled:

 $ qemu-system-arm -M raspi2b -display none
 ERROR:../../qom/object.c:562:object_initialize_with_type: assertion
 failed: (size >= type->instance_size)

This series fixes that by the simple expedient of adding
a padding field to the end of the C struct to ensure that
it's big enough to also fit the Rust version of the device.

It also moves the failure from runtime to compiletime,
by adding a Rust compile-time assert that it hasn't made
the state bigger than the C one, so if we do this again
it should be caught before it gets into git. (We don't
need to do the same thing for the HPET device, because there
the HPETState is a private implementation detail of the C
code, not exposed to its users.)

NB: if the Rust version also needed stricter alignment than the
C struct that would also be bad; I don't attempt to assert on
that here, assuming that it's unlikely that we'll be trying for
anything more aligned than the usual pointer-alignment.

Patch 1 is Paolo's static_assert macro that he sent out earlier today.

Having the C struct visible to its users like this is not ideal
in the longer term; we have had discussions before about shifting
back to a "users only get an opaque pointer" design style,
for instance for the benefit of the "create custom machines on
the commandline" effort. I think this Rust/C issue is further
weight towards moving that way. But that would be quite a
lot of reworking of existing C code.

Exposing the C struct to users of the device also means that
they have direct access to all its fields, which obviously
will go badly wrong if they try to touch them when the Rust
version of the device is being used. Those fields are supposed
to be private, but this is based purely on the honour system,
and we do actually have a few places in the code that take
shortcuts and directly access a few fields (not for the PL011).
I had a proposal a decade ago:
https://lore.kernel.org/qemu-devel/1399650964-21067-1-git-send-email-peter.maydell@linaro.org/
for using macros and the compiler 'deprecated' attribute to
generate compiler warnings/errors for accesses to struct fields
outside the file implementing the device itself. 
We could perhaps resurrect that idea as a mechanism for
detecting places we would need to clean up before conversions
of future C devices to Rust.

thanks
-- PMM

Paolo Bonzini (1):
  rust: assertions: add static_assert

Peter Maydell (2):
  hw/char/pl011: Pad PL011State struct to same size as Rust impl
  rust: pl011: Check size of state struct at compile time

 include/hw/char/pl011.h          |  5 +++++
 rust/wrapper.h                   |  1 +
 rust/hw/char/pl011/src/device.rs | 10 +++++++++-
 rust/qemu-api/src/assertions.rs  | 22 ++++++++++++++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/3] rust: assertions: add static_assert

[Peter Maydell]

From: Paolo Bonzini <pbonzini@redhat.com>

Add a new assertion that is similar to "const { assert!(...) }" but can be used
outside functions and with older versions of Rust.  A similar macro is found in
Linux, whereas the "static_assertions" crate has a const_assert macro that
produces worse error messages.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Supersedes: <20250320113356.799412-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 rust/qemu-api/src/assertions.rs | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs
index 104dec39774..bba38cfda11 100644
--- a/rust/qemu-api/src/assertions.rs
+++ b/rust/qemu-api/src/assertions.rs
@@ -120,3 +120,25 @@ macro_rules! assert_match {
         );
     };
 }
+
+/// Assert at compile time that an expression is true.  This is similar
+/// to `const { assert!(...); }` but it works outside functions, as well as
+/// on versions of Rust before 1.79.
+///
+/// # Examples
+///
+/// ```
+/// # use qemu_api::static_assert;
+/// static_assert!("abc".len() == 3);
+/// ```
+///
+/// ```compile_fail
+/// # use qemu_api::static_assert;
+/// static_assert!("abc".len() == 2); // does not compile
+/// ```
+#[macro_export]
+macro_rules! static_assert {
+    ($x:expr) => {
+        const _: () = assert!($x);
+    };
+}
-- 
2.43.0

[PATCH 2/3] hw/char/pl011: Pad PL011State struct to same size as Rust impl

[Peter Maydell]

We have some users of the PL011 struct which embed it directly into
their own state structs. This means that the Rust version of the
device must have a state struct that is the same size or smaller
than the C struct.

In commit 9b642097d6b7 ("rust: pl011: switch to safe chardev operation")
the Rust PL011 state struct changed from having a bindings::CharBackend
to a chardev::CharBackend, which made it grow larger than the C
version. This results in an assertion at startup when QEMU was
built with Rust enabled:

 $ qemu-system-arm -M raspi2b -display none
 ERROR:../../qom/object.c:562:object_initialize_with_type: assertion
 failed: (size >= type->instance_size)

The long-term better approach to this problem would be to move
our C device code patterns away from "embed a struct" and (back)
to "have a pointer to the device", so we can make the C PL011State
struct a private implementation detail rather than exposed to
its users.

For the short term, add a padding field at the end of the C struct
so it's big enough that the Rust state struct can fit.

Fixes: 9b642097d6b7 ("rust: pl011: switch to safe chardev operation")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/char/pl011.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
index 4fcaf3d7d30..299ca9b18bb 100644
--- a/include/hw/char/pl011.h
+++ b/include/hw/char/pl011.h
@@ -52,6 +52,11 @@ struct PL011State {
     Clock *clk;
     bool migrate_clk;
     const unsigned char *id;
+    /*
+     * Since some users embed this struct directly, we must
+     * ensure that the C struct is at least as big as the Rust one.
+     */
+    uint8_t padding_for_rust[16];
 };
 
 DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr);
-- 
2.43.0

[PATCH 3/3] rust: pl011: Check size of state struct at compile time

[Peter Maydell]

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

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)]
-- 
2.43.0

Re: [PATCH 1/3] rust: assertions: add static_assert

[Philippe Mathieu-Daudé]

On 20/3/25 14:32, Peter Maydell wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Add a new assertion that is similar to "const { assert!(...) }" but can be used
> outside functions and with older versions of Rust.  A similar macro is found in
> Linux, whereas the "static_assertions" crate has a const_assert macro that
> produces worse error messages.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Supersedes: <20250320113356.799412-1-pbonzini@redhat.com>

^ extraneous tag

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   rust/qemu-api/src/assertions.rs | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)

Re: [PATCH 1/3] rust: assertions: add static_assert

[Zhao Liu]

On Thu, Mar 20, 2025 at 01:32:46PM +0000, Peter Maydell wrote:
> Date: Thu, 20 Mar 2025 13:32:46 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 1/3] rust: assertions: add static_assert
> X-Mailer: git-send-email 2.43.0
> 
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Add a new assertion that is similar to "const { assert!(...) }" but can be used
> outside functions and with older versions of Rust.  A similar macro is found in
> Linux, whereas the "static_assertions" crate has a const_assert macro that
> produces worse error messages.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Supersedes: <20250320113356.799412-1-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  rust/qemu-api/src/assertions.rs | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

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

Re: [PATCH 2/3] hw/char/pl011: Pad PL011State struct to same size as Rust impl

[Zhao Liu]

On Thu, Mar 20, 2025 at 01:32:47PM +0000, Peter Maydell wrote:
> Date: Thu, 20 Mar 2025 13:32:47 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 2/3] hw/char/pl011: Pad PL011State struct to same size as
>  Rust impl
> X-Mailer: git-send-email 2.43.0
> 
> We have some users of the PL011 struct which embed it directly into
> their own state structs. This means that the Rust version of the
> device must have a state struct that is the same size or smaller
> than the C struct.
> 
> In commit 9b642097d6b7 ("rust: pl011: switch to safe chardev operation")
> the Rust PL011 state struct changed from having a bindings::CharBackend
> to a chardev::CharBackend, which made it grow larger than the C
> version. This results in an assertion at startup when QEMU was
> built with Rust enabled:
> 
>  $ qemu-system-arm -M raspi2b -display none
>  ERROR:../../qom/object.c:562:object_initialize_with_type: assertion
>  failed: (size >= type->instance_size)
> 
> The long-term better approach to this problem would be to move
> our C device code patterns away from "embed a struct" and (back)
> to "have a pointer to the device", so we can make the C PL011State
> struct a private implementation detail rather than exposed to
> its users.
> 
> For the short term, add a padding field at the end of the C struct
> so it's big enough that the Rust state struct can fit.
> 
> Fixes: 9b642097d6b7 ("rust: pl011: switch to safe chardev operation")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/char/pl011.h | 5 +++++
>  1 file changed, 5 insertions(+)

LGTM, BqlRefCell<> has extra fields to make BqlRefCell<T> bigger than T,

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

Re: [PATCH 3/3] rust: pl011: Check size of state struct at compile time

[Zhao Liu]

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

Re: [PATCH 3/3] rust: pl011: Check size of state struct at compile time

[Peter Maydell]

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

Re: [PATCH 3/3] rust: pl011: Check size of state struct at compile time

[Zhao Liu]

> > > +// 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