Re: [PATCH v4] hostmem-file: add offset option

[Peter Xu <peterx@redhat.com>]

On Mon, Apr 03, 2023 at 09:13:29AM +0200, David Hildenbrand wrote:
> On 01.04.23 19:47, Stefan Hajnoczi wrote:
> > On Sat, Apr 01, 2023 at 12:42:57PM +0000, Alexander Graf wrote:
> > > Add an option for hostmem-file to start the memory object at an offset
> > > into the target file. This is useful if multiple memory objects reside
> > > inside the same target file, such as a device node.
> > > 
> > > In particular, it's useful to map guest memory directly into /dev/mem
> > > for experimentation.
> > > 
> > > Signed-off-by: Alexander Graf <graf@amazon.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
> > > 
> > > ---
> > > 
> > > v1 -> v2:
> > > 
> > >    - add qom documentation
> > >    - propagate offset into truncate, size and alignment checks
> > > 
> > > v2 -> v3:
> > > 
> > >    - failed attempt at fixing typo
> > > 
> > > v2 -> v4:
> > > 
> > >    - fix typo
> > > ---
> > >   backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > >   include/exec/memory.h   |  2 ++
> > >   include/exec/ram_addr.h |  3 ++-
> > >   qapi/qom.json           |  5 +++++
> > >   qemu-options.hx         |  6 +++++-
> > >   softmmu/memory.c        |  3 ++-
> > >   softmmu/physmem.c       | 14 ++++++++++----
> > >   7 files changed, 65 insertions(+), 8 deletions(-)
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The change itself looks good to me, but I do think some other QEMU code that
> ends up working on the RAMBlock is not prepared yet. Most probably, because
> we never ended up using fd with an offset as guest RAM.
> 
> We don't seem to be remembering that offset in the RAMBlock. First, I
> thought block->offset would be used for that, but that's just the offset in
> the ram_addr_t space. Maybe we need a new "block->fd_offset" to remember the
> offset (unless I am missing something).

I think you're right.

> 
> The real offset in the file would be required at least in two cases I can
> see (whenever we essentially end up calling mmap() on the fd again):
> 
> 1) qemu_ram_remap(): We'd have to add the file offset on top of the
> calculated offset.
> 
> 2) vhost-user: most probably whenever we set the mmap_offset. For example,
> in vhost_user_fill_set_mem_table_msg() we'd similarly have to add the
> file_offset on top of the calculated offset. vhost_user_get_mr_data() should
> most probably do that.

I had a patch to add that offset for the upcoming doublemap feature here:

https://lore.kernel.org/all/20230117220914.2062125-8-peterx@redhat.com/

But that was because doublemap wants to map the guest mem twice for other
purposes. I didn't yet notice that the code seem to be already broken if
without offset==0.

While, I _think_ we already have offset!=0 case for a ramblock, since:

        commit ed5d001916dd46ceed6d8850e453bcd7b5db2acb
        Author: Jagannathan Raman <jag.raman@oracle.com>
        Date:   Fri Jan 29 11:46:13 2021 -0500

        multi-process: setup memory manager for remote device

Where there's:

        memory_region_init_ram_from_fd(subregion, NULL,
                                       name, sysmem_info->sizes[region],
                                       RAM_SHARED, msg->fds[region],
                                       sysmem_info->offsets[region],
                                       errp);

Thanks,

-- 
Peter Xu