| Message ID | 20250309103120.1116448-20-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/25] chardev: express dependency on io/ | expand |
On Sun, 9 Mar 2025 at 10:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Switch bindings::CharBackend with chardev::CharBackend. This removes > occurrences of "unsafe" due to FFI and switches the wrappers for receive, > can_receive and event callbacks to the common ones implemented by > chardev::CharBackend. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Hi -- this commit seems to have broken use of the PL011 in boards/SoCs that directly embed it in their state structs, so "qemu-system-arm -M raspi2b -display none" now asserts on startup. The Rust PL011's state struct size is now larger than the C state struct size, so it trips the assert in the QOM code that we didn't try to initialize a type into less memory than it needs. (I don't understand *why* the type size has changed, because this commit doesn't at first glance seem to be adding anything to the state struct...but it definitely goes up from 0x540 to 0x550.) (It would be good if we had a compile time check that the state struct sizes matched between Rust and C, rather than having it only be caught in runtime asserts. This does cause failures in check-functional, at least, so it's not completely untested.) Here's the repro and gdb backtrace: $ gdb --args ./build/rust/qemu-system-arm -M raspi2b -display none [...] ** ERROR:../../qom/object.c:562:object_initialize_with_type: assertion failed: (size >= type->instance_size) Bail out! ERROR:../../qom/object.c:562:object_initialize_with_type: assertion failed: (size >= type->instance_size) Thread 1 "qemu-system-arm" received signal SIGABRT, Aborted. Download failed: Invalid argument. Continuing without source file ./nptl/./nptl/pthread_kill.c. __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimised out>) at ./nptl/pthread_kill.c:44 warning: 44 ./nptl/pthread_kill.c: No such file or directory (gdb) bt #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimised out>) at ./nptl/pthread_kill.c:44 #1 __pthread_kill_internal (signo=6, threadid=<optimised out>) at ./nptl/pthread_kill.c:78 #2 __GI___pthread_kill (threadid=<optimised out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89 #3 0x00007ffff4a4527e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #4 0x00007ffff4a288ff in __GI_abort () at ./stdlib/abort.c:79 #5 0x00007ffff6e58f5b in g_assertion_message (domain=domain@entry=0x0, file=file@entry=0x55555678fdeb "../../qom/object.c", line=line@entry=562, func=func@entry=0x5555567906d0 <__func__.33> "object_initialize_with_type", message=message@entry=0x555557f7f400 "assertion failed: (size >= type->instance_size)") at ../../../glib/gtestutils.c:3331 #6 0x00007ffff6ec1a97 in g_assertion_message_expr (domain=0x0, file=0x55555678fdeb "../../qom/object.c", line=562, func=0x5555567906d0 <__func__.33> "object_initialize_with_type", expr=<optimised out>) at ../../../glib/gtestutils.c:3357 #7 0x0000555556188bc6 in object_initialize_with_type (obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at ../../qom/object.c:562 #8 0x0000555556188cb5 in object_initialize (data=0x555557d4e190, size=1344, typename=0x5555566d9142 "pl011") at ../../qom/object.c:578 #9 0x0000555556188e3d in object_initialize_child_with_propsv (parentobj=0x555557d45710, propname=0x5555566d9148 "uart0", childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011", errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd360) at ../../qom/object.c:608 #10 0x0000555556188db6 in object_initialize_child_with_props (parentobj=0x555557d45710, propname=0x5555566d9148 "uart0", childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011", errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591 #11 0x0000555556188f3b in object_initialize_child_internal (parent=0x555557d45710, propname=0x5555566d9148 "uart0", child=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011") at ../../qom/object.c:645 #12 0x0000555555d446ea in raspi_peripherals_base_init (obj=0x555557d45710) at ../../hw/arm/bcm2835_peripherals.c:100 #13 0x0000555556188639 in object_init_with_type (obj=0x555557d45710, ti=0x5555579d4af0) at ../../qom/object.c:428 #14 0x000055555618861b in object_init_with_type (obj=0x555557d45710, ti=0x5555579d4950) at ../../qom/object.c:424 #15 0x0000555556188c49 in object_initialize_with_type (obj=0x555557d45710, size=597040, type=0x5555579d4950) at ../../qom/object.c:570 #16 0x0000555556188cb5 in object_initialize (data=0x555557d45710, size=597040, typename=0x555556738ca5 "bcm2835-peripherals") at ../../qom/object.c:578 #17 0x0000555556188e3d in object_initialize_child_with_propsv (parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals", childobj=0x555557d45710, size=597040, type=0x555556738ca5 "bcm2835-peripherals", errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd630) at ../../qom/object.c:608 #18 0x0000555556188db6 in object_initialize_child_with_props (parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals", childobj=0x555557d45710, size=597040, type=0x555556738ca5 "bcm2835-peripherals", errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591 #19 0x0000555556188f3b in object_initialize_child_internal (parent=0x555557d34760, propname=0x555556738cb9 "peripherals", child=0x555557d45710, size=597040, type=0x555556738ca5 "bcm2835-peripherals") at ../../qom/object.c:645 #20 0x0000555555f07080 in bcm283x_init (obj=0x555557d34760) at ../../hw/arm/bcm2836.c:49 #21 0x0000555556188639 in object_init_with_type (obj=0x555557d34760, ti=0x5555579af8a0) at ../../qom/object.c:428 #22 0x000055555618861b in object_init_with_type (obj=0x555557d34760, ti=0x5555579af6e0) at ../../qom/object.c:424 #23 0x0000555556188c49 in object_initialize_with_type (obj=0x555557d34760, size=666592, type=0x5555579af6e0) at ../../qom/object.c:570 #24 0x0000555556188cb5 in object_initialize (data=0x555557d34760, size=666592, typename=0x555556739030 "bcm2836") at ../../qom/object.c:578 #25 0x0000555556188e3d in object_initialize_child_with_propsv (parentobj=0x555557d34500, propname=0x55555673917b "soc", childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836", errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd8f0) at ../../qom/object.c:608 #26 0x0000555556188db6 in object_initialize_child_with_props (parentobj=0x555557d34500, propname=0x55555673917b "soc", childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836", errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591 #27 0x0000555556188f3b in object_initialize_child_internal (parent=0x555557d34500, propname=0x55555673917b "soc", child=0x555557d34760, size=666592, type=0x555556739030 "bcm2836") at ../../qom/object.c:645 #28 0x0000555555f0859b in raspi_machine_init (machine=0x555557d34500) at ../../hw/arm/raspi.c:313 #29 0x00005555559d4674 in machine_run_board_init (machine=0x555557d34500, mem_path=0x0, errp=0x7fffffffda90) at ../../hw/core/machine.c:1680 #30 0x0000555555d8615b in qemu_init_board () at ../../system/vl.c:2709 #31 0x0000555555d8650c in qmp_x_exit_preconfig (errp=0x555557863700 <error_fatal>) at ../../system/vl.c:2805 #32 0x0000555555d891bf in qemu_init (argc=5, argv=0x7fffffffde48) at ../../system/vl.c:3838 #33 0x000055555634c832 in main (argc=5, argv=0x7fffffffde48) at ../../system/main.c:68 (gdb) frame 7 #7 0x0000555556188bc6 in object_initialize_with_type (obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at ../../qom/object.c:562 562 g_assert(size >= type->instance_size); (gdb) print *type $2 = {name = 0x555557a0bec0 "pl011", class_size = 208, instance_size = 1360, instance_align = 16, class_init = 0x55555634ede0 <qemu_api::qom::rust_class_init<pl011::device::PL011State>>, class_base_init = 0x0, class_data = 0x0, instance_init = 0x55555634f0f0 <qemu_api::qom::rust_instance_init<pl011::device::PL011State>>, instance_post_init = 0x55555634f1e0 <qemu_api::qom::rust_instance_post_init<pl011::device::PL011State>>, instance_finalize = 0x55555634eb40 <qemu_api::qom::drop_object<pl011::device::PL011State>>, abstract = false, parent = 0x555557a0bee0 "sys-bus-device", parent_type = 0x55555798c650, class = 0x555557a72370, num_interfaces = 0, interfaces = { {typename = 0x0} <repeats 32 times>}} (gdb) print /x type->instance_size $3 = 0x550 (gdb) print /x size $4 = 0x540 thanks -- PMM
On 3/19/25 20:25, Peter Maydell wrote: > On Sun, 9 Mar 2025 at 10:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> Switch bindings::CharBackend with chardev::CharBackend. This removes >> occurrences of "unsafe" due to FFI and switches the wrappers for receive, >> can_receive and event callbacks to the common ones implemented by >> chardev::CharBackend. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Hi -- this commit seems to have broken use of the PL011 in > boards/SoCs that directly embed it in their state structs, so > "qemu-system-arm -M raspi2b -display none" now asserts on startup. > > The Rust PL011's state struct size is now larger than the > C state struct size, so it trips the assert in the QOM code > that we didn't try to initialize a type into less memory than > it needs. (I don't understand *why* the type size has changed, > because this commit doesn't at first glance seem to be adding > anything to the state struct...but it definitely goes up from > 0x540 to 0x550.) Thanks very much for reporting this. The reason why it changes is that it switches the imported symbol from bindings::CharBackend (the C struct) to chardev::CharBackend which has two extra values in it (a count and some debugging info to provide better backtraces on error). It is guaranteed to _start_ with a bindings::CharBackend, which is helpful for the qdev property, but it's bigger. I don't think there's a good fix other than not using an embedded PL011, since you probably would not have that option available when using a device without a Rust equivalent. Paolo > (It would be good if we had a compile time check that the state > struct sizes matched between Rust and C, rather than having it > only be caught in runtime asserts. This does cause failures in > check-functional, at least, so it's not completely untested.) > > Here's the repro and gdb backtrace: > > $ gdb --args ./build/rust/qemu-system-arm -M raspi2b -display none > [...] > ** > ERROR:../../qom/object.c:562:object_initialize_with_type: assertion > failed: (size >= type->instance_size) > Bail out! ERROR:../../qom/object.c:562:object_initialize_with_type: > assertion failed: (size >= type->instance_size) > > Thread 1 "qemu-system-arm" received signal SIGABRT, Aborted. > Download failed: Invalid argument. Continuing without source file > ./nptl/./nptl/pthread_kill.c. > __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimised > out>) at ./nptl/pthread_kill.c:44 > warning: 44 ./nptl/pthread_kill.c: No such file or directory > (gdb) bt > #0 __pthread_kill_implementation (no_tid=0, signo=6, > threadid=<optimised out>) at ./nptl/pthread_kill.c:44 > #1 __pthread_kill_internal (signo=6, threadid=<optimised out>) at > ./nptl/pthread_kill.c:78 > #2 __GI___pthread_kill (threadid=<optimised out>, > signo=signo@entry=6) at ./nptl/pthread_kill.c:89 > #3 0x00007ffff4a4527e in __GI_raise (sig=sig@entry=6) at > ../sysdeps/posix/raise.c:26 > #4 0x00007ffff4a288ff in __GI_abort () at ./stdlib/abort.c:79 > #5 0x00007ffff6e58f5b in g_assertion_message > (domain=domain@entry=0x0, file=file@entry=0x55555678fdeb > "../../qom/object.c", line=line@entry=562, > func=func@entry=0x5555567906d0 <__func__.33> > "object_initialize_with_type", message=message@entry=0x555557f7f400 > "assertion failed: (size >= type->instance_size)") at > ../../../glib/gtestutils.c:3331 > #6 0x00007ffff6ec1a97 in g_assertion_message_expr > (domain=0x0, file=0x55555678fdeb "../../qom/object.c", line=562, > func=0x5555567906d0 <__func__.33> "object_initialize_with_type", > expr=<optimised out>) at ../../../glib/gtestutils.c:3357 > #7 0x0000555556188bc6 in object_initialize_with_type > (obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at > ../../qom/object.c:562 > #8 0x0000555556188cb5 in object_initialize (data=0x555557d4e190, > size=1344, typename=0x5555566d9142 "pl011") > at ../../qom/object.c:578 > #9 0x0000555556188e3d in object_initialize_child_with_propsv > (parentobj=0x555557d45710, propname=0x5555566d9148 "uart0", > childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011", > errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd360) at > ../../qom/object.c:608 > #10 0x0000555556188db6 in object_initialize_child_with_props > (parentobj=0x555557d45710, propname=0x5555566d9148 "uart0", > childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011", > errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591 > #11 0x0000555556188f3b in object_initialize_child_internal > (parent=0x555557d45710, propname=0x5555566d9148 "uart0", > child=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011") > at ../../qom/object.c:645 > #12 0x0000555555d446ea in raspi_peripherals_base_init > (obj=0x555557d45710) at ../../hw/arm/bcm2835_peripherals.c:100 > #13 0x0000555556188639 in object_init_with_type (obj=0x555557d45710, > ti=0x5555579d4af0) at ../../qom/object.c:428 > #14 0x000055555618861b in object_init_with_type (obj=0x555557d45710, > ti=0x5555579d4950) at ../../qom/object.c:424 > #15 0x0000555556188c49 in object_initialize_with_type > (obj=0x555557d45710, size=597040, type=0x5555579d4950) > at ../../qom/object.c:570 > #16 0x0000555556188cb5 in object_initialize (data=0x555557d45710, > size=597040, typename=0x555556738ca5 "bcm2835-peripherals") > at ../../qom/object.c:578 > #17 0x0000555556188e3d in object_initialize_child_with_propsv > (parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals", > childobj=0x555557d45710, size=597040, type=0x555556738ca5 > "bcm2835-peripherals", errp=0x5555578636f8 <error_abort>, > vargs=0x7fffffffd630) at ../../qom/object.c:608 > #18 0x0000555556188db6 in object_initialize_child_with_props > (parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals", > childobj=0x555557d45710, size=597040, type=0x555556738ca5 > "bcm2835-peripherals", errp=0x5555578636f8 <error_abort>) at > ../../qom/object.c:591 > #19 0x0000555556188f3b in object_initialize_child_internal > (parent=0x555557d34760, propname=0x555556738cb9 "peripherals", > child=0x555557d45710, size=597040, type=0x555556738ca5 > "bcm2835-peripherals") at ../../qom/object.c:645 > #20 0x0000555555f07080 in bcm283x_init (obj=0x555557d34760) at > ../../hw/arm/bcm2836.c:49 > #21 0x0000555556188639 in object_init_with_type (obj=0x555557d34760, > ti=0x5555579af8a0) at ../../qom/object.c:428 > #22 0x000055555618861b in object_init_with_type (obj=0x555557d34760, > ti=0x5555579af6e0) at ../../qom/object.c:424 > #23 0x0000555556188c49 in object_initialize_with_type > (obj=0x555557d34760, size=666592, type=0x5555579af6e0) > at ../../qom/object.c:570 > #24 0x0000555556188cb5 in object_initialize (data=0x555557d34760, > size=666592, typename=0x555556739030 "bcm2836") > at ../../qom/object.c:578 > #25 0x0000555556188e3d in object_initialize_child_with_propsv > (parentobj=0x555557d34500, propname=0x55555673917b "soc", > childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836", > errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd8f0) at > ../../qom/object.c:608 > #26 0x0000555556188db6 in object_initialize_child_with_props > (parentobj=0x555557d34500, propname=0x55555673917b "soc", > childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836", > errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591 > #27 0x0000555556188f3b in object_initialize_child_internal > (parent=0x555557d34500, propname=0x55555673917b "soc", > child=0x555557d34760, size=666592, type=0x555556739030 "bcm2836") > at ../../qom/object.c:645 > #28 0x0000555555f0859b in raspi_machine_init (machine=0x555557d34500) > at ../../hw/arm/raspi.c:313 > #29 0x00005555559d4674 in machine_run_board_init > (machine=0x555557d34500, mem_path=0x0, errp=0x7fffffffda90) > at ../../hw/core/machine.c:1680 > #30 0x0000555555d8615b in qemu_init_board () at ../../system/vl.c:2709 > #31 0x0000555555d8650c in qmp_x_exit_preconfig (errp=0x555557863700 > <error_fatal>) at ../../system/vl.c:2805 > #32 0x0000555555d891bf in qemu_init (argc=5, argv=0x7fffffffde48) at > ../../system/vl.c:3838 > #33 0x000055555634c832 in main (argc=5, argv=0x7fffffffde48) at > ../../system/main.c:68 > (gdb) frame 7 > #7 0x0000555556188bc6 in object_initialize_with_type > (obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at > ../../qom/object.c:562 > 562 g_assert(size >= type->instance_size); > (gdb) print *type > $2 = {name = 0x555557a0bec0 "pl011", class_size = 208, instance_size = > 1360, instance_align = 16, > class_init = 0x55555634ede0 > <qemu_api::qom::rust_class_init<pl011::device::PL011State>>, > class_base_init = 0x0, class_data = 0x0, > instance_init = 0x55555634f0f0 > <qemu_api::qom::rust_instance_init<pl011::device::PL011State>>, > instance_post_init = 0x55555634f1e0 > <qemu_api::qom::rust_instance_post_init<pl011::device::PL011State>>, > instance_finalize = 0x55555634eb40 > <qemu_api::qom::drop_object<pl011::device::PL011State>>, abstract = > false, > parent = 0x555557a0bee0 "sys-bus-device", parent_type = > 0x55555798c650, class = 0x555557a72370, num_interfaces = 0, interfaces > = { > {typename = 0x0} <repeats 32 times>}} > (gdb) print /x type->instance_size > $3 = 0x550 > (gdb) print /x size > $4 = 0x540 > > thanks > -- PMM > >
On Wed, 19 Mar 2025 at 20:51, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 3/19/25 20:25, Peter Maydell wrote: > > Hi -- this commit seems to have broken use of the PL011 in > > boards/SoCs that directly embed it in their state structs, so > > "qemu-system-arm -M raspi2b -display none" now asserts on startup. > > > > The Rust PL011's state struct size is now larger than the > > C state struct size, so it trips the assert in the QOM code > > that we didn't try to initialize a type into less memory than > > it needs. > The reason why it changes is that it switches the imported symbol from > bindings::CharBackend (the C struct) to chardev::CharBackend which has > two extra values in it (a count and some debugging info to provide > better backtraces on error). It is guaranteed to _start_ with a > bindings::CharBackend, which is helpful for the qdev property, but it's > bigger. > > I don't think there's a good fix other than not using an embedded PL011, > since you probably would not have that option available when using a > device without a Rust equivalent. (do you mean "without a C equivalent" there?) Hmm. The embedded-struct approach to devices is extremely common, especially in more recent QEMU C code. Generally those users of an embedded struct don't actually poke around inside the struct, so we don't need to have the Rust code match the C layout, but we do at least need the size to be no bigger than the C version. (There are some exceptions for some devices, not including the PL011, where people do poke around inside the struct of a device they've created...) There has been discussion that we ought to move (back!) to a "users of the device just get a pointer to it and the struct is opaque to them" design pattern -- command line generation and wiring up of machines would also make that a more natural approach -- but that's a long term process we'd need to plan and go through. I had some ideas a long time ago for making "code outside the C device implementation touched a field in the device struct" be a compile error using attribute("deprecated") hidden via macros, so we could resurrect that as a way to confirm that nobody is trying to do dubious things before we do a C-to-rust conversion of a device model. For the moment, I guess the expedient thing to do would be to add an extra 16 bytes of padding to the C PL011State struct ? thanks -- PMM
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 4cdbbf4b73d..4e282bc9e9d 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -2,18 +2,10 @@ // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> // SPDX-License-Identifier: GPL-2.0-or-later -use std::{ - ffi::CStr, - os::raw::{c_int, c_void}, - ptr::{addr_of, addr_of_mut, NonNull}, -}; +use std::{ffi::CStr, ptr::addr_of_mut}; use qemu_api::{ - bindings::{ - qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers, - qemu_chr_fe_write_all, CharBackend, QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK, - }, - chardev::Chardev, + chardev::{CharBackend, Chardev, Event}, impl_vmstate_forward, irq::{IRQState, InterruptSource}, memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder}, @@ -235,7 +227,7 @@ pub(self) fn write( &mut self, offset: RegisterOffset, value: u32, - char_backend: *mut CharBackend, + char_backend: &CharBackend, ) -> bool { // eprintln!("write offset {offset} value {value}"); use RegisterOffset::*; @@ -269,17 +261,9 @@ pub(self) fn write( self.reset_tx_fifo(); } let update = (self.line_control.send_break() != new_val.send_break()) && { - let mut break_enable: c_int = new_val.send_break().into(); - // SAFETY: self.char_backend is a valid CharBackend instance after it's been - // initialized in realize(). - unsafe { - qemu_chr_fe_ioctl( - char_backend, - CHR_IOCTL_SERIAL_SET_BREAK as i32, - addr_of_mut!(break_enable).cast::<c_void>(), - ); - } - self.loopback_break(break_enable > 0) + let break_enable = new_val.send_break(); + let _ = char_backend.send_break(break_enable); + self.loopback_break(break_enable) }; self.line_control = new_val; self.set_read_trigger(); @@ -551,9 +535,7 @@ fn read(&self, offset: hwaddr, _size: u32) -> u64 { let (update_irq, result) = self.regs.borrow_mut().read(field); if update_irq { self.update(); - unsafe { - qemu_chr_fe_accept_input(addr_of!(self.char_backend) as *mut _); - } + self.char_backend.accept_input(); } result.into() } @@ -567,21 +549,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) { // callback, so handle writes before entering PL011Registers. if field == RegisterOffset::DR { // ??? Check if transmitter is enabled. - let ch: u8 = value as u8; - // SAFETY: char_backend is a valid CharBackend instance after it's been - // initialized in realize(). + let ch: [u8; 1] = [value as u8]; // XXX this blocks entire thread. Rewrite to use // qemu_chr_fe_write and background I/O callbacks - unsafe { - qemu_chr_fe_write_all(addr_of!(self.char_backend) as *mut _, &ch, 1); - } + let _ = self.char_backend.write_all(&ch); } - update_irq = self.regs.borrow_mut().write( - field, - value as u32, - addr_of!(self.char_backend) as *mut _, - ); + update_irq = self + .regs + .borrow_mut() + .write(field, value as u32, &self.char_backend); } else { eprintln!("write bad offset {offset} value {value}"); } @@ -590,15 +567,18 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) { } } - pub fn can_receive(&self) -> bool { - // trace_pl011_can_receive(s->lcr, s->read_count, r); + fn can_receive(&self) -> u32 { let regs = self.regs.borrow(); - regs.read_count < regs.fifo_depth() + // trace_pl011_can_receive(s->lcr, s->read_count, r); + u32::from(regs.read_count < regs.fifo_depth()) } - pub fn receive(&self, ch: u32) { + fn receive(&self, buf: &[u8]) { + if buf.is_empty() { + return; + } let mut regs = self.regs.borrow_mut(); - let update_irq = !regs.loopback_enabled() && regs.put_fifo(ch); + let update_irq = !regs.loopback_enabled() && regs.put_fifo(buf[0].into()); // Release the BqlRefCell before calling self.update() drop(regs); @@ -607,10 +587,10 @@ pub fn receive(&self, ch: u32) { } } - pub fn event(&self, event: QEMUChrEvent) { + fn event(&self, event: Event) { let mut update_irq = false; let mut regs = self.regs.borrow_mut(); - if event == QEMUChrEvent::CHR_EVENT_BREAK && !regs.loopback_enabled() { + if event == Event::CHR_EVENT_BREAK && !regs.loopback_enabled() { update_irq = regs.put_fifo(registers::Data::BREAK.into()); } // Release the BqlRefCell before calling self.update() @@ -622,20 +602,8 @@ pub fn event(&self, event: QEMUChrEvent) { } fn realize(&self) { - // SAFETY: self.char_backend has the correct size and alignment for a - // CharBackend object, and its callbacks are of the correct types. - unsafe { - qemu_chr_fe_set_handlers( - addr_of!(self.char_backend) as *mut CharBackend, - Some(pl011_can_receive), - Some(pl011_receive), - Some(pl011_event), - None, - addr_of!(*self).cast::<c_void>() as *mut c_void, - core::ptr::null_mut(), - true, - ); - } + self.char_backend + .enable_handlers(self, Self::can_receive, Self::receive, Self::event); } fn reset_hold(&self, _type: ResetType) { @@ -666,43 +634,6 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> { Interrupt::E.0, ]; -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_can_receive(opaque: *mut c_void) -> c_int { - let state = NonNull::new(opaque).unwrap().cast::<PL011State>(); - unsafe { state.as_ref().can_receive().into() } -} - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -/// -/// The buffer and size arguments must also be valid. -pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const u8, size: c_int) { - let state = NonNull::new(opaque).unwrap().cast::<PL011State>(); - unsafe { - if size > 0 { - debug_assert!(!buf.is_null()); - state.as_ref().receive(u32::from(buf.read_volatile())); - } - } -} - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_event(opaque: *mut c_void, event: QEMUChrEvent) { - let state = NonNull::new(opaque).unwrap().cast::<PL011State>(); - unsafe { state.as_ref().event(event) } -} - /// # Safety /// /// We expect the FFI user of this function to pass a valid pointer for `chr`
Switch bindings::CharBackend with chardev::CharBackend. This removes occurrences of "unsafe" due to FFI and switches the wrappers for receive, can_receive and event callbacks to the common ones implemented by chardev::CharBackend. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/hw/char/pl011/src/device.rs | 119 +++++++------------------------ 1 file changed, 25 insertions(+), 94 deletions(-)