Re: [PATCH] drm/virtio: use the DMA API for resource backing on Xen

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing dma_sync_sgtable_for_cpu() in guest-bound transfer paths causes data corruption with SWIOTLB bounce buffers.
--

--- Patch [1]: [PATCH] drm/virtio: use the DMA API for resource backing on Xen ---
commit 5102ee1180b3819921a73a9ff7e4e18ce5a910f1
Author: Benjamin Leggett <[email protected]>

drm/virtio: use the DMA API for resource backing on Xen

This commit enables the use of the DMA API for virtio-gpu resource backing
on Xen PV domains, regardless of the VIRTIO_F_ACCESS_PLATFORM feature bit.
It introduces virtio_gpu_use_dma_api() and updates the resource
initialization and transfer paths to use it. This ensures that the host
receives correct DMA addresses instead of guest-physical addresses when
swiotlb is active.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 905b1f42cd983..568fefd713e0f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c

[ ... ]

> @@ -1231,7 +1231,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
>  	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(objs->objs[0]);
>  	struct virtio_gpu_transfer_host_3d *cmd_p;
>  	struct virtio_gpu_vbuffer *vbuf;
> -	bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
> +	bool use_dma_api = virtio_gpu_use_dma_api(vgdev->vdev);
>  
>  	if (virtio_gpu_is_shmem(bo) && use_dma_api)
>  		dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
>  					    bo->base.sgt, DMA_TO_DEVICE);

[Severity: High]
This is a pre-existing issue, but while enabling the DMA API for Xen PV
domains, does this leave guest-bound transfers vulnerable to data corruption
with SWIOTLB bounce buffers?

While this patch ensures dma_sync_sgtable_for_device() is called for
host-bound transfers (DMA_TO_DEVICE), it appears that guest-bound transfers
are missing the corresponding CPU synchronization:

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_cmd_transfer_from_host_3d() {
    ...
    // Missing dma_sync_sgtable_for_cpu() or
    // dma_sync_sgtable_for_device(..., DMA_FROM_DEVICE)
}

If a user-space application maps a virtio-gpu SHMEM buffer and triggers a
VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D via
DRM_IOCTL_VIRTGPU_TRANSFER_FROM_HOST, will the CPU read stale data from
the original backing pages?

Because the driver never calls dma_sync_sgtable_for_cpu() when the transfer
completes, it looks like the SWIOTLB will never copy the bounce buffer
contents back to the original physical pages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1