virtio-scsi: finalize IOMMU support

[Qemu-devel] [PATCH] virtio-scsi: finalize IOMMU support

Posted by Jason Wang 6 years, 9 months ago

After converting to use DMA api for virtio devices, we should use
dma_as instead of address_space_memory. Otherwise it won't work if
IOMMU is enabled.

Fixes: commit 8607f5c3072c ("virtio: convert to use DMA api")
Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/scsi/virtio-scsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index f46f06d..d076fe7 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -43,12 +43,13 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
 
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     const size_t zero_skip =
         offsetof(VirtIOSCSIReq, resp_iov) + sizeof(req->resp_iov);
 
     req->vq = vq;
     req->dev = s;
-    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
+    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, vdev->dma_as);
     qemu_iovec_init(&req->resp_iov, 1);
     memset((uint8_t *)req + zero_skip, 0, sizeof(*req) - zero_skip);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] virtio-scsi: finalize IOMMU support

Posted by Fam Zheng 6 years, 9 months ago

On Tue, 07/04 20:21, Jason Wang wrote:
> After converting to use DMA api for virtio devices, we should use
> dma_as instead of address_space_memory. Otherwise it won't work if
> IOMMU is enabled.
> 
> Fixes: commit 8607f5c3072c ("virtio: convert to use DMA api")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH] virtio-scsi: finalize IOMMU support

Posted by Wei Xu 6 years, 9 months ago

On Tue, Jul 04, 2017 at 08:21:06PM +0800, Jason Wang wrote:
> After converting to use DMA api for virtio devices, we should use
> dma_as instead of address_space_memory. Otherwise it won't work if
> IOMMU is enabled.
> 
> Fixes: commit 8607f5c3072c ("virtio: convert to use DMA api")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index f46f06d..d076fe7 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -43,12 +43,13 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
>  
>  void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
>  {
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>      const size_t zero_skip =
>          offsetof(VirtIOSCSIReq, resp_iov) + sizeof(req->resp_iov);
>  
>      req->vq = vq;
>      req->dev = s;
> -    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
> +    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, vdev->dma_as);
>      qemu_iovec_init(&req->resp_iov, 1);
>      memset((uint8_t *)req + zero_skip, 0, sizeof(*req) - zero_skip);
>  }

Reviewed-by: Wei Xu <wexu@redhat.com>

> -- 
> 2.7.4
> 
>

Re: [Qemu-devel] [PATCH] virtio-scsi: finalize IOMMU support

Posted by Fam Zheng 6 years, 9 months ago

On Tue, 07/04 20:21, Jason Wang wrote:
> After converting to use DMA api for virtio devices, we should use
> dma_as instead of address_space_memory. Otherwise it won't work if
> IOMMU is enabled.
> 
> Fixes: commit 8607f5c3072c ("virtio: convert to use DMA api")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index f46f06d..d076fe7 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -43,12 +43,13 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
>  
>  void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
>  {
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>      const size_t zero_skip =
>          offsetof(VirtIOSCSIReq, resp_iov) + sizeof(req->resp_iov);
>  
>      req->vq = vq;
>      req->dev = s;
> -    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
> +    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, vdev->dma_as);

Not related to this patch, but is our vIOMMU iotlb implementation safe for data
plane (both virtio-blk and virtio-scsi)? There we don't have BQL to synchronize
between IOVA lookup and update, which means threads can race when accessing the
iotlb GHashTable.

Did I overlook a synchronization mechanism? Or is it missing? Paolo, MST?

Fam

>      qemu_iovec_init(&req->resp_iov, 1);
>      memset((uint8_t *)req + zero_skip, 0, sizeof(*req) - zero_skip);
>  }
> -- 
> 2.7.4
> 
>

Re: [Qemu-devel] [PATCH] virtio-scsi: finalize IOMMU support

Posted by Paolo Bonzini 6 years, 9 months ago


On 05/07/2017 08:23, Fam Zheng wrote:
>>      req->vq = vq;
>>      req->dev = s;
>> -    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
>> +    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, vdev->dma_as);
> Not related to this patch, but is our vIOMMU iotlb implementation safe for data
> plane (both virtio-blk and virtio-scsi)? There we don't have BQL to synchronize
> between IOVA lookup and update, which means threads can race when accessing the
> iotlb GHashTable.
> 
> Did I overlook a synchronization mechanism? Or is it missing? Paolo, MST?

I think it's missing.  One way to fix it could be to finish the
MemoryRegionCache work and have an IOMMU->virtio notifier, similar to
how vhost works.

Paolo