Re: [PATCH] x86-iommu: Fail flag registration of DEVIOTLB if DT not supported

[Eric Auger <eric.auger@redhat.com>]

Hi Peter,

On 10/18/22 23:54, Peter Xu wrote:
> All x86 IOMMUs need to fail DEVIOTLB notification registrations if DT is
> not supported.  Otherwise any device (e.g. vhost) can register with
> DEVIOTLB but it'll never receive the notifications, causing device not
> operational.
>
> Fixes: b68ba1ca57 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP IOMMUTLBNotificationType")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/amd_iommu.c   | 8 ++++++++
>  hw/i386/intel_iommu.c | 7 +++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 725f69095b..42486e790f 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1476,6 +1476,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                                             Error **errp)
>  {
>      AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(as->iommu_state);
>  
>      if (new & IOMMU_NOTIFIER_MAP) {
>          error_setg(errp,
> @@ -1484,6 +1485,13 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                     PCI_FUNC(as->devfn));
>          return -EINVAL;
>      }
> +
> +    if ((new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) && !x86_iommu->dt_supported) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "Device-iotlb not declared support for vIOMMU");
with current vhost code, vhost will then silently fallbac to UNMAP
notifier registration and this will succeed. It would be nice to clarify
whether the vIOMMU works with vhost in this downgraded mode (at least
ats=off and device-ioltb=off)?

Thanks

Eric
> +        return -ENOTSUP;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 6524c2ee32..a07d9631b0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3057,6 +3057,7 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
>      IntelIOMMUState *s = vtd_as->iommu_state;
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>  
>      /* TODO: add support for VFIO and vhost users */
>      if (s->snoop_control) {
> @@ -3065,6 +3066,12 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>          return -ENOTSUP;
>      }
>  
> +    if ((new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) && !x86_iommu->dt_supported) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "Device-iotlb not declared support for vIOMMU");
> +        return -ENOTSUP;
> +    }
> +
>      /* Update per-address-space notifier flags */
>      vtd_as->notifier_flags = new;
>