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

[David Hildenbrand <david@redhat.com>]

On 03.04.23 17:49, Peter Xu wrote:
> 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);

Interesting ... maybe so far never used alongside vhost-user.

-- 
Thanks,

David / dhildenb