| Message ID | 20250227164538.814576-5-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: pl011 cleanups + chardev bindings | expand |
On Thu, 27 Feb 2025 at 16:48, 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> > @@ -567,21 +552,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) { > - 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}"); > } Entirely unrelated to this patch, but seeing this go past reminded me that I had a question I didn't get round to asking in the community call the other day. In this PL011State::write function, we delegate the job of updating the register state to PL011Registers::write, which returns a bool to tell us whether to call update(). I guess the underlying design idea here is "the register object updates itself and tells the device object what kinds of updates to the outside world it needs to do" ? But then, why is the irq output something that PL011State needs to handle itself whereas the chardev backend is something we can pass into PL011Registers ? In the C version, we just call pl011_update() where we need to; we could validly call it unconditionally for any write, we're just being (possibly prematurely) efficient by avoiding a call when we happen to know that the register write didn't touch any of the state that pl011_update() cares about. So it feels a bit odd to me that in the Rust version this "we happen to know that sometimes it would be unnecessary to call the update function" has been kind of promoted to being part of an interface between the two different types PL011Registers and PL011State. Thinking about other devices, presumably for more complex devices we might need to pass more than just a single 'bool' back from PL011Registers::write. What other kinds of thing might we need to do in the FooState function, and (since the pl011 code is presumably going to be used as a template for those other devices) is it worth having something that expresses that better than just a raw 'bool' return ? thanks -- PMM
On Thu, Feb 27, 2025 at 6:25 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 27 Feb 2025 at 16:48, 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> > > > @@ -567,21 +552,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) { > > > - 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}"); > > } > > Entirely unrelated to this patch, but seeing this go past > reminded me that I had a question I didn't get round to > asking in the community call the other day. In this > PL011State::write function, we delegate the job of > updating the register state to PL011Registers::write, > which returns a bool to tell us whether to call update(). > > I guess the underlying design idea here is "the register > object updates itself and tells the device object what > kinds of updates to the outside world it needs to do" ? > But then, why is the irq output something that PL011State > needs to handle itself whereas the chardev backend is > something we can pass into PL011Registers ? Just because the IRQ update is needed in many places and the chardev backend only in one place. > In the C version, we just call pl011_update() where we > need to; we could validly call it unconditionally for any > write, we're just being (possibly prematurely) efficient > by avoiding a call when we happen to know that the register > write didn't touch any of the state that pl011_update() > cares about. So it feels a bit odd to me that in the Rust > version this "we happen to know that sometimes it would be > unnecessary to call the update function" has been kind of > promoted to being part of an interface between the two > different types PL011Registers and PL011State. Yeah, if I was writing from scratch I would probably call update() unconditionally. If it turns out to be inefficient you could cache the current value of let flags = regs.int_level & regs.int_enabled; in PL011State as a BqlCell. > Thinking about other devices, presumably for more complex > devices we might need to pass more than just a single 'bool' > back from PL011Registers::write. What other kinds of thing > might we need to do in the FooState function, and (since > the pl011 code is presumably going to be used as a template > for those other devices) is it worth having something that > expresses that better than just a raw 'bool' return ? Ideally nothing, especially considering that more modern devices have edge-triggered interrupts like MSIs, instead of level-triggered interrupts that need qemu_set_irq() calls. But if you have something a lot more complex than a bool I would pass down the PL011State and do something like pl011.schedule_update_irq() which updates a BqlCell<>. The device could then use a bottom half or process them after "drop(regs)". HPET has another approach, which is to store a backpointer from HPETTimer to the HPETState, so that it can do self.get_state().irqs[route].pulse(); without passing down anything. The reason for this is that it has multiple timers on the same routine, and it assigns the timers to separate HPETTimers. I would not use it for PL011 because all accesses to the PL011Registers go through the PL011State. A while ago I checked how OpenVMM does it, and basically it does not have the PL011State/PL011Registers separation at all: the devices are automatically wrapped with a Mutex and memory accesses take a &mut. That removes some of the complexity, but also a lot of flexibility. Unfortunately, before being able to reason on how to make peace with the limitations of safe Rust, it was necessary to spend a lot of time writing API abstractions, otherwise you don't actually experience the limitations. But that means that the number of lines of code in qemu_api is not representative of my experience using it. :( I am sorry this isn't a great answer yet; certainly some aspects of the PL011 or HPET devices could be treated as a blueprint for future devices, but which and how to document that is something where I would like to consult with an actual Rust maven. Paolo
On Thu, 27 Feb 2025 at 18:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On Thu, Feb 27, 2025 at 6:25 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > Thinking about other devices, presumably for more complex > > devices we might need to pass more than just a single 'bool' > > back from PL011Registers::write. What other kinds of thing > > might we need to do in the FooState function, and (since > > the pl011 code is presumably going to be used as a template > > for those other devices) is it worth having something that > > expresses that better than just a raw 'bool' return ? > > Ideally nothing, especially considering that more modern devices have > edge-triggered interrupts like MSIs, instead of level-triggered > interrupts that need qemu_set_irq() calls. But if you have something a > lot more complex than a bool I would pass down the PL011State and do > something like pl011.schedule_update_irq() which updates a BqlCell<>. > The device could then use a bottom half or process them after > "drop(regs)". > > HPET has another approach, which is to store a backpointer from > HPETTimer to the HPETState, so that it can do > > self.get_state().irqs[route].pulse(); > > without passing down anything. The reason for this is that it has > multiple timers on the same routine, and it assigns the timers to > separate HPETTimers. I would not use it for PL011 because all accesses > to the PL011Registers go through the PL011State. I think the idea I vaguely have in the back of my mind is that maybe it's a nice idea to have a coding structure that enforces "you update your own internal state, and only then do things that might involve calling out to the outside world, and if there's something that you need to do with the result of that callout, there's an easy mechanism for 'this is what I will want to do next after that' continuations". The fact that our C device implementations don't do that is kind of the underlying cause of all the "recursive entry back into the device via DMA" problems that we squashed with the big hammer of "just forbid it entirely". It's also in a way the problem underlying the race condition segfault here: https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=Wn+k8dQneB_ynQ@mail.gmail.com/T/#u (memory_region_snapshot_and_clear_dirty() drops the BKL, no callers expect that, segfaults in the calling code in the framebuffer device models if something else gets in and e.g. resizes the framebuffer in the middle of a display update). So I was sort of wondering if the pl011 structure was aiming at providing that kind of separation of "internal state" and "external interactions", such that the compiler would complain if you tried to do an externally-interacting thing while your internal state was not consistent. thanks -- PMM
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 4cdbbf4b73d..2283bae71eb 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -4,16 +4,11 @@ use std::{ ffi::CStr, - os::raw::{c_int, c_void}, - ptr::{addr_of, addr_of_mut, NonNull}, + 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 +230,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 +264,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 +538,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 +552,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 +570,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 +590,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 +605,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 +637,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 | 116 +++++++------------------------ 1 file changed, 25 insertions(+), 91 deletions(-)