Add rcu_read_lock() & rcu_read_unlock() bindings, then they can be used
in memory access.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/qemu-api/meson.build |  1 +
 rust/qemu-api/src/lib.rs  |  1 +
 rust/qemu-api/src/rcu.rs  | 26 ++++++++++++++++++++++++++
 rust/qemu-api/wrapper.h   |  1 +
 4 files changed, 29 insertions(+)
 create mode 100644 rust/qemu-api/src/rcu.rs

diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index a362d44ed396..d40472092248 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -68,6 +68,7 @@ _qemu_api_rs = static_library(
       'src/prelude.rs',
       'src/qdev.rs',
       'src/qom.rs',
+      'src/rcu.rs',
       'src/sysbus.rs',
       'src/timer.rs',
       'src/uninit.rs',
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 86dcd8ef17a9..4705cf9ccbc5 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -26,6 +26,7 @@
 pub mod module;
 pub mod qdev;
 pub mod qom;
+pub mod rcu;
 pub mod sysbus;
 pub mod timer;
 pub mod uninit;
diff --git a/rust/qemu-api/src/rcu.rs b/rust/qemu-api/src/rcu.rs
new file mode 100644
index 000000000000..30d8b9e43967
--- /dev/null
+++ b/rust/qemu-api/src/rcu.rs
@@ -0,0 +1,26 @@
+// Copyright (C) 2025 Intel Corporation.
+// Author(s): Zhao Liu <zhao1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Bindings for `rcu_read_lock` and `rcu_read_unlock`.
+//! More details about RCU in QEMU, please refer docs/devel/rcu.rst.
+
+use crate::bindings;
+
+/// Used by a reader to inform the reclaimer that the reader is
+/// entering an RCU read-side critical section.
+pub fn rcu_read_lock() {
+    // SAFETY: no return and no argument, everything is done at C side.
+    unsafe { bindings::rcu_read_lock() }
+}
+
+/// Used by a reader to inform the reclaimer that the reader is
+/// exiting an RCU read-side critical section.  Note that RCU
+/// read-side critical sections may be nested and/or overlapping.
+pub fn rcu_read_unlock() {
+    // SAFETY: no return and no argument, everything is done at C side.
+    unsafe { bindings::rcu_read_unlock() }
+}
+
+// FIXME: maybe we need rcu_read_lock_held() to check the rcu context,
+// then make it possible to add assertion at any RCU critical section.
diff --git a/rust/qemu-api/wrapper.h b/rust/qemu-api/wrapper.h
index 15a1b19847f2..ce0ac8d3f550 100644
--- a/rust/qemu-api/wrapper.h
+++ b/rust/qemu-api/wrapper.h
@@ -69,3 +69,4 @@ typedef enum memory_order {
 #include "qemu/timer.h"
 #include "system/address-spaces.h"
 #include "hw/char/pl011.h"
+#include "qemu/rcu.h"
-- 
2.34.1

On Thu, Aug 7, 2025 at 3:09 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Add rcu_read_lock() & rcu_read_unlock() bindings, then they can be used
> in memory access.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  rust/qemu-api/meson.build |  1 +
>  rust/qemu-api/src/lib.rs  |  1 +
>  rust/qemu-api/src/rcu.rs  | 26 ++++++++++++++++++++++++++
>  rust/qemu-api/wrapper.h   |  1 +
>  4 files changed, 29 insertions(+)
>  create mode 100644 rust/qemu-api/src/rcu.rs
>
> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index a362d44ed396..d40472092248 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -68,6 +68,7 @@ _qemu_api_rs = static_library(
>        'src/prelude.rs',
>        'src/qdev.rs',
>        'src/qom.rs',
> +      'src/rcu.rs',
>        'src/sysbus.rs',
>        'src/timer.rs',
>        'src/uninit.rs',
> diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
> index 86dcd8ef17a9..4705cf9ccbc5 100644
> --- a/rust/qemu-api/src/lib.rs
> +++ b/rust/qemu-api/src/lib.rs
> @@ -26,6 +26,7 @@
>  pub mod module;
>  pub mod qdev;
>  pub mod qom;
> +pub mod rcu;
>  pub mod sysbus;
>  pub mod timer;
>  pub mod uninit;
> diff --git a/rust/qemu-api/src/rcu.rs b/rust/qemu-api/src/rcu.rs
> new file mode 100644
> index 000000000000..30d8b9e43967
> --- /dev/null
> +++ b/rust/qemu-api/src/rcu.rs
> @@ -0,0 +1,26 @@
> +// Copyright (C) 2025 Intel Corporation.
> +// Author(s): Zhao Liu <zhao1.liu@intel.com>
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +//! Bindings for `rcu_read_lock` and `rcu_read_unlock`.
> +//! More details about RCU in QEMU, please refer docs/devel/rcu.rst.
> +

How about a RAII guard type? e.g. RCUGuard and runs `rcu_read_unlock` on Drop.

Destructors are not guaranteed to run or run only once, but the former
should happen when things go wrong e.g. crashes/aborts. You can add a
flag in the RCUGuard to make sure Drop runs unlock only once (since it
takes &mut and not ownership)

> +use crate::bindings;
> +
> +/// Used by a reader to inform the reclaimer that the reader is
> +/// entering an RCU read-side critical section.
> +pub fn rcu_read_lock() {
> +    // SAFETY: no return and no argument, everything is done at C side.
> +    unsafe { bindings::rcu_read_lock() }
> +}
> +
> +/// Used by a reader to inform the reclaimer that the reader is
> +/// exiting an RCU read-side critical section.  Note that RCU
> +/// read-side critical sections may be nested and/or overlapping.
> +pub fn rcu_read_unlock() {
> +    // SAFETY: no return and no argument, everything is done at C side.
> +    unsafe { bindings::rcu_read_unlock() }
> +}
> +
> +// FIXME: maybe we need rcu_read_lock_held() to check the rcu context,
> +// then make it possible to add assertion at any RCU critical section.

This would be less necessary with Drop, maybe.

> diff --git a/rust/qemu-api/wrapper.h b/rust/qemu-api/wrapper.h
> index 15a1b19847f2..ce0ac8d3f550 100644
> --- a/rust/qemu-api/wrapper.h
> +++ b/rust/qemu-api/wrapper.h
> @@ -69,3 +69,4 @@ typedef enum memory_order {
>  #include "qemu/timer.h"
>  #include "system/address-spaces.h"
>  #include "hw/char/pl011.h"
> +#include "qemu/rcu.h"
> --
> 2.34.1
>

On 8/7/25 14:29, Manos Pitsidianakis wrote:

>> +//! Bindings for `rcu_read_lock` and `rcu_read_unlock`.
>> +//! More details about RCU in QEMU, please refer docs/devel/rcu.rst.
>> +
> 
> How about a RAII guard type? e.g. RCUGuard and runs `rcu_read_unlock` on Drop.

Clippy says Rcu not RCU.  :)

You're right, not just because it's nice but also because it bounds the 
dereference of the FlatView.  Something like this build on top of the 
guard object:

pub struct RcuCell<T> {
     data: AtomicPtr<T>
}

impl<T> RcuCell {
     pub fn raw_get(&self) -> *mut T {
         self.data.load(Ordering::Acquire)
     }

     pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
         unsafe {
             self.raw_get().as_ref()
         }
     }
}

Using this is a bit ugly, because you need transmute, but it's isolated:

impl AddressSpace {
    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {
        let flatp = unsafe {
            std::mem::transmute::<&*mut FlatView, &RcuCell<FlatView>>(
                &self.0.as_ptr().current_map)
        };
        flatp.get(rcu)
    }
}

impl GuestAddressSpace for AddressSpace {
     fn memory(&self) -> Self::T {
         let rcu = RcuGuard::guard();
         FlatViewRefGuard::new(self.get_flatview(rcu))
     }
}

> Destructors are not guaranteed to run or run only once, but the former
> should happen when things go wrong e.g. crashes/aborts. You can add a
> flag in the RCUGuard to make sure Drop runs unlock only once (since it
> takes &mut and not ownership)

Yeah I think many things would go wrong if Arc could run its drop 
implementation more than once.

Paolo

On Thu, Aug 07, 2025 at 03:38:52PM +0200, Paolo Bonzini wrote:
> Date: Thu, 7 Aug 2025 15:38:52 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 13/26] rust: Add RCU bindings
> 
> On 8/7/25 14:29, Manos Pitsidianakis wrote:
> 
> > > +//! Bindings for `rcu_read_lock` and `rcu_read_unlock`.
> > > +//! More details about RCU in QEMU, please refer docs/devel/rcu.rst.
> > > +
> > 
> > How about a RAII guard type? e.g. RCUGuard and runs `rcu_read_unlock` on Drop.
> 
> Clippy says Rcu not RCU.  :)
> 
> You're right, not just because it's nice but also because it bounds the
> dereference of the FlatView.  Something like this build on top of the guard
> object:
> 
> pub struct RcuCell<T> {
>     data: AtomicPtr<T>
> }
> 
> impl<T> RcuCell {
>     pub fn raw_get(&self) -> *mut T {
>         self.data.load(Ordering::Acquire)
>     }
> 
>     pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
>         unsafe {
>             self.raw_get().as_ref()
>         }
>     }
> }

I just implement a simple RcuGuard (but this doesn't consider the refer
count or flag. I would like to talk more about this at the last of this
reply.):

pub struct RcuGuard;

impl RcuGuard {
    pub fn new() -> Self {
        unsafe { bindings::rcu_read_lock() };
        Self
    }
}

impl Drop for RcuGuard {
    fn drop(&mut self) {
        unsafe { bindings::rcu_read_unlock() };
    }
}

> Using this is a bit ugly, because you need transmute, but it's isolated:
> 
> impl AddressSpace {
>    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {
>        let flatp = unsafe {
>            std::mem::transmute::<&*mut FlatView, &RcuCell<FlatView>>(
>                &self.0.as_ptr().current_map)
>        };
>        flatp.get(rcu)
>    }
> }
>
> impl GuestAddressSpace for AddressSpace {
>     fn memory(&self) -> Self::T {
>         let rcu = RcuGuard::guard();
>         FlatViewRefGuard::new(self.get_flatview(rcu))
>     }
> }

Why not use a constructor RcuCell::new() to replace transmute()? Then
we just need to play with the pointer without touching memory.

impl<T> RcuCell<T> {
    pub fn new(p: *mut T) -> Self {
        Self {
            data: AtomicPtr::new(p),
        }
    }
}

Then we could :

impl Deref for AddressSpace {
    type Target = bindings::AddressSpace;

    fn deref(&self) -> &Self::Target {
        unsafe { &*self.0.as_ptr() }
    }
}

impl AddressSpace {
    pub fn get_flatview<'g>(&self, rcu: &'g RcuGuard) -> &'g FlatView {
        let flatp = RcuCell::new(self.deref().current_map);
        unsafe { FlatView::from_raw(flatp.get(rcu).unwrap()) }
    }
}

impl GuestAddressSpace for AddressSpace {
    fn memory(&self) -> Self::T {
        let rcu = RcuGuard::new();
        FlatViewRefGuard::new(self.get_flatview(&rcu)).unwrap()
    }
}

> > Destructors are not guaranteed to run or run only once, but the former
> > should happen when things go wrong e.g. crashes/aborts. You can add a
> > flag in the RCUGuard to make sure Drop runs unlock only once (since it
> > takes &mut and not ownership)
> 
> Yeah I think many things would go wrong if Arc could run its drop
> implementation more than once.

wait, isn't RCU be held at thread-local case? We shouldn't share RCU
guard/cell at Arc<>. Furthermore, it seems necessary to introduce
`NotThreadSafe` into QEMU from kernel.

pub type NotThreadSafe = PhantomData<*mut ()>;

Then we could have stronger restrictions on RCU stuff, just like
kernel'rcu:

pub struct RcuGuard(NotThreadSafe);

Maybe we can also add `NotThreadSafe` to RcuCell. But the lifetime has
already restrict its use.

For another consideration about "guaranteeing to run" (for crashes/aborts
case), QEMU program will stop and OS will clean every thing up. Then we
don't need to care about the state of RCU, right?

Thanks,
Zhao

Thank you both!!

Please correct me is I'm wrong :).

On Thu, Aug 07, 2025 at 03:38:52PM +0200, Paolo Bonzini wrote:
> Date: Thu, 7 Aug 2025 15:38:52 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 13/26] rust: Add RCU bindings
> 
> On 8/7/25 14:29, Manos Pitsidianakis wrote:
> 
> > > +//! Bindings for `rcu_read_lock` and `rcu_read_unlock`.
> > > +//! More details about RCU in QEMU, please refer docs/devel/rcu.rst.
> > > +
> > 
> > How about a RAII guard type? e.g. RCUGuard and runs `rcu_read_unlock` on Drop.
> 
> Clippy says Rcu not RCU.  :)
> 
> You're right, not just because it's nice but also because it bounds the
> dereference of the FlatView.  Something like this build on top of the guard
> object:
> 
> pub struct RcuCell<T> {
>     data: AtomicPtr<T>
> }
> 
> impl<T> RcuCell {
>     pub fn raw_get(&self) -> *mut T {
>         self.data.load(Ordering::Acquire)

I understand this tries to provide an equivalent to qatomic_rcu_read.
Ordering::Acquire is especially necessary, because at C side,
qatomic_rcu_read has a barrier.

>     }
> 
>     pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
>         unsafe {
>             self.raw_get().as_ref()
>         }
>     }
> }
> 
> Using this is a bit ugly, because you need transmute, but it's isolated:
> 
> impl AddressSpace {
>    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {

IIUC, this lifetime is using the "branded type" pattern as ParentInit.

>        let flatp = unsafe {
>            std::mem::transmute::<&*mut FlatView, &RcuCell<FlatView>>(
>                &self.0.as_ptr().current_map)
>        };
>        flatp.get(rcu)
>    }
> }
> 
> impl GuestAddressSpace for AddressSpace {
>     fn memory(&self) -> Self::T {
>         let rcu = RcuGuard::guard();
>         FlatViewRefGuard::new(self.get_flatview(rcu))
>     }
> }

With RcuGuard, then we are actually calling qatomic_rcu_read in the
rcu critical section, which greatly enhances safety. This is a good
design for RCU binding.

> > Destructors are not guaranteed to run or run only once, but the former
> > should happen when things go wrong e.g. crashes/aborts. You can add a
> > flag in the RCUGuard to make sure Drop runs unlock only once (since it
> > takes &mut and not ownership)
> 
> Yeah I think many things would go wrong if Arc could run its drop
> implementation more than once.
 
Good point.

In addition, about rcu_read_lock_held(), I thought at C side, there're
so many comments are saying "Called within RCU critical section" but
without any check.

So I wonder whether we should do some check for RCU critical section,
just like bql check via `assert!(bql_locked())`. Maybe we can have a
Rcu debug feature to cover all these checks.

Thanks,
Zhao

Il sab 9 ago 2025, 09:00 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> >     pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
> >         unsafe {
> >             self.raw_get().as_ref()
> >         }
> >     }
> > }
> >
> > Using this is a bit ugly, because you need transmute, but it's isolated:
> >
> > impl AddressSpace {
> >    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {
>
> IIUC, this lifetime is using the "branded type" pattern as ParentInit.
>

No, it's much simpler (that one uses the combination of for<'identity> and
PhantomData as explained in the comment). It says that the lifetime of the
returned reference cannot exceed the guard. It's just like

pub fn get_item(&self, array: &'g [u8]) -> &'g u8 {
   &array[self.0]
}

Except that the guard is only there to limit the lifetime and not to hold
data.

In addition, about rcu_read_lock_held(), I thought at C side, there're
> so many comments are saying "Called within RCU critical section" but
> without any check.
>
> So I wonder whether we should do some check for RCU critical section,
> just like bql check via `assert!(bql_locked())`. Maybe we can have a
> Rcu debug feature to cover all these checks.
>

In Rust you would just pass a &RcuGuard into the function (or store it in a
struct) for a zero-cost assertion that you are in the RCU critical section.

Paolo


> Thanks,
> Zhao
>
>
>

On Sat, 9 Aug 2025, 12:13 Paolo Bonzini, <pbonzini@redhat.com> wrote:

>
>
> Il sab 9 ago 2025, 09:00 Zhao Liu <zhao1.liu@intel.com> ha scritto:
>
>> >     pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
>> >         unsafe {
>> >             self.raw_get().as_ref()
>> >         }
>> >     }
>> > }
>> >
>> > Using this is a bit ugly, because you need transmute, but it's isolated:
>> >
>> > impl AddressSpace {
>> >    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {
>>
>> IIUC, this lifetime is using the "branded type" pattern as ParentInit.
>>
>
> No, it's much simpler (that one uses the combination of for<'identity> and
> PhantomData as explained in the comment). It says that the lifetime of the
> returned reference cannot exceed the guard. It's just like
>
> pub fn get_item(&self, array: &'g [u8]) -> &'g u8 {
>    &array[self.0]
> }
>
> Except that the guard is only there to limit the lifetime and not to hold
> data.
>
> In addition, about rcu_read_lock_held(), I thought at C side, there're
>> so many comments are saying "Called within RCU critical section" but
>> without any check.
>>
>> So I wonder whether we should do some check for RCU critical section,
>> just like bql check via `assert!(bql_locked())`. Maybe we can have a
>> Rcu debug feature to cover all these checks.
>>
>
> In Rust you would just pass a &RcuGuard into the function (or store it in
> a struct) for a zero-cost assertion that you are in the RCU critical
> section.
>

Agreed. You could put debug_asserts for sanity check for good measure.

Paolo
>
>
>> Thanks,
>> Zhao
>>
>>
>>

> >> > impl AddressSpace {
> >> >    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {
> >>
> >> IIUC, this lifetime is using the "branded type" pattern as ParentInit.
> >>
> >
> > No, it's much simpler (that one uses the combination of for<'identity> and
> > PhantomData as explained in the comment). It says that the lifetime of the
> > returned reference cannot exceed the guard. It's just like
> >
> > pub fn get_item(&self, array: &'g [u8]) -> &'g u8 {
> >    &array[self.0]
> > }
> >
> > Except that the guard is only there to limit the lifetime and not to hold
> > data.

I see. It's clear for me now. Thank you!

> > In addition, about rcu_read_lock_held(), I thought at C side, there're
> >> so many comments are saying "Called within RCU critical section" but
> >> without any check.
> >>
> >> So I wonder whether we should do some check for RCU critical section,
> >> just like bql check via `assert!(bql_locked())`. Maybe we can have a
> >> Rcu debug feature to cover all these checks.
> >>
> >
> > In Rust you would just pass a &RcuGuard into the function (or store it in
> > a struct) for a zero-cost assertion that you are in the RCU critical
> > section.
> >
> 
> Agreed. You could put debug_asserts for sanity check for good measure.

Thanks!

Then I see, the most RCU critical part is accessing FlatView through
AddressSpace.

Here, require RCU by function signature (&RcuGuard) is very convenient:

pub fn get_flatview<'g>(&self, rcu: &'g RcuGuard) -> &'g FlatView;

As for the methods of FlatView itself and the lower-level interfaces
(e.g., the write & read detail methods), although RCU critical sections
are also required, there is no need to worry about them because the
upper-level access already ensures RCU lock is held. Of course, it would
be better to add &RcuGuard to some structures and check them with
debug_assert/assert.

Regards,
Zhao