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

[David Hildenbrand]

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