Re: [RFC PATCH v5 0/8] Add Rust support, implement ARM PL011

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Fri, 26 Jul 2024 12:26, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote:
>On Fri, 26 Jul 2024 at 11:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> As I said, I don't see the point in discussing this more, and I'm not
>> going to unless you provide a clear pointer to documentation that
>> states the opposite.
>
>Same here.

Next patch series version is taking more time than I expected because of 
unrelated issues, plus I'm on PTO these days, so I am posting the 
resolution (which I hope satisfies everyone) here early:

It's possible to refactor this code (and any other similar cases) to 
prevent recursive mutable calls:

diff --git a/rust/hw/char/pl011/src/device.rs 
b/rust/hw/char/pl011/src/device.rs
index 3643b7bdee..449930e34e 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -104,10 +127,14 @@ pub fn init(&mut self) {
         }
     }
 
-    pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 {
+    pub fn read(
+        &mut self,
+        offset: hwaddr,
+        _size: core::ffi::c_uint,
+    ) -> std::ops::ControlFlow<u64, u64> {
         use RegisterOffset::*;
 
-        match RegisterOffset::try_from(offset) {
+        std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
             Err(v) if (0x3f8..0x400).contains(&v) => {
                 u64::from(PL011_ID_ARM[((offset - 0xfe0) >> 2) as usize])
             }
@@ -134,10 +161,8 @@ pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 {
                 // Update error bits.
                 self.receive_status_error_clear = c.to_be_bytes()[3].into();
                 self.update();
-                // SAFETY: self.char_backend is a valid CharBackend instance after it's been
-                // initialized in realize().
-                unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };
-                c.into()
+                // Must call qemu_chr_fe_accept_input, so return Continue:
+                return std::ops::ControlFlow::Continue(c.into());
             }
             Ok(RSR) => u8::from(self.receive_status_error_clear).into(),
             Ok(FR) => u16::from(self.flags).into(),
@@ -159,7 +184,7 @@ pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 {
                 0
             }
             Ok(DMACR) => self.dmacr.into(),
-        }
+        })
     }
 
     pub fn write(&mut self, offset: hwaddr, value: u64) {
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
index 6144d28586..5e185b7cd7 100644
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ b/rust/hw/char/pl011/src/memory_ops.rs
@@ -29,7 +29,18 @@
 ) -> u64 {
     assert!(!opaque.is_null());
     let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
-    state.as_mut().read(addr, size)
+    let val = state.as_mut().read(addr, size);
+    match val {
+        std::ops::ControlFlow::Break(val) => val,
+        std::ops::ControlFlow::Continue(val) => {
+            // SAFETY: self.char_backend is a valid CharBackend instance after it's been
+            // initialized in realize().
+            let cb_ptr = core::ptr::addr_of_mut!(state.as_mut().char_backend);
+            unsafe { qemu_chr_fe_accept_input(cb_ptr) };
+
+            val
+        }
+    }
 }

When we iterate the APIs further we will find better idiomatic solutions 
that do not rely on devices themselves to do this kind of stuff, but 
offer interfaces from the `qemu_api` library crate.

Manos