mbox series

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

Message ID 20250320133248.1679485-1-peter.maydell@linaro.org
Headers show
Series rust: Fix PL011State size mismatch assert | expand

Message

Peter Maydell March 20, 2025, 1:32 p.m. UTC 
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(-)