* [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM
@ 2020-02-26 7:06 Jason Wang
2020-02-26 7:11 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2020-02-26 7:06 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: pasic, Jason Wang
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 <pasic@linux.ibm.com>
Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
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..6e12c3d2de 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_has_feature(dev->acked_features, 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 (dev->vdev->dma_as == &address_space_memory) {
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM
2020-02-26 7:06 [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM Jason Wang
@ 2020-02-26 7:11 ` Michael S. Tsirkin
2020-02-26 7:20 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2020-02-26 7:11 UTC (permalink / raw)
To: Jason Wang; +Cc: pasic, qemu-devel
On Wed, Feb 26, 2020 at 03:06:47PM +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 <pasic@linux.ibm.com>
> Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> 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..6e12c3d2de 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_has_feature(dev->acked_features, VIRTIO_F_IOMMU_PLATFORM);
> }
>
> static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
Why check acked_features and not host features here?
I'd worry that if we do it like this, userspace driver
within guest can clear the feature and make device access
memory directly.
> @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
> if (enable_log) {
> features |= 0x1ULL << VHOST_F_LOG_ALL;
> }
> + if (dev->vdev->dma_as == &address_space_memory) {
> + features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> + }
That's a guest visible change. Which seems at best unnecessary.
> r = dev->vhost_ops->vhost_set_features(dev, features);
> if (r < 0) {
> VHOST_OPS_DEBUG("vhost_set_features failed");
> --
> 2.19.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM
2020-02-26 7:11 ` Michael S. Tsirkin
@ 2020-02-26 7:20 ` Jason Wang
2020-02-26 8:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2020-02-26 7:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: pasic, qemu-devel
----- Original Message -----
> On Wed, Feb 26, 2020 at 03:06:47PM +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 <pasic@linux.ibm.com>
> > Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > 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..6e12c3d2de 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_has_feature(dev->acked_features,
> > VIRTIO_F_IOMMU_PLATFORM);
> > }
> >
> > static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
>
> Why check acked_features and not host features here?
> I'd worry that if we do it like this, userspace driver
> within guest can clear the feature and make device access
> memory directly.
Right, host_features should be more than enough.
>
> > @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev
> > *dev,
> > if (enable_log) {
> > features |= 0x1ULL << VHOST_F_LOG_ALL;
> > }
> > + if (dev->vdev->dma_as == &address_space_memory) {
> > + features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> > + }
>
>
> That's a guest visible change. Which seems at best unnecessary.
>
I don't get how this can be visible from guest? It works as F_LOG_ALL.
Thanks
> > r = dev->vhost_ops->vhost_set_features(dev, features);
> > if (r < 0) {
> > VHOST_OPS_DEBUG("vhost_set_features failed");
> > --
> > 2.19.1
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM
2020-02-26 7:20 ` Jason Wang
@ 2020-02-26 8:12 ` Michael S. Tsirkin
2020-02-26 9:09 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2020-02-26 8:12 UTC (permalink / raw)
To: Jason Wang; +Cc: pasic, qemu-devel
On Wed, Feb 26, 2020 at 02:20:36AM -0500, Jason Wang wrote:
>
>
> ----- Original Message -----
> > On Wed, Feb 26, 2020 at 03:06:47PM +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 <pasic@linux.ibm.com>
> > > Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > 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..6e12c3d2de 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_has_feature(dev->acked_features,
> > > VIRTIO_F_IOMMU_PLATFORM);
> > > }
> > >
> > > static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
> >
> > Why check acked_features and not host features here?
> > I'd worry that if we do it like this, userspace driver
> > within guest can clear the feature and make device access
> > memory directly.
>
> Right, host_features should be more than enough.
>
> >
> > > @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev
> > > *dev,
> > > if (enable_log) {
> > > features |= 0x1ULL << VHOST_F_LOG_ALL;
> > > }
> > > + if (dev->vdev->dma_as == &address_space_memory) {
> > > + features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> > > + }
> >
> >
> > That's a guest visible change. Which seems at best unnecessary.
> >
>
> I don't get how this can be visible from guest? It works as F_LOG_ALL.
>
> Thanks
Oh you are right.
So just call vhost_dev_has_iommu here too?
> > > r = dev->vhost_ops->vhost_set_features(dev, features);
> > > if (r < 0) {
> > > VHOST_OPS_DEBUG("vhost_set_features failed");
> > > --
> > > 2.19.1
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM
2020-02-26 8:12 ` Michael S. Tsirkin
@ 2020-02-26 9:09 ` Jason Wang
0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2020-02-26 9:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: pasic, qemu-devel
On 2020/2/26 下午4:12, Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2020 at 02:20:36AM -0500, Jason Wang wrote:
>>
>> ----- Original Message -----
>>> On Wed, Feb 26, 2020 at 03:06:47PM +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 <pasic@linux.ibm.com>
>>>> Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>> 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..6e12c3d2de 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_has_feature(dev->acked_features,
>>>> VIRTIO_F_IOMMU_PLATFORM);
>>>> }
>>>>
>>>> static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
>>> Why check acked_features and not host features here?
>>> I'd worry that if we do it like this, userspace driver
>>> within guest can clear the feature and make device access
>>> memory directly.
>> Right, host_features should be more than enough.
>>
>>>> @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev
>>>> *dev,
>>>> if (enable_log) {
>>>> features |= 0x1ULL << VHOST_F_LOG_ALL;
>>>> }
>>>> + if (dev->vdev->dma_as == &address_space_memory) {
>>>> + features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
>>>> + }
>>>
>>> That's a guest visible change. Which seems at best unnecessary.
>>>
>> I don't get how this can be visible from guest? It works as F_LOG_ALL.
>>
>> Thanks
> Oh you are right.
> So just call vhost_dev_has_iommu here too?
That should work.
Thanks
>
>>>> r = dev->vhost_ops->vhost_set_features(dev, features);
>>>> if (r < 0) {
>>>> VHOST_OPS_DEBUG("vhost_set_features failed");
>>>> --
>>>> 2.19.1
>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-26 9:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-26 7:06 [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM Jason Wang
2020-02-26 7:11 ` Michael S. Tsirkin
2020-02-26 7:20 ` Jason Wang
2020-02-26 8:12 ` Michael S. Tsirkin
2020-02-26 9:09 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).