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. -- Best regards, Dmitry
