[PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Jason Wang]

Hi:

We currently try to enable device IOTLB when iommu_platform is
set. This may lead unnecessary trasnsactions between qemu and vhost
when vIOMMU is not used (which is the typical case for the encrypted
VM).

So patch tries to use transport specific method to detect the enalbing
of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.

Please review.

Thanks

Jason Wang (3):
  virtio-bus: introduce iommu_enabled()
  virtio-pci: implement iommu_enabled()
  vhost: correctly detect the enabling IOMMU

 hw/virtio/vhost.c              |  2 +-
 hw/virtio/virtio-bus.c         | 14 ++++++++++++++
 hw/virtio/virtio-pci.c         | 14 ++++++++++++++
 include/hw/virtio/virtio-bus.h |  4 +++-
 4 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/3] virtio-bus: introduce iommu_enabled()

[Jason Wang]

This patch introduce a new method for the virtio-bus for the transport
to report whether or not the IOMMU is enabled for the device.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-bus.c         | 14 ++++++++++++++
 include/hw/virtio/virtio-bus.h |  4 +++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 859978d248..d23db98c56 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -325,6 +325,20 @@ static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
     return NULL;
 }
 
+bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+
+    if (!klass->iommu_enabled) {
+        return false;
+    }
+
+    return klass->iommu_enabled(qbus->parent);
+}
+
 static void virtio_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *bus_class = BUS_CLASS(klass);
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index ef8abe49c5..7ab8c9dab0 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -93,6 +93,7 @@ struct VirtioBusClass {
      */
     bool has_variable_vring_alignment;
     AddressSpace *(*get_dma_as)(DeviceState *d);
+    bool (*iommu_enabled)(DeviceState *d);
 };
 
 struct VirtioBusState {
@@ -154,5 +155,6 @@ void virtio_bus_release_ioeventfd(VirtioBusState *bus);
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
 /* Tell the bus that the ioeventfd handler is no longer required. */
 void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n);
-
+/* Whether the IOMMU is enabled for this device */
+bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev);
 #endif /* VIRTIO_BUS_H */
-- 
2.25.1

[PATCH 2/3] virtio-pci: implement iommu_enabled()

[Jason Wang]

This patch implements the PCI transport version of iommu_enabled. This
is done by comparing the address space returned by
pci_device_iommu_address_space() against address_space_memory.

Note that an ideal approach is to use pci_device_iommu_address_space()
in get_dma_as(), but it might not work well since the IOMMU could be
initialized after the virtio-pci device is initialized.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-pci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b321604d9b..050af2517b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1110,6 +1110,19 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d)
     return pci_get_address_space(dev);
 }
 
+static bool virtio_pci_iommu_enabled(DeviceState *d)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+    PCIDevice *dev = &proxy->pci_dev;
+    AddressSpace *dma_as = pci_device_iommu_address_space(dev);
+
+    if (dma_as == &address_space_memory) {
+        return false;
+    }
+
+    return true;
+}
+
 static bool virtio_pci_queue_enabled(DeviceState *d, int n)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
@@ -2173,6 +2186,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
     k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
     k->get_dma_as = virtio_pci_get_dma_as;
+    k->iommu_enabled = virtio_pci_iommu_enabled;
     k->queue_enabled = virtio_pci_queue_enabled;
 }
 
-- 
2.25.1

[PATCH 3/3] vhost: correctly detect the enabling IOMMU

[Jason Wang]

Vhost used to compare the dma_as against the address_space_memory to
detect whether the IOMMU is enabled or not. This might not work well
since the virito-bus may call get_dma_as if VIRTIO_F_IOMMU_PLATFORM is
set without an actual IOMMU enabled when device is plugged. In the
case of PCI where pci_get_address_space() is used, the bus master as
is returned. So vhost actually tries to enable device IOTLB even if
the IOMMU is not enabled. This will lead a lots of unnecessary
transactions between vhost and Qemu and will introduce a huge drop of
the performance.

For PCI, an ideal approach is to use pci_device_iommu_address_space()
just for get_dma_as. But Qemu may choose to initialize the IOMMU after
the virtio-pci which lead a wrong address space is returned during
device plugged. So this patch switch to use transport specific way via
iommu_enabled() to detect the IOMMU during vhost start. In this case,
we are fine since we know the IOMMU is initialized correctly.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7b7bde7657..d2097b7423 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -286,7 +286,7 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
      * 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 &&
+    return virtio_bus_device_iommu_enabled(vdev) &&
            virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
 }
 
-- 
2.25.1

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Chao Gao]

On Wed, Aug 04, 2021 at 11:48:00AM +0800, Jason Wang wrote:
>Hi:
>
>We currently try to enable device IOTLB when iommu_platform is
>set. This may lead unnecessary trasnsactions between qemu and vhost
>when vIOMMU is not used (which is the typical case for the encrypted
>VM).
>
>So patch tries to use transport specific method to detect the enalbing
>of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.
>
>Please review.

Tested-by: Chao Gao <chao.gao@intel.com>

Tested with TDX; this series fixes the performance issue we saw in a TD
when vhost was enabled.

Thanks
Chao

>
>Thanks
>
>Jason Wang (3):
>  virtio-bus: introduce iommu_enabled()
>  virtio-pci: implement iommu_enabled()
>  vhost: correctly detect the enabling IOMMU
>
> hw/virtio/vhost.c              |  2 +-
> hw/virtio/virtio-bus.c         | 14 ++++++++++++++
> hw/virtio/virtio-pci.c         | 14 ++++++++++++++
> include/hw/virtio/virtio-bus.h |  4 +++-
> 4 files changed, 32 insertions(+), 2 deletions(-)
>
>-- 
>2.25.1
>

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Peter Xu]

On Wed, Aug 04, 2021 at 11:48:00AM +0800, Jason Wang wrote:
> Hi:
> 
> We currently try to enable device IOTLB when iommu_platform is
> set. This may lead unnecessary trasnsactions between qemu and vhost
> when vIOMMU is not used (which is the typical case for the encrypted
> VM).
> 
> So patch tries to use transport specific method to detect the enalbing
> of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.

Just to mention that there's also the ordering requirement for e.g. vfio-pci
and the iommu device so far because vfio_realize() depends on vIOMMU being
realized too, so specifying "-device vfio-pci" before "-device intel-iommu"
will stop working, I think (see the similar pci_device_iommu_address_space()
call in vfio_realize()).

Do you think vhost can do the same to assume vIOMMU must be specified before
vhost?  Then vhost can call pci_device_iommu_address_space() freely.  It'll be
more gentle for vhost even when it's used wrong: instead of not working at all
(vfio-pci), vhost runs slower.

Currently libvirt should be able to guarantee that ordering so libvirt users
shouldn't need to bother.  I think libvirt should also guarantee the vIOMMU
device to be created before all the rest devices, including virtio/vhost.  But
need to check.  If that's the case libvirt will naturally work for vhost too.

For the long term we may need to think about making device creation to be not
ordered by user cmdline input but still has a priority specified in the code
itself.  Migration has something like that (MigrationPriority).  I think we
just need something similar for general device realizations.  Since vhost
raised the same need, I think that priority should bump up too.

The other concern is right now vhost has vhost_dev.dma_as but now we're not
using it for vhost_dev_has_iommu().  It's just a bit confusing as when to use
which.

What do you think?

Thanks,

-- 
Peter Xu

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Jason Wang]

在 2021/8/5 上午12:08, Peter Xu 写道:
> On Wed, Aug 04, 2021 at 11:48:00AM +0800, Jason Wang wrote:
>> Hi:
>>
>> We currently try to enable device IOTLB when iommu_platform is
>> set. This may lead unnecessary trasnsactions between qemu and vhost
>> when vIOMMU is not used (which is the typical case for the encrypted
>> VM).
>>
>> So patch tries to use transport specific method to detect the enalbing
>> of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.
> Just to mention that there's also the ordering requirement for e.g. vfio-pci
> and the iommu device so far because vfio_realize() depends on vIOMMU being
> realized too, so specifying "-device vfio-pci" before "-device intel-iommu"
> will stop working, I think (see the similar pci_device_iommu_address_space()
> call in vfio_realize()).


Right, I actually try to google and end up with one patch that you 
posted to make sure vtd is initialized first.


>
> Do you think vhost can do the same to assume vIOMMU must be specified before
> vhost?


See below and I think it's not user friendly. I think maybe there should 
a general way to handle the order/dependency of device initialization in 
Qemu. But until that is implemented, I tend to use the "workaround" like 
this.


> Then vhost can call pci_device_iommu_address_space() freely.  It'll be
> more gentle for vhost even when it's used wrong: instead of not working at all
> (vfio-pci), vhost runs slower.


It's not about the slower, if pci_device_iommu_address_space() is used 
we will end up a wrong dma_as which breaks virtio DMA.


>
> Currently libvirt should be able to guarantee that ordering so libvirt users
> shouldn't need to bother.  I think libvirt should also guarantee the vIOMMU
> device to be created before all the rest devices, including virtio/vhost.  But
> need to check.  If that's the case libvirt will naturally work for vhost too.
>
> For the long term we may need to think about making device creation to be not
> ordered by user cmdline input but still has a priority specified in the code
> itself.


I fully agree.


>   Migration has something like that (MigrationPriority).  I think we
> just need something similar for general device realizations.  Since vhost
> raised the same need, I think that priority should bump up too.


Yes.


> The other concern is right now vhost has vhost_dev.dma_as but now we're not
> using it for vhost_dev_has_iommu().


If I understand correctly, both of us means using 
pci_device_iommu_address_space() in get_dma_as. If this is true, it's 
not he vhost_dev.dma_as but virtio_dev.dma_as.

So it breaks virtio actually (see above).



>    It's just a bit confusing as when to use
> which.


Yes, but I don't think a better approach.

Thanks


>
> What do you think?
>
> Thanks,
>

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Peter Xu]

On Thu, Aug 05, 2021 at 09:46:10AM +0800, Jason Wang wrote:
> > For the long term we may need to think about making device creation to be not
> > ordered by user cmdline input but still has a priority specified in the code
> > itself.
> 
> I fully agree.

I'll see whether I can work on that in the near future.  Before that, no
intention to block this series. :)

Acked-by: Peter Xu <peterx@redhat.com>

Will keep you in the loop if there will be something.  Thanks,

-- 
Peter Xu

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Caculo, Sriyash]

[AMD Official Use Only]

Tested-by: Sriyash Caculo <sriyash.caculo@amd.com>

________________________________________
From: Peter Xu <peterx@redhat.com>
Sent: Thursday, August 5, 2021 8:38 AM
To: Jason Wang
Cc: qemu-devel@nongnu.org; chao.gao@intel.com; mst@redhat.com; pbonzini@redhat.com; dgilbert@redhat.com; Huang2, Wei; Caculo, Sriyash
Subject: Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[CAUTION: External Email]

On Thu, Aug 05, 2021 at 09:46:10AM +0800, Jason Wang wrote:
> > For the long term we may need to think about making device creation to be not
> > ordered by user cmdline input but still has a priority specified in the code
> > itself.
>
> I fully agree.

I'll see whether I can work on that in the near future.  Before that, no
intention to block this series. :)

Acked-by: Peter Xu <peterx@redhat.com>

Will keep you in the loop if there will be something.  Thanks,

--
Peter Xu

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Jason Wang]

在 2021/8/4 上午11:48, Jason Wang 写道:
> Hi:
>
> We currently try to enable device IOTLB when iommu_platform is
> set. This may lead unnecessary trasnsactions between qemu and vhost
> when vIOMMU is not used (which is the typical case for the encrypted
> VM).
>
> So patch tries to use transport specific method to detect the enalbing
> of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.
>
> Please review.


Hi Michael:

Does this looks good for you? If yes, would you like to pick this series?

Thanks


>
> Thanks
>
> Jason Wang (3):
>    virtio-bus: introduce iommu_enabled()
>    virtio-pci: implement iommu_enabled()
>    vhost: correctly detect the enabling IOMMU
>
>   hw/virtio/vhost.c              |  2 +-
>   hw/virtio/virtio-bus.c         | 14 ++++++++++++++
>   hw/virtio/virtio-pci.c         | 14 ++++++++++++++
>   include/hw/virtio/virtio-bus.h |  4 +++-
>   4 files changed, 32 insertions(+), 2 deletions(-)
>