Re: [RFC PATCH v1 1/1] virtio-gpu: Support mapping hostmem blobs with map_fixed

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

Hello Akihiko,

On 11/13/25 09:51, Akihiko Odaki wrote:
> On 2025/11/13 11:52, Akihiko Odaki wrote:
>> On 2025/11/13 8:31, Dmitry Osipenko wrote:
>>> Support mapping virgl blobs to a fixed location of a hostmem memory
>>> region using new virglrenderer MAP_FIXED API.
>>>
>>> This new feature closes multiple problems for virtio-gpu on QEMU:
>>>
>>> - Having dedicated memory region for each mapped blob works notoriously
>>> slow due to QEMU's memory region software design built around RCU that
>>> isn't optimized for frequent removal of the regions
>>>
>>> - KVM isn't optimized for a frequent slot changes too
>>>
>>> - QEMU/KVM has a limit for a total number of created memory regions,
>>> crashing QEMU when limit is reached
>>>
>>> This patch makes virtio-gpu-gl to pre-create a single anonymous memory
>>> region covering whole hostmem area to which blobs will be mapped using
>>> the MAP_FIXED API.
>>>
>>> Not all virgl resources will support mapping at a fixed memory
>>> address. For
>>> them, we will continue to create individual nested memory sub-
>>> regions. In
>>> particular, vrend resources may not have MAP_FIXED capability.
>>>
>>> Venus and DRM native contexts will largely benefit from the MAP_FIXED
>>> feature in terms of performance and stability improvement.
>>>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> ---
>>>   hw/display/virtio-gpu-gl.c     | 37 +++++++++++++++++
>>>   hw/display/virtio-gpu-virgl.c  | 72 +++++++++++++++++++++++++++++++++-
>>>   include/hw/virtio/virtio-gpu.h |  3 ++
>>>   3 files changed, 110 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>>> index b640900fc6f1..e1481291948a 100644
>>> --- a/hw/display/virtio-gpu-gl.c
>>> +++ b/hw/display/virtio-gpu-gl.c
>>> @@ -122,6 +122,9 @@ static void
>>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>>   {
>>>       ERRP_GUARD();
>>>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>>
>> Nitpick: this order is slightly odd as VirtIOGPUBase is the base class
>> of VirtIOGPU and VirtIOGPUGL. So let's do:
>>
>> VirtIOGPUBase *b = VIRTIO_GPU_BASE(qdev); // super class of b
>> VirtIOGPU *g = VIRTIO_GPU(qdev); // base class of gl
>> VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
>>
>> Arguments are unified to qdev for consistency with other functions.
>>
>>> +    void *map;
>>>   #if HOST_BIG_ENDIAN
>>>       error_setg(errp, "virgl is not supported on bigendian platforms");
>>> @@ -152,6 +155,31 @@ static void
>>> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>>   #endif
>>>       virtio_gpu_device_realize(qdev, errp);
>>> +
>>> +    /*
>>> +     * Check whether virtio_gpu_device_realize() failed.
>>> +     */
>>> +    if (!g->ctrl_bh) {
>>
>> Instead, do:
>> if (*errp) {
>>      return;
>> }
>>
>> With this change it is clear that it checks whether
>> virtio_gpu_device_realize() failed so the comment will be unnecessary.
>>
>>> +        return;
>>> +    }
>>> +
>>> +    if (virtio_gpu_hostmem_enabled(b->conf)) {
>>> +        map = mmap(NULL, b->conf.hostmem, PROT_NONE,
> 
> I'm concerned that mapping with PROT_NONE may allow the guest crash QEMU
> by accessing the hostmem region without blobs, especially with TCG (not
> sure about KVM).
> 
> Perhaps PROT_READ | PROT_WRITE may be a safe choice. It is ugly and lets
> the guest read and write garbage to the region without blobs, but at
> least avoids crashes.

Thanks a lot for a quick and thorough review, very appreciate. Will test
how KVM behaves when accessing PROT_NONE and address TCG + rest of the
comments.

-- 
Best regards,
Dmitry