Re: [RFC PATCH 0/3] VFIO: Report IOMMU fault event to userspace

["Lan, Tianyu" <tianyu.lan@intel.com>]

Hi Alex:
Does following comments make sense to you? In the previous discussion,
the type1 IOMMU driver isn't suitable for dynamic map/umap and we should
extend type1 or introduce type2. For fault event reporting or future
IOMMU related function, we need to figure out they should be in the
vfio-pci, vfio-IOMMU driver or something else. SVM support in VM also
will face such kind of choice. As Jean-Philippe posted SVM support for
ARM, I think most platforms have such requirement. Thanks.



On 2017年02月21日 13:29, Alex Williamson wrote:
> On Tue, 21 Feb 2017 12:49:18 +0800
> Lan Tianyu <tianyu.lan@intel.com> wrote:
>
>> On 2017年02月21日 04:53, Alex Williamson wrote:
>>> On Sun, 19 Feb 2017 22:47:06 +0800
>>> Lan Tianyu <tianyu.lan@intel.com> wrote:
>>>
>>>> This patchset proposes a solution to report hardware IOMMU fault
>>>> event to userspace. Motivation is to make vIOMMU in Qemu get physical
>>>> fault event and info from IOMMU hardware. vIOMMU is in charge of transforming
>>>> fault info and inject to guest. The feature is also very important to
>>>> support first level translation(Translation request with PASID) in VM
>>>> which requires vIOMMU to inject device page request to VM.
>>>
>>> Is the type1 IOMMU backend even a valid target here? vIOMMU support
>>> with type1 is functional, but not really usable except for very
>>> specific cases.  Type1 is not designed for and not well suited to
>>> dynamic DMA mapping in a VM, which means that anything other than
>>> iommu=pt or assigning the device to a VM user or l2 guest is going to
>>> have horrible performance. Faulting in mappings, as seems to be
>>> enabled here, sort of implies dynmaic mapping, right?
>>
>> Peter's patches have enabled vIOMMU IOVA with assigned device and guest
>> may change vIOMMU's IOVA page table dynamically. For example, if we
>> assign NIC and enable DMA protection in VM, NIC driver will dynamically
>> map/unmap DMA memory when receive/send packages and change vIOMMU IOVA
>> page table.
>
> Yes, it's functional, but does it have sufficient performance to bother
> to further extend it for the vIOMMU use case?  The benefit of the
> current level of vfio/viommu integration is that we can a) make use of
> DPDK with assigned devices in a secure mode within the guest, and b)
> use nested device assignment (though the usefulness of this this
> versus simple novelty is is questionable).  Both of these make use of
> relatively static mappings through vfio and therefore should not
> experience much mapping overhead aside from setup and teardown.


The fault event introduced by this patchset only works for vIOMMU's
IOVA(request without PASID) and it targets use cases just like you
mentioned(static mapping). IOVA doesn't support device page request and
so it doesn't introduce more dynamic mapping. These fault events are
triggered by misoperation of IO page table in VM.

>
> As I understand your use case, particularly with PASID, you're trying
> to enable the device to fault in mappings, which implies a dynamic
> mapping use case.  Run a 10GBps NIC (or 40 or 100) in a viommu guest
> _without_ using iommu=pt.  How close do you get to native performance?
> How close can a virtio device get in the same configuration?  What is
> the usefulness in extending type1 to support piommu faults if it
> doesn't have worthwhile performance?

For SVM(called first level translation in VTD spec), it doesn't require
to shadow all first level page table. We just need to shadow gPASID
table pointer and put into extend context entry of pIOMMU. VTD hardware
can read guest first level page table directly because it supports
nested translation for request with PASID which helps to translate GPA
to HPA during reading guest page table. So for SVM, we don't need to go 
through typ1 map/umap and dynamic map/umap will just happen in the guest.

For detail, please see Yi's [RFC Design Doc v2] Enable Shared Virtual
Memory feature
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg03617.html


>
>>> Our container
>>> based accounting falls apart for type1 backing a vIOMMU as well.  Each
>>> device lives in its own address space and therefore vfio container,
>>> which means that management of vIOMMU assigned device VMs needs to
>>> account for locked memory limits per device.  This quickly opens a
>>> large vulnerability if a VM can lock multiples of the VM memory size.
>>
>> This seems we have already this issue if we use assigned device with
>> vIOMMU regardless of whether we report pIOMMU fault event to vIOMMU or
>> not, right?
>
> Yes, so why continue to push viommu features into an iommu model that
> has these limitations?  Perhaps type1 can be extended to avoid them,
> perhaps a new "type2" iommu backend is a better solution.  Either way I
> don't see how adding iommu fault handling to type1 as it exists today
> is more than just a proof of concept exercise.

As mentioned above, these fault events won't make the situation worse.
I am not sure whether we still need to extend type1 or introduce type2?
If yes, could you give more guides how to do that. Thanks.

>
>>>
>>> So is what you're developing here just a proof of concept using type1,
>>> or is this something that you feel actually has value long term?
>>
>> We want to pass physical IOMMU fault event to vIOMMU and then inject
>> virtual fault event to VM. It's necessary to create the channel between
>> pIOMMU driver and vIOMMU. Currently, IOMMU map/umap requests go through
>> VFIO IOMMU type1 driver and so I supposed VFIO IOMMU type1 was better
>> place. This patchset is to show the idea. Another option is to go though
>> VFIO-PCI driver we have considered. It's very helpful if you can give
>> some guides here :)
>
> I understand, but is this any more than a functional proof of concept?
> What are your performance requirements?  Does type1 meet them?  Have
> you tested?  My expectation is that dynamic DMA mapping performance
> through type1 with a viommu configured for dynamic mappings is abysmal
> and it's therefore a poor target on which to base additional features
> such as fault handling.  Do you have data to suggest otherwise?  Going
> through the device rather than the container doesn't solve my assertion
> that type1 is merely functional and does not provide worthwhile
> performance when used in conjunction with dynamic mappings.  Thanks,

Above.

>
> Alex
>
>>>> Introduce two new VFIO cmd for VFIO IOMMU type1 driver
>>>> - VFIO_IOMMU_SET_FAULT_EVENT_FD
>>>>   Receive eventfd from user space and trigger eventfd when get fault event
>>>> from IOMMU.
>>>>
>>>> - VFIO_IOMMU_GET_FAULT_INFO
>>>>   Return IOMMU fault info to userspace.
>>>>
>>>> VFIO IOMMU Type1 driver needs to register IOMMU fault event notifier
>>>> to IOMMU driver for assigned devices when they are attached to VFIO
>>>> container. IOMMU driver will run VFIO fault event callback when hardware
>>>> IOMMU triggers fault events for devices assigned to VFIO container. Then,
>>>> type1 driver signals eventfd provided by userspace and usrspace gets fault
>>>> info via new cmd.
>>>>
>>>> IOMMU interface are still in design stage and so we still can't register
>>>> IOMMU fault event notifier to IOMMU driver. This patchset is prototype
>>>> code to check whether VFIO IOMMU type1 driver is suitable place to deal
>>>> with IOMMU fault event and just passes build test.
>>>>
>>>> Very appreciate for comments.
>>>>
>>>> Lan Tianyu (3):
>>>>   VFIO: Add new cmd to receive eventfd from userspace to notify IOMMU
>>>>     fault event
>>>>   VFIO: Add IOMMU fault notifier callback
>>>>   VFIO: Add new cmd for user space to get IOMMU fault info
>>>>
>>>>  drivers/vfio/vfio_iommu_type1.c | 106 ++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/iommu.h           |   7 +++
>>>>  include/uapi/linux/vfio.h       |  37 ++++++++++++++
>>>>  3 files changed, 150 insertions(+)
>>>>
>>>
>>
>>
>


-- 
Best regards
Tianyu Lan