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

[David Hildenbrand]

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

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.

--
Thanks,

David / dhildenb