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.

In addtion, add a global variable `unassigned_section` to help get a
pointer to an invalid MemoryRegionSection.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 system/physmem.c | 51 ++++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 785c9a4050c6..4af29ea2168e 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -103,6 +103,9 @@ AddressSpace address_space_io;
 AddressSpace address_space_memory;
 
 static MemoryRegion io_mem_unassigned;
+static MemoryRegionSection unassigned_section = {
+    .mr = &io_mem_unassigned
+};
 
 typedef struct PhysPageEntry PhysPageEntry;
 
@@ -418,14 +421,11 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
  * This function is called from RCU critical section.  It is the common
  * part of flatview_do_translate and address_space_translate_cached.
  */
-static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iommu_mr,
-                                                         hwaddr *xlat,
-                                                         hwaddr *plen_out,
-                                                         hwaddr *page_mask_out,
-                                                         bool is_write,
-                                                         bool is_mmio,
-                                                         AddressSpace **target_as,
-                                                         MemTxAttrs attrs)
+static MemoryRegionSection *
+address_space_translate_iommu(IOMMUMemoryRegion *iommu_mr, hwaddr *xlat,
+                              hwaddr *plen_out, hwaddr *page_mask_out,
+                              bool is_write, bool is_mmio,
+                              AddressSpace **target_as, MemTxAttrs attrs)
 {
     MemoryRegionSection *section;
     hwaddr page_mask = (hwaddr)-1;
@@ -463,10 +463,10 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm
     if (page_mask_out) {
         *page_mask_out = page_mask;
     }
-    return *section;
+    return section;
 
 unassigned:
-    return (MemoryRegionSection) { .mr = &io_mem_unassigned };
+    return &unassigned_section;
 }
 
 /**
@@ -489,15 +489,10 @@ unassigned:
  *
  * This function is called from RCU critical section
  */
-static MemoryRegionSection flatview_do_translate(FlatView *fv,
-                                                 hwaddr addr,
-                                                 hwaddr *xlat,
-                                                 hwaddr *plen_out,
-                                                 hwaddr *page_mask_out,
-                                                 bool is_write,
-                                                 bool is_mmio,
-                                                 AddressSpace **target_as,
-                                                 MemTxAttrs attrs)
+static MemoryRegionSection *
+flatview_do_translate(FlatView *fv, hwaddr addr, hwaddr *xlat, hwaddr *plen_out,
+                      hwaddr *page_mask_out, bool is_write, bool is_mmio,
+                      AddressSpace **target_as, MemTxAttrs attrs)
 {
     MemoryRegionSection *section;
     IOMMUMemoryRegion *iommu_mr;
@@ -523,14 +518,14 @@ static MemoryRegionSection flatview_do_translate(FlatView *fv,
         *page_mask_out = ~TARGET_PAGE_MASK;
     }
 
-    return *section;
+    return section;
 }
 
 /* Called from RCU critical section */
 IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
                                             bool is_write, MemTxAttrs attrs)
 {
-    MemoryRegionSection section;
+    MemoryRegionSection *section;
     hwaddr xlat, page_mask;
 
     /*
@@ -542,13 +537,13 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
                                     attrs);
 
     /* Illegal translation */
-    if (section.mr == &io_mem_unassigned) {
+    if (section->mr == &io_mem_unassigned) {
         goto iotlb_fail;
     }
 
     /* Convert memory region offset into address space offset */
-    xlat += section.offset_within_address_space -
-        section.offset_within_region;
+    xlat += section->offset_within_address_space -
+        section->offset_within_region;
 
     return (IOMMUTLBEntry) {
         .target_as = as,
@@ -569,13 +564,13 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr *xlat,
                                  MemTxAttrs attrs)
 {
     MemoryRegion *mr;
-    MemoryRegionSection section;
+    MemoryRegionSection *section;
     AddressSpace *as = NULL;
 
     /* This can be MMIO, so setup MMIO bit. */
     section = flatview_do_translate(fv, addr, xlat, plen, NULL,
                                     is_write, true, &as, attrs);
-    mr = section.mr;
+    mr = section->mr;
 
     if (xen_enabled() && memory_access_is_direct(mr, is_write, attrs)) {
         hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
@@ -3618,7 +3613,7 @@ static inline MemoryRegion *address_space_translate_cached(
     MemoryRegionCache *cache, hwaddr addr, hwaddr *xlat,
     hwaddr *plen, bool is_write, MemTxAttrs attrs)
 {
-    MemoryRegionSection section;
+    MemoryRegionSection *section;
     MemoryRegion *mr;
     IOMMUMemoryRegion *iommu_mr;
     AddressSpace *target_as;
@@ -3636,7 +3631,7 @@ static inline MemoryRegion *address_space_translate_cached(
     section = address_space_translate_iommu(iommu_mr, xlat, plen,
                                             NULL, is_write, true,
                                             &target_as, attrs);
-    return section.mr;
+    return section->mr;
 }
 
 /* Called within RCU critical section.  */
-- 
2.34.1

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.  Hopefully this does not show a bigger problem!

Paolo

> In addtion, add a global variable `unassigned_section` to help get a
> pointer to an invalid MemoryRegionSection.

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

Il mar 12 ago 2025, 17:17 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> 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, it's similar to NonNull<>. I am not sure that you need Opaque<> here;
since the pointer is const, maybe you can just dereference it to a
&bindings::MemoryRegionSection. Is it useful to have the Opaque<> wrapper
here?

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

Yes, adding const is useful in this case.

Paolo

What do you think?
>
> Thanks,
> Zhao
>
>

On Tue, Aug 12, 2025 at 09:23:59PM +0200, Paolo Bonzini wrote:
> Date: Tue, 12 Aug 2025 21:23:59 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 16/26] memory: Make flatview_do_translate() return a
>  pointer to MemoryRegionSection
> 
> Il mar 12 ago 2025, 17:17 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> 
> > 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, it's similar to NonNull<>. I am not sure that you need Opaque<> here;
> since the pointer is const, maybe you can just dereference it to a
> &bindings::MemoryRegionSection. Is it useful to have the Opaque<> wrapper
> here?

I agree. Opaque<> is not necessary here. We can have a simple wrapper:

pub struct MemoryRegionSection(*const bindings::MemoryRegionSection)

or

pub struct MemoryRegionSection(NonNull<bindings::MemoryRegionSection>)
with immutable only use.

In future, if there're more similar case, then we can have a OpaqueRef<>
like Manos suggested.

Thanks,
Zhao

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

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

Thanks! I'll dorp Opaque directly for this case. If there're more similar
cases, then we can have a OpaqueRef<>.

Regards,
Zhao