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

[David Hildenbrand <david@redhat.com>]

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

Indeed.

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

Most probably good enough. Having the offset part separately as a fix 
for ed5d001916 ("multi-process: setup memory manager for remote device") 
could be beneficial, though.

Thanks Alex!

-- 
Thanks,

David / dhildenb