On Wed, Feb 26, 2020 at 06:17:34PM +0800, Jason Wang wrote:
>
> On 2020/2/26 下午5:53, Michael S. Tsirkin wrote:
> > On Wed, Feb 26, 2020 at 05:43:57PM +0800, Jason Wang wrote:
> > > We turn on device IOTLB via VIRTIO_F_IOMMU_PLATFORM unconditionally on
> > > platform without IOMMU support. This can lead unnecessary IOTLB
> > > transactions which will damage the performance.
> > >
> > > Fixing this by check whether the device is backed by IOMMU and disable
> > > device IOTLB.
> > >
> > > Reported-by: Halil Pasic <pa...@linux.ibm.com>
> > > Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> > Well it's just an optimization, isn't it?
>
>
> Kind of, or a fix for the performance.
>
Given this wasn't a regression, it's a valuable enhancement
but Fixes: seems to agressive.
> > I don't think it's justified to push this to everyone using
> > vhost with IOTLB, is it?
>
>
> My understanding is that the function should be equivalent to IOTLB in this
> case.
>
> Since no IOMMU is used and device may only see GPA.
>
> Another possible direction is to qemu to update memory table via device
> IOTLB API, this seems less straightforward.
>
>
> > If you disagree, could you comment a bit on which configurations where
> > tested?
>
>
> I just do dirty shortcut to emulate the platform without get_dma_as wth
> IOMMU_PLATFORM set.
>
> Thanks
If you want Fixes tag with an expectation of everyone backporting,
then this also needs to be tested on a platform with a viommu,
such as dpdk within guest.
>
> >
> > > Cc: qemu-sta...@nongnu.org
> > > Signed-off-by: Jason Wang <jasow...@redhat.com>
> > Halil could you test this pls? Does this help your performance issue?
> >
> > > ---
> > > Changes from V1:
> > > - do not check acked_features
> > > - reuse vhost_dev_has_iommu()
> > > ---
> > > hw/virtio/vhost.c | 12 +++++++++++-
> > > 1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 9edfadc81d..9182a00495 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -290,7 +290,14 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
> > > {
> > > VirtIODevice *vdev = dev->vdev;
> > > - return virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> > > + /*
> > > + * For vhost, VIRTIO_F_IOMMU_PLATFORM means the backend support
> > > + * incremental memory mapping API via IOTLB API. For platform that
> > > + * does not have IOMMU, there's no need to enable this feature
> > > + * which may cause unnecessary IOTLB miss/update trnasactions.
> > > + */
> > > + return vdev->dma_as != &address_space_memory &&
> > > + virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> > > }
> > > static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
> > > @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev
> > > *dev,
> > > if (enable_log) {
> > > features |= 0x1ULL << VHOST_F_LOG_ALL;
> > > }
> > > + if (!vhost_dev_has_iommu(dev)) {
> > > + features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> > > + }
> > > r = dev->vhost_ops->vhost_set_features(dev, features);
> > > if (r < 0) {
> > > VHOST_OPS_DEBUG("vhost_set_features failed");
> > > --
> > > 2.19.1
