Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode

[Eric Auger]

Hi jason,

On 11/29/22 09:10, Jason Wang wrote:
> Without dt mode, device IOTLB notifier won't work since guest won't
> send device IOTLB invalidation descriptor in this case. Let's fail
> early instead of misbehaving silently.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9143376677..d025ef2873 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3179,6 +3179,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) {
> @@ -3193,6 +3194,13 @@ static int 
> vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                           PCI_FUNC(vtd_as->devfn));
>          return -ENOTSUP;
>      }
> +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires device IOTLB mode",
maybe precise INTEL IOMMU device-IOTLB mode. otherwise this may be
confused with device ATS capability?

While thinking about those error handlings (including the SMMU ones)
nothing should really prevent you from registering a notifier that is
not signalled. Maybe we should add in the documentation that any attempt
to register an IOMMU notifier to an IOMMU MR that is not able to signal
it will return an error.

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> +                         PCI_FUNC(vtd_as->devfn));
> +        return -ENOTSUP;
> +    }
>  
>      /* Update per-address-space notifier flags */
>      vtd_as->notifier_flags = new;