Hi Peter,
On 10/18/22 23:56, Peter Xu wrote:
> On Tue, Oct 18, 2022 at 05:08:19PM +0200, Eric Auger wrote:
>> Hi Peter,
>>
>> On 10/18/22 16:25, Peter Xu wrote:
>>> Hi, Eric,
>>>
>>> On Tue, Oct 18, 2022 at 02:28:52PM +0200, Eric Auger wrote:
>>>> Since b68ba1ca5767 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP
>>>> IOMMUTLBNotificationType"), vhost attempts to register DEVIOTLB_UNMAP
>>>> notifier. This latter is supported by the intel-iommu which supports
>>>> device-iotlb if the corresponding option is set. Then 958ec334bca3
>>>> ("vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support") allowed
>>>> silent fallback to the legacy UNMAP notifier if the viommu does not
>>>> support device iotlb.
>>>>
>>>> Initially vhost/viommu integration was introduced with intel iommu
>>>> assuming ats=on was set on virtio-pci device and device-iotlb was set
>>>> on the intel iommu. vhost acts as an ATS capable device since it
>>>> implements an IOTLB on kernel side. However translated transactions
>>>> that hit the device IOTLB do not transit through the vIOMMU. So this
>>>> requires a limited ATS support on viommu side.
>>>>
>>>> However, in theory, if ats=on is set on a pci device, the
>>>> viommu should support ATS for that device to work.
>>> Pure question: what will happen if one ATS supported PCI device got plugged
>>> into a system whose physical IOMMU does not support ATS? Will ATS just be
>>> ignored and the device keep working simply without ATS?
>> Yes that's my understanding: in that case the ATS capable device would
>> work with ats disabled (baremetal case). In the iommu driver you can
>> have a look at the pci_enable_ats() call which is guarded by
>> info->ats_supported for instance on intel iommu.
>>
>> Following that reasoning vhost modality should not be enabled without
>> ATS support on vIOMMU side. But it is.
>>
>> In that sense I may rename the ats_enabled helpers with ats_capable?
> Sounds good to me.
OK
>
>> If I understand correctly setting ats=on exposes the ATS capability (
>> 615c4ed205 virtio-pci: address space translation service (ATS) support)
>> which is then enabled by the guest driver.
> I think it won't, as long as vIOMMU doesn't have DT support declared?
That's my assumption too
>
>>> [1]
>>>
>>> [...]
>>>
>>>> @@ -760,8 +771,16 @@ static void vhost_iommu_region_add(MemoryListener
>>>> *listener,
>>>> iommu->iommu_offset = section->offset_within_address_space -
>>>> section->offset_within_region;
>>>> iommu->hdev = dev;
>>>> - ret = memory_region_register_iommu_notifier(section->mr, &iommu->n,
>>>> NULL);
>>>> + ret = memory_region_register_iommu_notifier(section->mr, &iommu->n,
>>>> &err);
>>>> if (ret) {
>>>> + if (vhost_dev_ats_enabled(dev)) {
>>>> + error_reportf_err(err,
>>>> + "vhost cannot register DEVIOTLB_UNMAP "
>>>> + "although ATS is enabled, "
>>>> + "fall back to legacy UNMAP notifier: ");
>>> We want to use the warning message to either remind the user to (1) add the
>>> dev-iotlb=on parameter for vIOMMU, or (2) drop the ats=on on device. Am I
>>> right?
>> My focus is to warn the end user there is no support for device-iotlb
>> support in virtio-iommu or vsmmuv3 but vhost does not really require
>> it.Indeed current users of virtio-iommu/vsmmuv3 seem confused now wrt
>> vhost integration and the lack of device-iotlb option on those viommus.
>>
>> On intel I understand we would like to enforce that ats and dev-iotlb
>> are both set or unset. But this is not really addressed in that series.
>> Indeed vtd_iommu_notify_flag_changed does not reject any registration of
>> IOMMU_NOTIFIER_DEVIOTLB_UNMAP notifier in case it does not support
>> device-iotlb. I think it should.
> Yes I agree, thanks for finding it. Just posted a patch:
>
> https://lore.kernel.org/r/20221018215407.363986-1-pet...@redhat.com
OK thanks
>
>> The trouble is vhost_iommu_region_add
>> is not meant to nicely fail.
>>> As we've discussed - I remember Jason used to test with/without dev-iotlb
>>> on vhost on Intel and dev-iotlb is faster on vt-d guest driver than without
>> It would be nice to have a clarification about this. Indeed
>>
>> [PATCH v3 0/5] memory: Skip assertion in
>> memory_region_unregister_iommu_notifier
>> https://lore.kernel.org/all/20201116165506.31315-1-epere...@redhat.com/
>> mostly focussed on removing an assertion although one patch mentionned perf
>> improvements. What does make the perf better (less device iotlb flushes than
>> general iotlb flushes?)
> I'll leave that to Jason. Thanks.
OK thanks
Eric
>
>>> it. So that can make sense to me for (1). I don't know whether it helps
>>> for (2) because fundamentally it's the same question as [1] above, and
>>> whether that's a legal configuration.
>>>
>>> Thanks,
>>>
>> Adding jean in the loop too
>>
>> Thanks
>>
>> Eric
>>
