Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request

[David Hildenbrand <david@redhat.com>]

On 27.11.24 13:31, Albert Esteve wrote:
> On Wed, Nov 27, 2024 at 1:18 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 27.11.24 13:10, David Hildenbrand wrote:
>>> On 27.11.24 11:50, David Hildenbrand wrote:
>>>>
>>>>>> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
>>>>>> and map whatever you want in there.
>>>>>>
>>>>>> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
>>>>>> and would end up mmaping implicitly via qemu_ram_mmap().
>>>>>>
>>>>>> Then, your shared region would simply be an empty container into which
>>>>>> you map these RAM memory regions.
>>>>>
>>>>
>>>> Hi,
>>>>
>>>>> Hi, sorry it took me so long to get back to this. Lately I have been
>>>>> testing the patch and fixing bugs, and I am was going to add some
>>>>> tests to be able to verify the patch without having to use a backend
>>>>> (which is what I am doing right now).
>>>>>
>>>>> But I wanted to address/discuss this comment. I am not sure of the
>>>>> actual problem with the current approach (I am not completely aware of
>>>>> the concern in your first paragraph), but I see other instances where
>>>>> qemu mmaps stuff into a MemoryRegion.
>>>>
>>>> I suggest you take a look at the three relevant MAP_FIXED users outside
>>>> of user emulation code.
>>>>
>>>> (1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
>>>>         memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
>>>>         into any existing RAMBlock.
>>>>
>>>> (2) system/physmem.c: I suggest you take a close look at
>>>>         qemu_ram_remap() and how it is one example of how RAMBlock
>>>>         properties describe exactly what is mmaped.
>>>>
>>>> (3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
>>>>         to bring a RAMBlock to life. See qemu_ram_mmap().
>>>>
>>>> There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
>>>> guest RAM without RAMBlocks.
>>>>
>>>>> Take into account that the
>>>>> implementation follows the definition of shared memory region here:
>>>>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
>>>>> Which hints to a memory region per ID, not one per required map. So
>>>>> the current strategy seems to fit it better.
>>>>
>>>> I'm confused, we are talking about an implementation detail here? How is
>>>> that related to the spec?
>>>>
>>>>>
>>>>> Also, I was aware that I was not the first one attempting this, so I
>>>>> based this code in previous attempts (maybe I should give credit in
>>>>> the commit now that I think of):
>>>>> https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
>>>>> As you can see, it pretty much follows the same strategy.
>>>>
>>>> So, people did some hacky things in a QEMU fork 6 years ago ... :) That
>>>> cannot possibly be a good argument why we should have it like that in QEMU.
>>>>
>>>>> And in my
>>>>> examples I have been able to use this to video stream with multiple
>>>>> queues mapped into the shared memory (used to capture video frames),
>>>>> using the backend I mentioned above for testing. So the concept works.
>>>>> I may be wrong with this, but for what I understood looking at the
>>>>> code, crosvm uses a similar strategy. Reserve a memory block and use
>>>>> for all your mappings, and use an allocator to find a free slot.
>>>>>
>>>>
>>>> Again, I suggest you take a look at what a RAMBlock is, and how it's
>>>> properties describe the containing mmap().
>>>>
>>>>> And if I were to do what you say, those distinct RAMBlocks should be
>>>>> created when the device starts? What would be their size? Should I
>>>>> create them when qemu receives a request to mmap? How would the driver
>>>>> find the RAMBlock?
>>>>
>>>> You'd have an empty memory region container into which you will map
>>>> memory regions that describe the memory you want to share.
>>>>
>>>> mr = g_new0(MemoryRegion, 1);
>>>> memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
>>>>
>>>>
>>>> Assuming you are requested to mmap an fd, you'd create a new
>>>> MemoryRegion+RAMBlock that describes the memory and performs the mmap()
>>>> for you:
>>>>
>>>> map_mr = g_new0(MemoryRegion, 1);
>>>> memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
>>>>                              RAM_SHARED, map_fd, map_offs, errp);
>>>>
>>>> To then map it into your container:
>>>>
>>>> memory_region_add_subregion(mr, offset_within_container, map_mr);
>>>>
>>>>
>>>> To unmap, you'd first remove the subregion, to then unref the map_mr.
>>>>
>>>>
>>>>
>>>> The only alternative would be to do it like (1) above: you perform all
>>>> of the mmap() yourself, and create it using
>>>> memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
>>>> RAMBlock and tell QEMU "this is special, just disregard it". The bad
>>>> thing about RAM_PREALLOC will be that it will not be compatible with
>>>> vfio, not communicated to other vhost-user devices etc ... whereby what
>>>> I describe above would just work with them.
>>>>
>>>
>>> FWIW, I took another look at this patch and I cannot really make sense
>>> of what you are doing.
>>>
>>> In virtio_new_shmem_region(), you allocate a region, but I don't see how
>>> it would ever get initialized?
>>>
>>> In vhost_user_backend_handle_shmem_map(), you then assume that it is
>>> suddenly a RAM memory region? For example, that
>>> memory_region_get_ram_ptr() returns something reasonable.
>>>
>>> Likely, you really just want to initialize that MR using
>>> memory_region_init_ram_from_fd(), and have that just perform the proper
>>> mmap() for you and setup the RAMBlock?
>>
>> In patch #4 I find: memory_region_init_ram_ptr(vdev->shmem_list[i].mr ...
>>
>> Which does what I describe as the alternative. That makes it clearer
>> that you are not operating on arbitrary RAMBlocks. So that should indeed
>> work.
>>
>> The way you structured these patches really is suboptimal for review.
> 
> Firstly, thank you so much for taking another look at the patch after
> so much time. I understand it may be challenging given the time it
> passed since I sent this version. Also, I rushed this version trying
> to get to settle the spec first while I was working on something else,
> so the code was untested and has quite a few bugs. I am mentioning it
> just to note that I think this patch should not get a fine grain
> review in its current status (although Stefan kindly did review it). I
> fixed all (at least most) of the bugs in what would be the next
> version, and it is what I use for testing. When I say I proved this
> code and works, I am mostly referring to my local changes. As it is
> here, this won't work.
> 
> That said, I understand what you mean, I will try to refactor /
> reorder the patch to make it easier to review.

If we could make it clearer that we are operating on RAM_PREALLOC 
MRs/RAMBlocks in when handling shmem_map/shmem_unmap requests somehow, 
that would already take quite some magic out of this code. :)

Having that said, I now understand why you want RAM_PREALLOC, because 
you might only activate/deactivate some parts within a RAMBlock.

-- 
Cheers,

David / dhildenb