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....@intel.com/ >> >> >> Reported-by: Eric Auger <eric.au...@redhat.com> >> Fixes: b68ba1ca57677acf870d5ab10579e6105c1f5338 >> Reviewed-by: Eric Auger <eric.au...@redhat.com> >> Tested-by: Eric Auger <eric.au...@redhat.com> >> Signed-off-by: Peter Xu <pet...@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 >