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

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

On 11/13/25 15:16, Dmitry Osipenko wrote:
> 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.

KVM faults and terminates QEMU. Both KVM and TCG shouldn't use
PROT_NONE, good catch.

-- 
Best regards,
Dmitry