[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4] hostmem-file: add offset option
From: |
Peter Xu |
Subject: |
Re: [PATCH v4] hostmem-file: add offset option |
Date: |
Mon, 3 Apr 2023 11:49:28 -0400 |
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