On Tue, 12 Aug 2025, 18:17 Zhao Liu, <[email protected]> wrote:
> On Thu, Aug 07, 2025 at 03:57:17PM +0200, Paolo Bonzini wrote:
> > Date: Thu, 7 Aug 2025 15:57:17 +0200
> > From: Paolo Bonzini <[email protected]>
> > Subject: Re: [RFC 16/26] memory: Make flatview_do_translate() return a
> > pointer to MemoryRegionSection
> >
> > On 8/7/25 14:30, Zhao Liu wrote:
> > > Rust side will use cell::Opaque<> to hide details of C structure, and
> > > this could help avoid the direct operation on C memory from Rust side.
> > >
> > > Therefore, it's necessary to wrap a translation binding and make it
> only
> > > return the pointer to MemoryRegionSection, instead of the copy.
> > >
> > > As the first step, make flatview_do_translate return a pointer to
> > > MemoryRegionSection, so that we can build a wrapper based on it.
> >
> > Independent of Rust, doing the copy as late as possible is good, but
> make it
> > return a "const MemoryRegionSection*" so that there's no risk of
> overwriting
> > data.
>
> Yes, const MemoryRegionSection* is helpful...
>
> > Hopefully this does not show a bigger problem!
>
> ...then we will get `*const bindings::MemoryRegionSection` from
> flatview_translate_section().
>
> This is mainly about how to construct Opaque<T> from `*cont T`:
>
> impl FlatView {
> fn translate(
> &self,
> addr: GuestAddress,
> len: GuestUsize,
> is_write: bool,
> ) -> Option<(&MemoryRegionSection, MemoryRegionAddress, GuestUsize)> {
> ...
> let ptr = unsafe {
> flatview_translate_section(
> self.as_mut_ptr(),
> addr.raw_value(),
> &mut raw_addr,
> &mut remain,
> is_write,
> MEMTXATTRS_UNSPECIFIED,
> )
> };
>
> ...
>
> ------> // Note here, Opaque<>::from_raw() requires *mut T.
> // And we can definitely convert *cont T to *mut T!
> let s = unsafe { <FlatView as GuestMemory>::R::from_raw(ptr as
> *mut _) };
> ...
> }
>
> But look closer to Opaque<>, it has 2 safe methods: as_mut_ptr() &
> raw_get().
>
> These 2 methods indicate that the T pointed by Qpaque<T> is mutable,
> which has the conflict with the original `*const
> bindings::MemoryRegionSection`.
>
> So from this point, it seems unsafe to use Qpaque<> on this case.
>
Yes, the usual approach is to have a Ref and a RefMut type e.g. Opaque and
OpaqueMut, and the OpaqueMut type can dereference immutably as an Opaque.
See std::cell::{Ref, RefMut} for inspiration.
To address this, I think we need:
> - rich comments about this MemoryRegionSection is actually immuatble.
> - modify other C functions to accept `const *MemoryRegionSection` as
> argument.
>
> What do you think?
>
> Thanks,
> Zhao
>
>
