Re: [RFC 24/26] rust/memory: Provide AddressSpace bindings

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/7/25 14:30, Zhao Liu wrote:
> +impl GuestAddressSpace for AddressSpace {
> +    type M = FlatView;
> +    type T = FlatViewRefGuard;
> +
> +    /// Get the memory of the [`AddressSpace`].
> +    ///
> +    /// This function retrieves the [`FlatView`] for the current
> +    /// [`AddressSpace`].  And it should be called from an RCU
> +    /// critical section.  The returned [`FlatView`] is used for
> +    /// short-term memory access.
> +    ///
> +    /// Note, this function method may **panic** if [`FlatView`] is
> +    /// being distroying.  Fo this case, we should consider to providing
> +    /// the more stable binding with [`bindings::address_space_get_flatview`].
> +    fn memory(&self) -> Self::T {
> +        let flatp = unsafe { address_space_to_flatview(self.0.as_mut_ptr()) };
> +        FlatViewRefGuard::new(unsafe { Self::M::from_raw(flatp) }).expect(
> +            "Failed to clone FlatViewRefGuard: the FlatView may have been destroyed concurrently.",
> +        )

This is essentially address_space_get_flatview().  You can call it 
directly, or you need to loop if FlatViewRefGuard finds a zero reference 
count.

> +    }
> +}
> +
> +impl AddressSpace {
> +    /// The write interface of `AddressSpace`.
> +    ///
> +    /// This function is similar to `address_space_write` in C side.
> +    ///
> +    /// But it assumes the memory attributes is MEMTXATTRS_UNSPECIFIED.
> +    pub fn write(&self, buf: &[u8], addr: GuestAddress) -> Result<usize> {
> +        rcu_read_lock();
> +        let r = self.memory().deref().write(buf, addr);
> +        rcu_read_unlock();

self.memory() must not need rcu_read_lock/unlock around it, they should 
be called by the memory() function itself.

> +        r.map_err(guest_mem_err_to_qemu_err)
> +    }

I think it's ok to return the vm-memory error.  Ultimately, the error 
will be either ignored or turned into a device error condition, but I 
don't think it's ever going to become an Error**.

> +    /// The store interface of `AddressSpace`.
> +    ///
> +    /// This function is similar to `address_space_st{size}` in C side.
> +    ///
> +    /// But it only assumes @val follows target-endian by default. So ensure
> +    /// the endian of `val` aligned with target, before using this method.

QEMU is trying to get rid of target endianness.  We should use the 
vm-memory BeNN and LeNN as much as possible.  It would be great if you 
could write either

     ADDRESS_SPACE_MEMORY.store::<Le32>(addr, 42);

or

     let n = Le32(42);
     ADDRESS_SPACE_MEMORY.store(addr, n);

but not

     ADDRESS_SPACE_MEMORY.store(addr, 42);

(Also I've not looked at the patches closely enough, but wouldn't 
store() use *host* endianness? Same in patch 23).

Paolo

> +    /// And it assumes the memory attributes is MEMTXATTRS_UNSPECIFIED.
> +    pub fn store<T: AtomicAccess>(&self, addr: GuestAddress, val: T) -> Result<()> {
> +        rcu_read_lock();
> +        let r = self.memory().deref().store(val, addr, Ordering::Relaxed);
> +        rcu_read_unlock();
> +        r.map_err(guest_mem_err_to_qemu_err)
> +    }
> +
> +    /// The load interface of `AddressSpace`.
> +    ///
> +    /// This function is similar to `address_space_ld{size}` in C side.
> +    ///
> +    /// But it only support target-endian by default.  The returned value is
> +    /// with target-endian.
> +    ///
> +    /// And it assumes the memory attributes is MEMTXATTRS_UNSPECIFIED.
> +    pub fn load<T: AtomicAccess>(&self, addr: GuestAddress) -> Result<T> {
> +        rcu_read_lock();
> +        let r = self.memory().deref().load(addr, Ordering::Relaxed);
> +        rcu_read_unlock();
> +        r.map_err(guest_mem_err_to_qemu_err)
> +    }
> +}
> +
> +/// The safe binding around [`bindings::address_space_memory`].
> +///
> +/// `ADDRESS_SPACE_MEMORY` provides the complete address space
> +/// abstraction for the whole Guest memory.
> +pub static ADDRESS_SPACE_MEMORY: &AddressSpace = unsafe {
> +    let ptr: *const bindings::AddressSpace = addr_of!(address_space_memory);
> +
> +    // SAFETY: AddressSpace is #[repr(transparent)].
> +    let wrapper_ptr: *const AddressSpace = ptr.cast();
> +
> +    // SAFETY: `address_space_memory` structure is valid in C side during
> +    // the whole QEMU life.
> +    &*wrapper_ptr
> +};