On Thu, Aug 07, 2025 at 03:50:45PM +0200, Paolo Bonzini wrote: > Date: Thu, 7 Aug 2025 15:50:45 +0200 > From: Paolo Bonzini <pbonz...@redhat.com> > Subject: Re: [RFC 24/26] rust/memory: Provide AddressSpace bindings > > 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.
Yes. Here address_space_get_flatview() is better. > > + } > > +} > > + > > +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. Ah, then rcu just ensures &FlatView is valid since we increments its ref count during rcu critical section. But rcu will no longer cover the entire write process! Combining this RcuGuard proposal in the reply of patch 13: https://lore.kernel.org/qemu-devel/ajsx9hh%2fjwblz...@intel.com/ 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() } } rcu is dropped at the end of memory(). So `&'g RcuGuard` is not enough for this case. > > + 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**. Sure, and for HPET, the error isn't be handled except panic... > > + /// 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 Yes, this is the ideal way. This will involve the changes in both vm-memory and QEMU: * vm-memory: we need to implement AtomicAccess trait for BeNN and LeNN in vm-memory (but this is not a big deal). * For QEMU, Now to handle AtomicAccess, I've abstracted a uniform C store() binding in patch 21: MemTxResult section_rust_store(MemoryRegionSection *section, hwaddr mr_offset, const uint8_t *buf, MemTxAttrs attrs, hwaddr len); If you haven't looked at this, you can see the comments in: impl Bytes<MemoryRegionAddress> for MemoryRegionSection { fn store<T: AtomicAccess>( &self, val: T, addr: MemoryRegionAddress, _order: Ordering, ) -> GuestMemoryResult<()> {} } section_rust_store() supports target endian by default like address_space_st(). If we wants to add LE & BE support, I think we have 2 options: 1) Add another endian argument in section_rust_store, but this also requires to pass endian informantion in Bytes trait. Ethier we need to implement Bytes<(MemoryRegionAddress, DeviceEndiann)>, or we need to add endian info in AtomicAccess. 2) simplify section_rust_store() further: ignore endian stuff and just store the data from *buf to MMIO/ram. For this way, we need to adjust adjust_endianness() to do nothing: section_rust_store() -> memory_region_dispatch_write() -> adjust_endianness() However, adjust_endianness() is still very useful, especially for QEMU, the caller of store() doesn't know the access is for MMIO or RAM. So I prefer 1) for now, and maybe it's better to add endian info in AtomicAccess. > 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); Yes, this way is similar with my previous attempt...but I don't know what's the best to handler LE/BE, so this RFC just omitted these cases, and chose a simplest case - native endian. > (Also I've not looked at the patches closely enough, but wouldn't store() > use *host* endianness? Same in patch 23). It seems QEMU's interfaces don't use *host* endianness? I'm referring address_space_ld*() & address_space_st*(), and their doc said: /* address_space_ld*: load from an address space * address_space_st*: store to an address space * * These functions perform a load or store of the byte, word, * longword or quad to the specified address within the AddressSpace. * The _le suffixed functions treat the data as little endian; * _be indicates big endian; no suffix indicates "same endianness * as guest CPU". * * The "guest CPU endianness" accessors are deprecated for use outside * target-* code; devices should be CPU-agnostic and use either the LE * or the BE accessors. */ I also considerred host endianness. But host endianness doesn't align with C side... C side only supports little/big/native (target/guest) endianness. So, do you think Rust side should consider host endianness? Or maybe we can add DEVICE_HOST_ENDIAN in device_endian. But is there the way to check what the specific endianness of Host is? Thanks, Zhao