On 5/5/24 09:47, Akihiko Odaki wrote:
> On 2024/05/02 4:20, Dmitry Osipenko wrote:
>> On 4/27/24 08:52, Akihiko Odaki wrote:
>>> On 2024/04/24 19:30, Dmitry Osipenko wrote:
>>>> On 4/19/24 12:18, Akihiko Odaki wrote:
>>>>>> @@ -61,6 +61,10 @@ struct virtio_gpu_simple_resource {
>>>>>> int dmabuf_fd;
>>>>>> uint8_t *remapped;
>>>>>> + MemoryRegion *mr;
>>>>>> + bool async_unmap_completed;
>>>>>> + bool async_unmap_in_progress;
>>>>>> +
>>>>>
>>>>> Don't add fields to virtio_gpu_simple_resource but instead create a
>>>>> struct that embeds virtio_gpu_simple_resource in virtio-gpu-virgl.c.
>>>>
>>>> Please give a justification. I'd rather rename
>>>> virtio_gpu_simple_resource s/_simple//. Simple resource already
>>>> supports
>>>> blob and the added fields are directly related to the blob. Don't see
>>>> why another struct is needed.
>>>>
>>>
>>> Because mapping is only implemented in virtio-gpu-gl while blob itself
>>> is implemented also in virtio-gpu.
>>
>> Rutubaga maps blobs and it should do unmapping blobs asynchronously as
>> well, AFAICT.
>>
>
> Right. It makes sense to put mr in struct virtio_gpu_simple_resource in
> preparation for such a situation.
>
> Based on this discussion, I think it is fine to put mr either in struct
> virtio_gpu_simple_resource or a distinct struct. However if you put mr
> in struct virtio_gpu_simple_resource, the logic that manages
> MemoryRegion should also be moved to virtio-gpu.c for consistency.
Rutabaga uses static MRs. It will either need a different workaround or
will have to move to dynamic MRs. I'll keep using distinct struct for now.
AFAICT, its a lesser problem for rutabaga because static MR isn't
subjected to the dynamic MR UAF problem that virgl has. On the other
hand, rutabaga is re-initing already inited static MR object on each new
mapping, that looks like a bug and it will need to move to dynamic MRs.
--
Best regards,
Dmitry
