Re: [Qemu-devel] [PATCH for 2.8 02/11] virtio: convert to use DMA api

[Michael S. Tsirkin]

On Tue, Aug 30, 2016 at 01:02:14PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 30, 2016 at 09:31:27AM +0200, Cornelia Huck wrote:
> > On Tue, 30 Aug 2016 11:06:50 +0800
> > Jason Wang <address@hidden> 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 when iommu platform (VIRTIO_F_IOMMU_PLATFORM) was enabled
> > >   for this device.
> > > 
> > > Cc: Michael S. Tsirkin <address@hidden>
> > > Cc: Stefan Hajnoczi <address@hidden>
> > > Cc: Kevin Wolf <address@hidden>
> > > Cc: Amit Shah <address@hidden>
> > > Cc: Paolo Bonzini <address@hidden>
> > > Cc: address@hidden
> > > Signed-off-by: Jason Wang <address@hidden>
> > > ---
> > >  hw/block/virtio-blk.c             |  2 +-
> > >  hw/char/virtio-serial-bus.c       |  3 +-
> > >  hw/scsi/virtio-scsi.c             |  4 ++-
> > >  hw/virtio/virtio-pci.c            | 14 +++++++++
> > >  hw/virtio/virtio.c                | 62 
> > > ++++++++++++++++++++++++---------------
> > >  include/hw/virtio/virtio-access.h | 43 ++++++++++++++++++++-------
> > >  include/hw/virtio/virtio-bus.h    |  1 +
> > >  include/hw/virtio/virtio.h        |  8 +++--
> > >  8 files changed, 98 insertions(+), 39 deletions(-)
> > > 
> > 
> > > diff --git a/include/hw/virtio/virtio-access.h 
> > > b/include/hw/virtio/virtio-access.h
> > > index 440b455..4071dad 100644
> > > --- a/include/hw/virtio/virtio-access.h
> > > +++ b/include/hw/virtio/virtio-access.h
> > > @@ -17,12 +17,25 @@
> > >  #define QEMU_VIRTIO_ACCESS_H
> > > 
> > >  #include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-bus.h"
> > >  #include "exec/address-spaces.h"
> > > 
> > >  #if defined(TARGET_PPC64) || defined(TARGET_ARM)
> > >  #define LEGACY_VIRTIO_IS_BIENDIAN 1
> > >  #endif
> > > 
> > > +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 (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
> > > +        k->get_dma_as) {
> > > +        return k->get_dma_as(qbus->parent);
> > > +    }
> > > +    return &address_space_memory;
> > > +}
> > 
> > One thing I'm a bit worried about is that we're introducing a check
> > even if we know that the device will never support
> > VIRTIO_F_IOMMU_PLATFORM (i.e. virtio-ccw). The qom incantations will
> > add cycles to any invocation of this.
> 
> Yes - let's do container_of calls as opposed to QOM on data path.

BTW downstreams are building with --disable-qom-cast-debug which drops
all QOM casts on data path - one way is to say we just make this the
default upstream as well. Another to say that we want to distinguish
fast path calls from slow path, this way we will be able to bring back
some of the checks.


> > Is the address space likely to change during device lifetime? Can we
> > cache it in some way?