Re: [Qemu-devel] [PATCH for 2.8 10/11] Revert "intel_iommu: Throw hw_error on notify_started"

[Alex Williamson]

[cc +dgibson]

On Thu, 1 Sep 2016 10:29:29 +0800
Peter Xu <address@hidden> wrote:

> On Wed, Aug 31, 2016 at 10:45:37AM +0800, Jason Wang wrote:
> > 
> > 
> > On 2016年08月30日 11:37, Alex Williamson wrote:  
> > >On Tue, 30 Aug 2016 11:06:58 +0800
> > >Jason Wang <address@hidden> wrote:
> > >  
> > >>From: Peter Xu <address@hidden>
> > >>
> > >>This reverts commit 3cb3b1549f5401dc3a5e1d073e34063dc274136f. Vhost
> > >>device IOTLB API will get notified and send invalidation request to
> > >>vhost through this notifier.  
> > >AFAICT this series does not address the original problem for which
> > >commit 3cb3b1549f54 was added.  We've only addressed the very narrow
> > >use case of a device iotlb firing the iommu notifier therefore this
> > >change is a regression versus 2.7 since it allows invalid
> > >configurations with a physical iommu which will never receive the
> > >necessary notifies from intel-iommu emulation to work properly.  Thanks,
> > >
> > >Alex  
> > 
> > Looking at vfio, it cares about map but vhost only cares about IOTLB
> > invalidation. Then I think we probably need another kind of notifier in this
> > case to avoid this.  
> 
> Shall we leverage IOMMUTLBEntry.perm == IOMMU_NONE as a sign for
> invalidation? If so, we can use the same IOTLB interface as before.
> IMHO these two interfaces are not conflicting?
> 
> Alex,
> 
> Do you mean we should still disallow user from passing through devices
> while Intel IOMMU enabled? If so, not sure whether patch below can
> solve the issue.
> 
> It seems that we need a "name" for either IOMMU notifier
> provider/consumer, and we should not allow (provider==Intel &&
> consumer==VFIO) happen. In the following case, I added a name for
> provider, and VFIO checks it.

Absolutely not, intel-iommu emulation is simply incomplete, the IOMMU
notifier is never called for mappings.  There's a whole aspect of
iommu notifiers that intel-iommu simply hasn't bothered to implement.
Don't punish vfio for actually making use of the interface as it was
intended to be used.  AFAICT you're implementing the unmap/invalidation
half, without the actual mapping half of the interface.  It's broken
and incompatible with any iommu notifiers that expect to see both
sides.  Thanks,

Alex


> --------8<----------
> 
> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
> index 883db13..936c2e6 100644
> --- a/hw/alpha/typhoon.c
> +++ b/hw/alpha/typhoon.c
> @@ -725,6 +725,7 @@ static IOMMUTLBEntry typhoon_translate_iommu(MemoryRegion 
> *iommu, hwaddr addr,
>  }
> 
>  static const MemoryRegionIOMMUOps typhoon_iommu_ops = {
> +    .iommu_type = "typhoon",
>      .translate = typhoon_translate_iommu,
>  };
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 28c31a2..f5e3875 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2347,6 +2347,7 @@ static void vtd_init(IntelIOMMUState *s)
>      memset(s->w1cmask, 0, DMAR_REG_SIZE);
>      memset(s->womask, 0, DMAR_REG_SIZE);
> 
> +    s->iommu_ops.iommu_type = "intel";
>      s->iommu_ops.translate = vtd_iommu_translate;
>      s->iommu_ops.notify_started = vtd_iommu_notify_started;
>      s->root = 0;
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 653e711..9cfbb73 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -323,6 +323,7 @@ static IOMMUTLBEntry pbm_translate_iommu(MemoryRegion 
> *iommu, hwaddr addr,
>  }
> 
>  static MemoryRegionIOMMUOps pbm_iommu_ops = {
> +    .iommu_type = "pbm",
>      .translate = pbm_translate_iommu,
>  };
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 6bc4d4d..e3e8739 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -244,6 +244,7 @@ static const VMStateDescription vmstate_spapr_tce_table = 
> {
>  };
> 
>  static MemoryRegionIOMMUOps spapr_iommu_ops = {
> +    .iommu_type = "spapr",
>      .translate = spapr_tce_translate_iommu,
>      .get_min_page_size = spapr_tce_get_min_page_size,
>      .notify_started = spapr_tce_notify_started,
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 9c1c04e..4414462 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -443,6 +443,7 @@ static IOMMUTLBEntry s390_translate_iommu(MemoryRegion 
> *iommu, hwaddr addr,
>  }
> 
>  static const MemoryRegionIOMMUOps s390_iommu_ops = {
> +    .iommu_type = "s390",
>      .translate = s390_translate_iommu,
>  };
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b313e7c..317e08b 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -441,6 +441,11 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      if (memory_region_is_iommu(section->mr)) {
>          VFIOGuestIOMMU *giommu;
> 
> +        if (!strcmp(memory_region_iommu_type(section->mr), "intel")) {
> +            error_report("Device passthrough cannot work with Intel IOMMU");
> +            exit(1);
> +        }
> +
>          trace_vfio_listener_region_add_iommu(iova, end);
>          /*
>           * FIXME: For VFIO iommu types which have KVM acceleration to
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 3e4d416..f012f77 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -149,6 +149,8 @@ struct MemoryRegionOps {
>  typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;
> 
>  struct MemoryRegionIOMMUOps {
> +    /* Type of IOMMU */
> +    const char *iommu_type;
>      /* Return a TLB entry that contains a given address. */
>      IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool 
> is_write);
>      /* Returns minimum supported page size */
> @@ -593,6 +595,21 @@ static inline bool memory_region_is_iommu(MemoryRegion 
> *mr)
>      return mr->iommu_ops;
>  }
> 
> +/**
> + * memory_region_iommu_type: return type of IOMMU
> + *
> + * Returns type of IOMMU, empty string ("") if not a IOMMU region.
> + *
> + * @mr: the memory region being queried
> + */
> +static inline const char *memory_region_iommu_type(MemoryRegion *mr)
> +{
> +    if (mr->iommu_ops && mr->iommu_ops->iommu_type) {
> +        return mr->iommu_ops->iommu_type;
> +    }
> +
> +    return "";
> +}
> 
>  /**
>   * memory_region_iommu_get_min_page_size: get minimum supported page size
> 
> ------>8--------  
> 
> Thanks,
> 
> -- peterx