| Message ID | 20210804034803.1644-1-jasowang@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Disable vhost device IOTLB is IOMMU is not enabled | expand |
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 >
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,
在 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, >
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,
[AMD Official Use Only]
Tested-by: Sriyash Caculo <sriyash.caculo@amd.com>
在 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(-) >