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 <[email protected]> >>>> --- >>>> 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
