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

[Alexander Graf <graf@amazon.com>]

On 03.04.23 09:13, 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).
>
> 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.


This one is a bit tricky to test, as we're only running into that code 
path with KVM when we see an #MCE. But it's trivial, so I'm confident it 
will work as expected.


>
> 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 agree - adding the offset as part of get_mr_data() is sufficient. I 
have validated it works correctly with QEMU's vhost-user-blk target.

I think the changes are still obvious enough that I'll fold them all 
into a single patch.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879