Re: [PATCH v4] drm/virtio: sync shmem backing on guest-bound transfers

[Dmitry Osipenko]

On 8/15/26 01:20, Benjamin Leggett via B4 Relay wrote:
> From: Benjamin Leggett <benjamin@xxxxxxxx>
> 
> virtio_gpu_cmd_transfer_to_host_{2d,3d}() sync the shmem backing for the
> device before the transfer, but nothing syncs for the CPU when a transfer
> runs the other way. That breaks two ways. Where the DMA layer bounces, the
> device writes into the bounce buffer while the guest keeps reading the
> original pages. Where DMA is not coherent, the device writes memory while
> the CPU keeps stale cache lines, because nothing reaches
> arch_sync_dma_for_cpu(). Either way DRM_IOCTL_VIRTGPU_TRANSFER_FROM_HOST
> hands back stale data.
> 
> Sashiko originally found this in
> https://lore.kernel.org/dri-devel/20260806231002.27B4D1F000E9@xxxxxxxxxxxxxxx
> but the suggestion there to fix this with dma_sync_sgtable_for_cpu()
> isn't a sufficient fix, for two reasons.
> 
> - The transfer is asynchronous. virtio_gpu_cmd_transfer_from_host_3d() only
> queues the command, so a sync there would run before the device had written
> anything. It belongs on completion, and ahead of any fence signalling.
> A waiter woken by the fence would otherwise race the sync and read the
> backing pages regardless. It needs its own pass over the reclaim list
> rather than a step inside the existing one, because
> virtio_gpu_fence_event_process() also signals every earlier fence in the
> same context, so any entry in that loop may signal an earlier entry's
> fence.
> 
> - The transfer is also partial, carrying an offset, a level and a box.
> Where the mapping bounces, a sync for the CPU copies the whole mapping
> back, so unless the mapping is primed first the regions the device did not
> write come back holding whatever the bounce buffer contained, discarding
> data the guest owned.
> 
> So the fix: Prime the mapping before queueing, tag the vbuffer, and sync
> for the CPU on completion before the fence is signalled.
> 
> A second transfer must not snapshot the mapping while an earlier one is
> still in flight, or the snapshot would predate whatever the CPU wrote once
> the earlier fence signalled and the later sync would discard it.
> 
> To mitigate this, wait for outstanding fences under the reservation before
> priming.
> 
> Neither sync copies anything unless the mapping genuinely bounces:
> swiotlb_sync_single_for_cpu() and its Xen counterpart look the address up
> in the bounce pool and return early when it is absent. On a platform with
> non-coherent DMA they still perform the necessary cache maintenance.
> 
> The range cannot be narrowed to the box, since for a non-blob resource
> virtio_gpu_transfer_from_host_ioctl() rejects a caller-supplied stride and
> layer_stride, leaving the layout to the host and the guest with no way to
> work out which bytes the device writes. A host3d guest blob does carry
> both, so its extent could be bounded, but the sync is left whole there too
> rather than special-cased: priming makes the untouched regions round-trip
> unchanged either way.
> 
> Behaviour changes worth noting:
> 
> - TRANSFER_FROM_HOST can now block, where before it returned as soon as the
> command was queued. Repeated readbacks of one resource serialise, and a
> readback can wait behind an earlier queued command that touched it, since
> virtio_gpu_array_add_fence() tags uploads, execbufs and plane flushes
> alike with DMA_RESV_USAGE_WRITE. -ERESTARTSYS was already possible here
> via dma_resv_lock_interruptible().
> 
> - A CPU write racing an in-flight transfer to the same resource is now
> lost, where before it survived and the transfer was lost instead. Priming
> captures the pages as of queueing, so a write landing before completion is
> overwritten by the sync.
> 
> - TRANSFER_TO_HOST can also block now, but only while a guest-bound
> transfer on the same resource is outstanding, which happens only for
> callers that issue both without waiting.
> 
> - Where a batch of completions contains a guest-bound transfer, the sync
> pass delays fence signalling for the whole batch. Only bounced pages are
> copied and the swiotlb pool bounds it. A batch with no such transfer is
> unaffected.
> 
> Tested under QEMU on x86 with swiotlb=force and virtio-vga-gl
> iommu_platform=on, which forces both preconditions required to hit the
> original bug.
> 
> Reported-by: Sashiko AI review <sashiko-bot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/dri-devel/20260806231002.27B4D1F000E9@xxxxxxxxxxxxxxx/
> Signed-off-by: Benjamin Leggett <benjamin@xxxxxxxx>
> ---
> This depends on 6a736d2f9d0c ("drm/virtio: use the DMA API for resource backing on Xen"), currently in
> drm-misc-fixes only, so it needs to go through the same branch.
> ---
> Changes in v4:
> - drm/virtio: use DMA_RESV_USAGE_READ.
> - Link to v3: https://lore.kernel.org/r/20260814-virtgpu-from-host-sync-v3-1-f2538afd7d6e@xxxxxxxx
> 
> Changes in v3:
> - drm/virtio: use smp_load_acquire()/smp_store_release().
> - Link to v2: https://lore.kernel.org/r/20260814-virtgpu-from-host-sync-v2-1-fa3910caf3e5@xxxxxxxx
> 
> Changes in v2:
> - drm/virtio: add guard on virtio_gpu_transfer_to_host_ioctl.
> - Link to v1: https://lore.kernel.org/r/20260814-virtgpu-from-host-sync-v1-1-814e3afb5b08@xxxxxxxx
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  5 ++++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 43 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/virtio/virtgpu_vq.c    | 46 ++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+)

Applied to misc-fixes, thanks!

-- 
Best regards,
Dmitry