Re: [RFC 10/26] subprojects/vm-memory: Patch vm-memory for QEMU memory backend

[Zhao Liu <zhao1.liu@intel.com>]

On Fri, Aug 08, 2025 at 10:17:51AM +0200, Paolo Bonzini wrote:
> Date: Fri, 8 Aug 2025 10:17:51 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 10/26] subprojects/vm-memory: Patch vm-memory for QEMU
>  memory backend
> 
> On 8/8/25 10:17, Zhao Liu wrote:
> > (+Hanna: I would like to align with Hanna on 0002.diff patch :-))
> > 
> > On Thu, Aug 07, 2025 at 03:59:26PM +0200, Paolo Bonzini wrote:
> > > Date: Thu, 7 Aug 2025 15:59:26 +0200
> > > From: Paolo Bonzini <pbonzini@redhat.com>
> > > Subject: Re: [RFC 10/26] subprojects/vm-memory: Patch vm-memory for QEMU
> > >   memory backend
> > > 
> > > On 8/7/25 14:30, Zhao Liu wrote:
> > > > Add 2 patches to support QEMU memory backend implementation.
> > > > 
> > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > ---
> > > >    .../packagefiles/vm-memory-0.16-rs/0001.diff  |  81 +++++++++++++
> > > >    .../packagefiles/vm-memory-0.16-rs/0002.diff  | 111 ++++++++++++++++++
> > > >    subprojects/vm-memory-0.16-rs.wrap            |   2 +
> > > >    3 files changed, 194 insertions(+)
> > > >    create mode 100644 subprojects/packagefiles/vm-memory-0.16-rs/0001.diff
> > > >    create mode 100644 subprojects/packagefiles/vm-memory-0.16-rs/0002.diff
> > > > 
> > > > diff --git a/subprojects/packagefiles/vm-memory-0.16-rs/0001.diff b/subprojects/packagefiles/vm-memory-0.16-rs/0001.diff
> > > > new file mode 100644
> > > > index 000000000000..037193108d45
> > > > --- /dev/null
> > > > +++ b/subprojects/packagefiles/vm-memory-0.16-rs/0001.diff
> > > > @@ -0,0 +1,81 @@
> > > > +From 298f8ba019b2fe159fa943e0ae4dfd3c83ee64e0 Mon Sep 17 00:00:00 2001
> > > > +From: Zhao Liu <zhao1.liu@intel.com>
> > > > +Date: Wed, 6 Aug 2025 11:31:11 +0800
> > > > +Subject: [PATCH 1/2] guest_memory: Add a marker tarit to implement
> > > > + Bytes<GuestAddress> for GuestMemory
> > > 
> > > This was a bit surprising.  Maybe this is something where GuestMemory needs
> > > some extra flexibility.
> > 
> > At least, the default GuestMemory::try_access() need to re-implement in
> > QEMU, and this is because GuestMemory::iter() doesn't fit for QEMU's
> > case, and GuestMemory::to_region_addr() also needs adjustment to support
> > complete translation.
> > 
> > For details,
> > 
> > 1) iter() - QEMU has implemented the two-level "page" walk in
> >     `phys_page_find`, which is more efficient than linear iteration.
> > 
> > 2) to_region_addr() - it's function signature is:
> > 
> >      fn to_region_addr(
> >          &self,
> > 	addr: GuestAddress
> >      ) -> Option<(&Self::R, MemoryRegionAddress)>;
> > 
> > but QEMU currentlt wants:
> > 
> >      fn translate(
> >          &self,
> >          addr: GuestAddress,
> >          len: GuestUsize,
> >          is_write: bool,
> >      ) -> Option<(&MemoryRegionSection, MemoryRegionAddress, GuestUsize)>
> > 
> > `is_write` is mainly about IOMMU (and read-only case, but that could be
> > workaround I think).
> > 
> > And the 3rd member `GuestUsize` of (&MemoryRegionSection,
> > MemoryRegionAddress, GuestUsize) indicates the remianing size, which is
> > used to detect cross-region case. Maybe this `GuestUsize` is not
> > necessary in its return, since we can check the size of `MemoryRegionSection`
> > later. But this would be a bit repetitive.
> > 
> > But at least, this marker trait is acceptable, right? :-)
> > 
> > The marker trait for GuestMemoryRegion is introduced at commit 66ff347
> > ("refactor: use matches! instead of to_string() for tests").
> > 
> > > > @@ -0,0 +1,111 @@
> > > > +From 2af7ea12a589fde619690e5060c01710cb6f2e0e Mon Sep 17 00:00:00 2001
> > > > +From: Zhao Liu <zhao1.liu@intel.com>
> > > > +Date: Wed, 6 Aug 2025 14:27:14 +0800
> > > > +Subject: [PATCH 2/2] guest_memory: Add is_write argument for
> > > > + GuestMemory::try_access()
> > > 
> > > This should be fine.  But Hanna is also working on IOMMU so maybe this won't
> > > be needed!
> > 
> > I'm not sure what method could align with Hanna's design. If there's
> > another interface/method, I can have a try.
> 
> For example she already has similar fixes in
> https://github.com/rust-vmm/vm-memory/pull/327:
> 
> https://github.com/rust-vmm/vm-memory/pull/327/commits/9bcd5ac9b9ae37d1fb421f86f0aff310411933af
>    Bytes: Fix read() and write()
> 
>    read() and write() must not ignore the `count` parameter: The
>    mappings passed into the `try_access()` closure are only valid for up
>    to `count` bytes, not more.
> 
> https://github.com/rust-vmm/vm-memory/pull/327/commits/2b83c72be656e5d46b83cb3a66d580e56cf33d5b
>     Bytes: Do not use to_region_addr()
> 
>     When we switch to a (potentially) virtual memory model [...]
>     the one memory-region-referencing part we are going to keep is
>     `try_access()` [...] switch `Bytes::load()` and `store()` from using
>     `to_region_addr()` to `try_access()`.
> 
> With some luck, your custom implementation of Bytes<GuestAddress> is not
> needed once vm-memory supports iommu.

Nice! I took a quick look, and these patches seem to be shaped as what
this RFC wants. I'll give them a try.

Thanks,
Zhao