Re: [PATCH v16 00/13] Support blob memory and venus on qemu

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 01, 2024 at 02:48:58PM +0400, Marc-André Lureau wrote:
> Hi,
> 
> All R-b now, it looks good to merge. Thanks for all the effort! 
> 
> Michael, are you taking it through your tree?


I could but did you intend to give your Reviewed-by?


> On Sun, Jun 23, 2024 at 7:23 PM Dmitry Osipenko <dmitry.osipenko@collabora.com>
> wrote:
> 
>     Hello,
> 
>     This series enables Vulkan Venus context support on virtio-gpu.
> 
>     All virglrender and almost all Linux kernel prerequisite changes
>     needed by Venus are already in upstream. For kernel there is a pending
>     KVM patchset that fixes mapping of compound pages needed for DRM drivers
>     using TTM [1], othewrwise hostmem blob mapping will fail with a KVM error
>     from Qemu.
> 
>     [1] https://lore.kernel.org/kvm/
>     20240229025759.1187910-1-stevensd@google.com/
> 
>     You'll need to use recent Mesa version containing patch that removes
>     dependency on cross-device feature from Venus that isn't supported by
>     Qemu [2].
> 
>     [2] https://gitlab.freedesktop.org/mesa/mesa/-/commit/
>     087e9a96d13155e26987befae78b6ccbb7ae242b
> 
>     Example Qemu cmdline that enables Venus:
> 
>       qemu-system-x86_64 -device virtio-vga-gl,hostmem=4G,blob=true,venus=true
>     \
>           -machine q35,accel=kvm,memory-backend=mem1 \
>           -object memory-backend-memfd,id=mem1,size=8G -m 8G
> 
> 
>     Changes from V15 to V16
> 
>     - Fixed resource_get_info() change made in v15 that was squshed into a
>       wrong patch. Squashed set_scanout_blob patch into the blob-commands
>     patch,
>       otherwise we'll need to revert back ordering of blob patches to older v7
>       variant.
> 
>     - Changed hostmem mapping state to boolean, suggested by Akihiko Odaki.
> 
>     - Documented Venus in virtio-gpu doc. Amended "Support Venus context" patch
>       with the doc addition. Suggested by Akihiko Odaki.
> 
>     Changes from V14 to V15
> 
>     - Dropped hostmem mapping state that got unused in v14, suggested by
>       Akihiko Odaki.
> 
>     - Moved resource_get_info() from set_scanout_blob() to create_blob(),
>       suggested by Akihiko Odaki.
> 
>     - Fixed unitilized variable in create_blob(), spotted by Alex Bennée.
> 
>     Changes from V13 to V14
> 
>     - Fixed erronous fall-through in renderer_state's switch-case that was
>       spotted by Marc-André Lureau.
> 
>     - Reworked HOSTMEM_MR_FINISH_UNMAPPING handling as was suggested by
>       Akihiko Odaki. Now it shares the same code path with HOSTMEM_MR_MAPPED.
> 
>     - Made use of g_autofree in virgl_cmd_resource_create_blob() as was
>       suggested by Akihiko Odaki.
> 
>     - Removed virtio_gpu_virgl_deinit() and moved all deinit code to
>       virtio_gpu_gl_device_unrealize() as was suggested by Marc-André Lureau.
> 
>     - Replaced HAVE_FEATURE in mseon.build with virglrenderer's VERSION_MAJOR
>       check as was suggested by Marc-André Lureau.
> 
>     - Added trace event for cmd-suspension as was suggested by Marc-André
>     Lureau.
> 
>     - Added patch to replace in-flight printf's with trace events as was
>       suggested by Marc-André Lureau
> 
>     Changes from V12 to V13
> 
>     - Replaced `res->async_unmap_in_progress` flag with a mapping state,
>       moved it to the virtio_gpu_virgl_hostmem_region like was suggested
>       by Akihiko Odaki.
> 
>     - Renamed blob_unmap function and added back cmd_suspended argument
>       to it. Suggested by Akihiko Odaki.
> 
>     - Reordered VirtIOGPUGL refactoring patches to minimize code changes
>       like was suggested by Akihiko Odaki.
> 
>     - Replaced gl->renderer_inited with gl->renderer_state, like was suggested
>       by Alex Bennée.
> 
>     - Added gl->renderer state resetting to gl_device_unrealize(), for
>       consistency. Suggested by Alex Bennée.
> 
>     - Added rb's from Alex and Manos.
> 
>     - Fixed compiling with !HAVE_VIRGL_RESOURCE_BLOB.
> 
>     Changes from V11 to V12
> 
>     - Fixed virgl_cmd_resource_create_blob() error handling. Now it doesn't
>       corrupt resource list and releases resource properly on error. Thanks
>       to Akihiko Odaki for spotting the bug.
> 
>     - Added new patch that handles virtio_gpu_virgl_init() failure gracefully,
>       fixing Qemu crash. Besides fixing the crash, it allows to implement
>       a cleaner virtio_gpu_virgl_deinit().
> 
>     - virtio_gpu_virgl_deinit() now assumes that previously virgl was
>       initialized successfully when it was inited at all. Suggested by
>       Akihiko Odaki.
> 
>     - Fixed missed freeing of print_stats timer in virtio_gpu_virgl_deinit()
> 
>     - Added back blob unmapping or RESOURCE_UNREF that was requested
>       by Akihiko Odaki. Added comment to the code explaining how
>       async unmapping works. Added back `res->async_unmap_in_progress`
>       flag and added comment telling why it's needed.
> 
>     - Moved cmdq_resume_bh to VirtIOGPUGL and made coding style changes
>       suggested by Akihiko Odaki.
> 
>     - Added patches that move fence_poll and print_stats timers to VirtIOGPUGL
>       for consistency with cmdq_resume_bh.
> 
>     Changes from V10 to V11
> 
>     - Replaced cmd_resume bool in struct ctrl_command with
>       "cmd->finished + !VIRTIO_GPU_FLAG_FENCE" checking as was requested
>       by Akihiko Odaki.
> 
>     - Reworked virgl_cmd_resource_unmap/unref_blob() to avoid re-adding
>       the 'async_unmap_in_progress' flag that was dropped in v9:
> 
>         1. virgl_cmd_resource_[un]map_blob() now doesn't check itself whether
>            resource was previously mapped and lets virglrenderer to do the
>            checking.
> 
>         2. error returned by virgl_renderer_resource_unmap() is now handled
>            and reported properly, previously the error wasn't checked. The
>            virgl_renderer_resource_unmap() fails if resource wasn't mapped.
> 
>         3. virgl_cmd_resource_unref_blob() now doesn't allow to unref resource
>            that is mapped, it's a error condition if guest didn't unmap
>     resource
>            before doing the unref. Previously unref was implicitly unmapping
>            resource.
> 
>     Changes from V9 to V10
> 
>     - Dropped 'async_unmap_in_progress' variable and switched to use
>       aio_bh_new() isntead of oneshot variant in the "blob commands" patch.
> 
>     - Further improved error messages by printing error code when actual error
>       occurrs and using ERR_UNSPEC instead of ERR_ENOMEM when we don't really
>       know if it was ENOMEM for sure.
> 
>     - Added vdc->unrealize for the virtio GL device and freed virgl data.
> 
>     - Dropped UUID and doc/migration patches. UUID feature isn't needed
>       anymore, instead we changed Mesa Venus driver to not require UUID.
> 
>     - Renamed virtio-gpu-gl "vulkan" property name back to "venus".
> 
>     Changes from V8 to V9
> 
>     - Added resuming of cmdq processing when hostmem MR is freed,
>       as was suggested by Akihiko Odaki.
> 
>     - Added more error messages, suggested by Akihiko Odaki
> 
>     - Dropped superfluous 'res->async_unmap_completed', suggested
>       by Akihiko Odaki.
> 
>     - Kept using cmd->suspended flag. Akihiko Odaki suggested to make
>       virtio_gpu_virgl_process_cmd() return false if cmd processing is
>       suspended, but it's not easy to implement due to ubiquitous
>       VIRTIO_GPU_FILL_CMD() macros that returns void, requiring to change
>       all the virtio-gpu processing code.
> 
>     - Added back virtio_gpu_virgl_resource as was requested by Akihiko Odaki,
>       though I'm not convinced it's really needed.
> 
>     - Switched to use GArray, renamed capset2_max_ver/size vars and moved
>       "vulkan" property definition to the virtio-gpu-gl device in the Venus
>       patch, like was suggested by Akihiko Odaki.
> 
>     - Moved UUID to virtio_gpu_virgl_resource and dropped UUID save/restore
>       since it will require bumping VM version and virgl device isn't miratable
>       anyways.
> 
>     - Fixed exposing UUID feature with Rutabaga
> 
>     - Dropped linux-headers update patch because headers were already updated
>       in Qemu/staging.
> 
>     - Added patch that updates virtio migration doc with a note about
>     virtio-gpu
>       migration specifics, suggested by Akihiko Odaki.
> 
>     - Addressed coding style issue noticed by Akihiko Odaki
> 
>     Changes from V7 to V8
> 
>     - Supported suspension of virtio-gpu commands processing and made
>       unmapping of hostmem region asynchronous by blocking/suspending
>       cmd processing until region is unmapped. Suggested by Akihiko Odaki.
> 
>     - Fixed arm64 building of x86 targets using updated linux-headers.
>       Corrected the update script. Thanks to Rob Clark for reporting
>       the issue.
> 
>     - Added new patch that makes registration of virgl capsets dynamic.
>       Requested by Antonio Caggiano and Pierre-Eric Pelloux-Prayer.
> 
>     - Venus capset now isn't advertised if Vulkan is disabled with vulkan=false
> 
>     Changes from V6 to V7
> 
>     - Used scripts/update-linux-headers.sh to update Qemu headers based
>       on Linux v6.8-rc3 that adds Venus capset definition to virtio-gpu
>       protocol, was requested by Peter Maydel
> 
>     - Added r-bs that were given to v6 patches. Corrected missing s-o-bs
> 
>     - Dropped context_init Qemu's virtio-gpu device configuration flag,
>       was suggested by Marc-André Lureau
> 
>     - Added missing error condition checks spotted by Marc-André Lureau
>       and Akihiko Odaki, and few more
> 
>     - Returned back res->mr referencing to memory_region_init_ram_ptr() like
>       was suggested by Akihiko Odaki. Incorporated fix suggested by Pierre-Eric
>       to specify the MR name
> 
>     - Dropped the virgl_gpu_resource wrapper, cleaned up and simplified
>       patch that adds blob-cmd support
> 
>     - Fixed improper blob resource removal from resource list on resource_unref
>       that was spotted by Akihiko Odaki
> 
>     - Change order of the blob patches, was suggested by Akihiko Odaki.
>       The cmd_set_scanout_blob support is enabled first
> 
>     - Factored out patch that adds resource management support to
>     virtio-gpu-gl,
>       was requested by Marc-André Lureau
> 
>     - Simplified and improved the UUID support patch, dropped the hash table
>       as we don't need it for now. Moved QemuUUID to
>     virtio_gpu_simple_resource.
>       This all was suggested by Akihiko Odaki and Marc-André Lureau
> 
>     - Dropped console_has_gl() check, suggested by Akihiko Odaki
> 
>     - Reworked Meson cheking of libvirglrender features, made new features
>       available based on virglrender pkgconfig version instead of checking
>       symbols in header. This should fix build error using older virglrender
>       version, reported by Alex Bennée
> 
>     - Made enabling of Venus context configrable via new virtio-gpu device
>       "vulkan=true" flag, suggested by Marc-André Lureau. The flag is disabled
>       by default because it requires blob and hostmem options to be enabled
>       and configured
> 
>     Changes from V5 to V6
> 
>     - Move macros configurations under virgl.found() and rename
>       HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS.
> 
>     - Handle the case while context_init is disabled.
> 
>     - Enable context_init by default.
> 
>     - Move virtio_gpu_virgl_resource_unmap() into
>       virgl_cmd_resource_unmap_blob().
> 
>     - Introduce new struct virgl_gpu_resource to store virgl specific members.
> 
>     - Remove erro handling of g_new0, because glib will abort() on OOM.
> 
>     - Set resource uuid as option.
> 
>     - Implement optional subsection of vmstate_virtio_gpu_resource_uuid_state
>       for virtio live migration.
> 
>     - Use g_int_hash/g_int_equal instead of the default
> 
>     - Add scanout_blob function for virtio-gpu-virgl
> 
>     - Resolve the memory leak on virtio-gpu-virgl
> 
>     - Remove the unstable API flags check because virglrenderer is already 1.0
> 
>     - Squash the render server flag support into "Initialize Venus"
> 
>     Changes from V4 (virtio gpu V4) to V5
> 
>     - Inverted patch 5 and 6 because we should configure
>       HAVE_VIRGL_CONTEXT_INIT firstly.
> 
>     - Validate owner of memory region to avoid slowing down DMA.
> 
>     - Use memory_region_init_ram_ptr() instead of
>       memory_region_init_ram_device_ptr().
> 
>     - Adjust sequence to allocate gpu resource before virglrender resource
>       creation
> 
>     - Add virtio migration handling for uuid.
> 
>     - Send kernel patch to define VIRTIO_GPU_CAPSET_VENUS.
>       https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
> 
>     - Add meson check to make sure unstable APIs defined from 0.9.0.
> 
>     Changes from V1 to V2 (virtio gpu V4)
> 
>     - Remove unused #include "hw/virtio/virtio-iommu.h"
> 
>     - Add a local function, called virgl_resource_destroy(), that is used
>       to release a vgpu resource on error paths and in resource_unref.
> 
>     - Remove virtio_gpu_virgl_resource_unmap from
>       virtio_gpu_cleanup_mapping(),
>       since this function won't be called on blob resources and also because
>       blob resources are unmapped via virgl_cmd_resource_unmap_blob().
> 
>     - In virgl_cmd_resource_create_blob(), do proper cleanup in error paths
>       and move QTAILQ_INSERT_HEAD(&g->reslist, res, next) after the resource
>       has been fully initialized.
> 
>     - Memory region has a different life-cycle from virtio gpu resources
>       i.e. cannot be released synchronously along with the vgpu resource.
>       So, here the field "region" was changed to a pointer and is allocated
>       dynamically when the blob is mapped.
>       Also, since the pointer can be used to indicate whether the blob
>       is mapped, the explicite field "mapped" was removed.
> 
>     - In virgl_cmd_resource_map_blob(), add check on the value of
>       res->region, to prevent beeing called twice on the same resource.
> 
>     - Add a patch to enable automatic deallocation of memory regions to resolve
>       use-after-free memory corruption with a reference.
> 
> 
>     Antonio Caggiano (1):
>       virtio-gpu: Support Venus context
> 
>     Dmitry Osipenko (8):
>       virtio-gpu: Use trace events for tracking number of in-flight fences
>       virtio-gpu: Move fence_poll timer to VirtIOGPUGL
>       virtio-gpu: Move print_stats timer to VirtIOGPUGL
>       virtio-gpu: Handle virtio_gpu_virgl_init() failure
>       virtio-gpu: Unrealize GL device
>       virtio-gpu: Use pkgconfig version to decide which virgl features are
>         available
>       virtio-gpu: Don't require udmabuf when blobs and virgl are enabled
>       virtio-gpu: Support suspension of commands processing
> 
>     Huang Rui (2):
>       virtio-gpu: Support context-init feature with virglrenderer
>       virtio-gpu: Add virgl resource management
> 
>     Pierre-Eric Pelloux-Prayer (1):
>       virtio-gpu: Register capsets dynamically
> 
>     Robert Beckett (1):
>       virtio-gpu: Handle resource blob commands
> 
>      docs/system/devices/virtio-gpu.rst |  11 +
>      hw/display/trace-events            |   3 +
>      hw/display/virtio-gpu-gl.c         |  62 ++-
>      hw/display/virtio-gpu-virgl.c      | 585 +++++++++++++++++++++++++++--
>      hw/display/virtio-gpu.c            |  44 ++-
>      include/hw/virtio/virtio-gpu.h     |  32 +-
>      meson.build                        |   5 +-
>      7 files changed, 685 insertions(+), 57 deletions(-)
> 
>     --
>     2.45.2
> 
> 
> 
> 
> --
> Marc-André Lureau