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 <pbonzini@redhat.com>
> 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.

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