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