Re: [PATCH] vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support

[Auger Eric]

Hi,

On 2/5/21 4:16 AM, Jason Wang wrote:
> 
> On 2021/2/5 上午3:12, Peter Xu wrote:
>> Previous work on dev-iotlb message broke vhost on either SMMU
> 
> 
> Have a quick git grep and it looks to me v3 support ATS and have command
> for device iotlb (ATC) invalidation.


Yes I will do that. Should not be a big deal.
> 
> 
>> or virtio-iommu
>> since dev-iotlb (or PCIe ATS)
> 
> 
> We may need to add this in the future.
added Jean-Philippe in CC
> 
> 
>> is not yet supported for those archs.
> 
> 
> Rethink about this, it looks to me the point is that we should make
> vhost work when ATS is disabled like what ATS spec defined:
> 
> """
> 
> ATS is enabled through a new Capability and associated configuration
> structure.  To enable 15 ATS, software must detect this Capability and
> enable the Function to issue ATS TLP.  If a Function is not enabled, the
> Function is required not to issue ATS Translation Requests and is
> required to issue all DMA Read and Write Requests with the TLP AT field
> set to “untranslated.”
> 
> """
> 
> Maybe we can add this in the commit log.
> 
> 
>>
>> An initial idea is that we can let IOMMU to export this information to
>> vhost so
>> that vhost would know whether the vIOMMU would support dev-iotlb, then
>> vhost
>> can conditionally register to dev-iotlb or the old iotlb way.  We can
>> work
>> based on some previous patch to introduce PCIIOMMUOps as Yi Liu
>> proposed [1].
>>
>> However it's not as easy as I thought since vhost_iommu_region_add()
>> does not
>> have a PCIDevice context at all since it's completely a backend.  It
>> seems
>> non-trivial to pass over a PCI device to the backend during init. 
>> E.g. when
>> the IOMMU notifier registered hdev->vdev is still NULL.
>>
>> To make the fix smaller and easier, this patch goes the other way to
>> leverage
>> the flag_changed() hook of vIOMMUs so that SMMU and virtio-iommu can
>> trap the
>> dev-iotlb registration and fail it.  Then vhost could try the fallback
>> solution
>> as using UNMAP invalidation for it's translations.
>>
>> [1]
>> https://lore.kernel.org/qemu-devel/1599735398-6829-4-git-send-email-yi.l.liu@intel.com/
>>
>>
>> Reported-by: Eric Auger <eric.auger@redhat.com>
>> Fixes: b68ba1ca57677acf870d5ab10579e6105c1f5338
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Tested-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>   hw/arm/smmuv3.c          |  5 +++++
>>   hw/virtio/vhost.c        | 13 +++++++++++--
>>   hw/virtio/virtio-iommu.c |  5 +++++
>>   3 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 98b99d4fe8e..bd1f97000d9 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -1497,6 +1497,11 @@ static int
>> smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>>       SMMUv3State *s3 = sdev->smmu;
>>       SMMUState *s = &(s3->smmu_state);
>>   +    if (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) {
>> +        error_setg(errp, "SMMUv3 does not support dev-iotlb yet");
>> +        return -EINVAL;
>> +    }
>> +
>>       if (new & IOMMU_NOTIFIER_MAP) {
>>           error_setg(errp,
>>                      "device %02x.%02x.%x requires iommu MAP notifier
>> which is "
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 28c7d781721..6e17d631f77 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -704,6 +704,7 @@ static void vhost_iommu_region_add(MemoryListener
>> *listener,
>>       Int128 end;
>>       int iommu_idx;
>>       IOMMUMemoryRegion *iommu_mr;
>> +    int ret;
>>         if (!memory_region_is_iommu(section->mr)) {
>>           return;
>> @@ -726,8 +727,16 @@ static void vhost_iommu_region_add(MemoryListener
>> *listener,
>>       iommu->iommu_offset = section->offset_within_address_space -
>>                             section->offset_within_region;
>>       iommu->hdev = dev;
>> -    memory_region_register_iommu_notifier(section->mr, &iommu->n,
>> -                                          &error_fatal);
>> +    ret = memory_region_register_iommu_notifier(section->mr,
>> &iommu->n, NULL);
>> +    if (ret) {
>> +        /*
>> +         * Some vIOMMUs do not support dev-iotlb yet.  If so, try to
>> use the
>> +         * UNMAP legacy message
>> +         */
>> +        iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP;
>> +        memory_region_register_iommu_notifier(section->mr, &iommu->n,
>> +                                              &error_fatal);
>> +    }
>>       QLIST_INSERT_HEAD(&dev->iommu_list, iommu, iommu_next);
>>       /* TODO: can replay help performance here? */
>>   }
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 6b9ef7f6b2b..c2883a2f6c8 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -893,6 +893,11 @@ static int
>> virtio_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu_mr,
>>                                               IOMMUNotifierFlag new,
>>                                               Error **errp)
>>   {
>> +    if (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) {
>> +        error_setg(errp, "Virtio-iommu does not support dev-iotlb yet");
>> +        return -EINVAL;
>> +    }
>> +
>>       if (old == IOMMU_NOTIFIER_NONE) {
>>           trace_virtio_iommu_notify_flag_add(iommu_mr->parent_obj.name);
>>       } else if (new == IOMMU_NOTIFIER_NONE) {
> 
> 
> Patch looks good. I wonder whether we should fix intel when ATS is
> disabled.
good point

Thanks

Eric
> 
> Thanks
>