Re: [Qemu-devel] [RFC] virtio: convert to use DMA api

[Jason Wang <jasowang@redhat.com>]

On 11/23/2015 04:06 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 23, 2015 at 03:41:11PM +0800, Jason Wang wrote:
>> Currently, all virtio devices bypass IOMMU completely. This is because
>> address_space_memory is assumed and used during DMA emulation. This
>> patch converts the virtio core API to use DMA API. This idea is
>>
>> - introducing a new transport specific helper to query the dma address
>>   space. (only pci version is implemented).
>> - query and use this address space during virtio device guest memory
>>   accessing
>>
>> With this virtiodevices will not bypass IOMMU anymore. Little tested with
>> intel_iommu=on with virtio guest DMA series posted in
>> https://lkml.org/lkml/2015/10/28/64.
>>
>> TODO:
>> - Feature bit for this
> This probably implies it should only be implemented
> if device supports the modern mode.
> Not sure what's the best way to handle transitional
> devices.

Don't see the issue here. Transitional devices could decide based on the
feature bit?

, for the name, how about something like VIRTIO_F_HOST_IOMMU ?

>
>> - Implement this for all transports
> Tested with vhost, and it does not seem to work.
> So more TODO:
>  - make this work with vhost-user and vhost-net.
>
> Also, it seems prudent to add
>  - make the new behaviour conditional on a new property

Right.

>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/block/virtio-blk.c             |  2 +-
>>  hw/char/virtio-serial-bus.c       |  2 +-
>>  hw/scsi/virtio-scsi.c             |  2 +-
>>  hw/virtio/virtio-pci.c            |  9 +++++++++
>>  hw/virtio/virtio.c                | 36 +++++++++++++++++++--------------
>>  include/hw/virtio/virtio-access.h | 42 +++++++++++++++++++++++++++++----------
>>  include/hw/virtio/virtio-bus.h    |  1 +
>>  include/hw/virtio/virtio.h        |  2 +-
>>  8 files changed, 67 insertions(+), 29 deletions(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index e70fccf..5499480 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -846,7 +846,7 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
>>          req->next = s->rq;
>>          s->rq = req;
>>  
>> -        virtqueue_map(&req->elem);
>> +        virtqueue_map(vdev, &req->elem);
>>      }
>>  
>>      return 0;
>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>> index 497b0af..39e9ed2 100644
>> --- a/hw/char/virtio-serial-bus.c
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -705,7 +705,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
>>  
>>                  qemu_get_buffer(f, (unsigned char *)&port->elem,
>>                                  sizeof(port->elem));
>> -                virtqueue_map(&port->elem);
>> +                virtqueue_map(&s->parent_obj, &port->elem);
>>  
>>                  /*
>>                   *  Port was throttled on source machine.  Let's
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 7655401..0734d27 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -206,7 +206,7 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>>      req = virtio_scsi_init_req(s, vs->cmd_vqs[n]);
>>      qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
>>  
>> -    virtqueue_map(&req->elem);
>> +    virtqueue_map(&vs->parent_obj, &req->elem);
>>  
>>      if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
>>                                sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index dd48562..876505d 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1211,6 +1211,14 @@ static int virtio_pci_query_nvectors(DeviceState *d)
>>      return proxy->nvectors;
>>  }
>>  
>> +static AddressSpace *virtio_pci_get_dma_as(DeviceState *d)
>> +{
>> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>> +    PCIDevice *dev = &proxy->pci_dev;
>> +
>> +    return pci_get_address_space(dev);
>> +}
>> +
>>  static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
>>                                     struct virtio_pci_cap *cap)
>>  {
>> @@ -2476,6 +2484,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
>>      k->device_plugged = virtio_pci_device_plugged;
>>      k->device_unplugged = virtio_pci_device_unplugged;
>>      k->query_nvectors = virtio_pci_query_nvectors;
>> +    k->get_dma_as = virtio_pci_get_dma_as;
>>  }
>>  
>>  static const TypeInfo virtio_pci_bus_info = {
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 1edef59..e09430d 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -21,6 +21,7 @@
>>  #include "hw/virtio/virtio-bus.h"
>>  #include "migration/migration.h"
>>  #include "hw/virtio/virtio-access.h"
>> +#include "sysemu/dma.h"
>>  
>>  /*
>>   * The alignment to use between consumer and producer parts of vring.
>> @@ -247,6 +248,7 @@ int virtio_queue_empty(VirtQueue *vq)
>>  static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
>>                                 unsigned int len)
>>  {
>> +    AddressSpace *dma_as = virtio_get_dma_as(vq->vdev);
>>      unsigned int offset;
>>      int i;
>>  
>> @@ -254,17 +256,17 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
>>      for (i = 0; i < elem->in_num; i++) {
>>          size_t size = MIN(len - offset, elem->in_sg[i].iov_len);
>>  
>> -        cpu_physical_memory_unmap(elem->in_sg[i].iov_base,
>> -                                  elem->in_sg[i].iov_len,
>> -                                  1, size);
>> +        dma_memory_unmap(dma_as, elem->in_sg[i].iov_base, elem->in_sg[i].iov_len,
>> +                         DMA_DIRECTION_FROM_DEVICE, size);
>>  
>>          offset += size;
>>      }
>>  
>>      for (i = 0; i < elem->out_num; i++)
>> -        cpu_physical_memory_unmap(elem->out_sg[i].iov_base,
>> -                                  elem->out_sg[i].iov_len,
>> -                                  0, elem->out_sg[i].iov_len);
>> +        dma_memory_unmap(dma_as, elem->out_sg[i].iov_base,
>> +                         elem->out_sg[i].iov_len,
>> +                         DMA_DIRECTION_TO_DEVICE,
>> +                         elem->out_sg[i].iov_len);
>>  }
>>  
>>  void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>> @@ -448,9 +450,9 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>>      return in_bytes <= in_total && out_bytes <= out_total;
>>  }
>>  
>> -static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
>> -                                unsigned int *num_sg, unsigned int max_size,
>> -                                int is_write)
>> +static void virtqueue_map_iovec(VirtIODevice *vdev, struct iovec *sg,
>> +                                hwaddr *addr, unsigned int *num_sg,
>> +                                unsigned int max_size, int is_write)
>>  {
>>      unsigned int i;
>>      hwaddr len;
>> @@ -469,7 +471,10 @@ static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
>>  
>>      for (i = 0; i < *num_sg; i++) {
>>          len = sg[i].iov_len;
>> -        sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
>> +        sg[i].iov_base = dma_memory_map(virtio_get_dma_as(vdev),
>> +                                        addr[i], &len, is_write ?
>> +                                        DMA_DIRECTION_FROM_DEVICE :
>> +                                        DMA_DIRECTION_TO_DEVICE);
>>          if (!sg[i].iov_base) {
>>              error_report("virtio: error trying to map MMIO memory");
>>              exit(1);
>> @@ -491,13 +496,14 @@ static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
>>      }
>>  }
>>  
>> -void virtqueue_map(VirtQueueElement *elem)
>> +void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem)
>>  {
>> -    virtqueue_map_iovec(elem->in_sg, elem->in_addr, &elem->in_num,
>> +    virtqueue_map_iovec(vdev, elem->in_sg, elem->in_addr, &elem->in_num,
>>                          MIN(ARRAY_SIZE(elem->in_sg), ARRAY_SIZE(elem->in_addr)),
>>                          1);
>> -    virtqueue_map_iovec(elem->out_sg, elem->out_addr, &elem->out_num,
>> -                        MIN(ARRAY_SIZE(elem->out_sg), ARRAY_SIZE(elem->out_addr)),
>> +    virtqueue_map_iovec(vdev, elem->out_sg, elem->out_addr, &elem->out_num,
>> +                        MIN(ARRAY_SIZE(elem->out_sg),
>> +                        ARRAY_SIZE(elem->out_addr)),
>>                          0);
>>  }
>>  
>> @@ -562,7 +568,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>>      } while ((i = virtqueue_next_desc(vdev, desc_pa, i, max)) != max);
>>  
>>      /* Now map what we have collected */
>> -    virtqueue_map(elem);
>> +    virtqueue_map(vdev, elem);
>>  
>>      elem->index = head;
>>  
>> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
>> index 8aec843..cca7d2a 100644
>> --- a/include/hw/virtio/virtio-access.h
>> +++ b/include/hw/virtio/virtio-access.h
>> @@ -15,8 +15,20 @@
>>  #ifndef _QEMU_VIRTIO_ACCESS_H
>>  #define _QEMU_VIRTIO_ACCESS_H
>>  #include "hw/virtio/virtio.h"
>> +#include "hw/virtio/virtio-bus.h"
>>  #include "exec/address-spaces.h"
>>  
>> +static inline AddressSpace *virtio_get_dma_as(VirtIODevice *vdev)
>> +{
>> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> +
>> +    if (k->get_dma_as) {
>> +        return k->get_dma_as(qbus->parent);
>> +    }
>> +    return &address_space_memory;
>> +}
>> +
>>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>>  {
>>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>> @@ -47,45 +59,55 @@ static inline bool virtio_legacy_is_cross_endian(VirtIODevice *vdev)
>>  
>>  static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
>>  {
>> +    AddressSpace *dma_as = virtio_get_dma_as(vdev);
>> +
>>      if (virtio_access_is_big_endian(vdev)) {
>> -        return lduw_be_phys(&address_space_memory, pa);
>> +        return lduw_be_phys(dma_as, pa);
>>      }
>> -    return lduw_le_phys(&address_space_memory, pa);
>> +    return lduw_le_phys(dma_as, pa);
>>  }
>>  
>>  static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
>>  {
>> +    AddressSpace *dma_as = virtio_get_dma_as(vdev);
>> +
>>      if (virtio_access_is_big_endian(vdev)) {
>> -        return ldl_be_phys(&address_space_memory, pa);
>> +        return ldl_be_phys(dma_as, pa);
>>      }
>> -    return ldl_le_phys(&address_space_memory, pa);
>> +    return ldl_le_phys(dma_as, pa);
>>  }
>>  
>>  static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
>>  {
>> +    AddressSpace *dma_as = virtio_get_dma_as(vdev);
>> +
>>      if (virtio_access_is_big_endian(vdev)) {
>> -        return ldq_be_phys(&address_space_memory, pa);
>> +        return ldq_be_phys(dma_as, pa);
>>      }
>> -    return ldq_le_phys(&address_space_memory, pa);
>> +    return ldq_le_phys(dma_as, pa);
>>  }
>>  
>>  static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa,
>>                                     uint16_t value)
>>  {
>> +    AddressSpace *dma_as = virtio_get_dma_as(vdev);
>> +
>>      if (virtio_access_is_big_endian(vdev)) {
>> -        stw_be_phys(&address_space_memory, pa, value);
>> +        stw_be_phys(dma_as, pa, value);
>>      } else {
>> -        stw_le_phys(&address_space_memory, pa, value);
>> +        stw_le_phys(dma_as, pa, value);
>>      }
>>  }
>>  
>>  static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
>>                                     uint32_t value)
>>  {
>> +    AddressSpace *dma_as = virtio_get_dma_as(vdev);
>> +
>>      if (virtio_access_is_big_endian(vdev)) {
>> -        stl_be_phys(&address_space_memory, pa, value);
>> +        stl_be_phys(dma_as, pa, value);
>>      } else {
>> -        stl_le_phys(&address_space_memory, pa, value);
>> +        stl_le_phys(dma_as, pa, value);
>>      }
>>  }
>>  
>> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
>> index 6c3d4cb..638735e 100644
>> --- a/include/hw/virtio/virtio-bus.h
>> +++ b/include/hw/virtio/virtio-bus.h
>> @@ -71,6 +71,7 @@ typedef struct VirtioBusClass {
>>       * Note that changing this will break migration for this transport.
>>       */
>>      bool has_variable_vring_alignment;
>> +    AddressSpace *(*get_dma_as)(DeviceState *d);
>>  } VirtioBusClass;
>>  
>>  struct VirtioBusState {
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 205fadf..7a8ef13 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -151,7 +151,7 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>>  void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>>                      unsigned int len, unsigned int idx);
>>  
>> -void virtqueue_map(VirtQueueElement *elem);
>> +void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem);
>>  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
>>  int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>>                            unsigned int out_bytes);
>> -- 
>> 2.5.0