Re: [RFC 16/26] memory: Make flatview_do_translate() return a pointer to MemoryRegionSection

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

On Tue, 12 Aug 2025, 18:17 Zhao Liu, <zhao1.liu@intel.com> 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 <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.
>

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
>
>

[-- Attachment #2: Type: text/html, Size: 4084 bytes --]