From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39620) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bfHym-0007QD-Mf for qemu-devel@nongnu.org; Wed, 31 Aug 2016 22:43:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bfHyi-000300-H9 for qemu-devel@nongnu.org; Wed, 31 Aug 2016 22:43:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54600) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bfHyi-0002zJ-9O for qemu-devel@nongnu.org; Wed, 31 Aug 2016 22:43:44 -0400 Date: Wed, 31 Aug 2016 20:43:42 -0600 From: Alex Williamson Message-ID: <20160831204342.77fbe728@t450s.home> In-Reply-To: <20160901022929.GA3558@pxdev.xzpeter.org> References: <1472526419-5900-1-git-send-email-jasowang@redhat.com> <1472526419-5900-11-git-send-email-jasowang@redhat.com> <20160829213702.536df0df@t450s.home> <76fb2f05-4f9c-3e91-d301-3e0a6d4cabaf@redhat.com> <20160901022929.GA3558@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for 2.8 10/11] Revert "intel_iommu: Throw hw_error on notify_started" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: Jason Wang , mst@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, cornelia.huck@de.ibm.com, wexu@redhat.com, vkaplans@redhat.com, David Gibson [cc +dgibson] On Thu, 1 Sep 2016 10:29:29 +0800 Peter Xu wrote: > On Wed, Aug 31, 2016 at 10:45:37AM +0800, Jason Wang wrote: > >=20 > >=20 > > On 2016=E5=B9=B408=E6=9C=8830=E6=97=A5 11:37, Alex Williamson wrote: =20 > > >On Tue, 30 Aug 2016 11:06:58 +0800 > > >Jason Wang wrote: > > > =20 > > >>From: Peter Xu > > >> > > >>This reverts commit 3cb3b1549f5401dc3a5e1d073e34063dc274136f. Vhost > > >>device IOTLB API will get notified and send invalidation request to > > >>vhost through this notifier. =20 > > >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. Thank= s, > > > > > >Alex =20 > >=20 > > 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. =20 >=20 > Shall we leverage IOMMUTLBEntry.perm =3D=3D 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? >=20 > Alex, >=20 > 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. >=20 > It seems that we need a "name" for either IOMMU notifier > provider/consumer, and we should not allow (provider=3D=3DIntel && > consumer=3D=3DVFIO) 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<---------- >=20 > 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(MemoryRe= gion *iommu, hwaddr addr, > } >=20 > static const MemoryRegionIOMMUOps typhoon_iommu_ops =3D { > + .iommu_type =3D "typhoon", > .translate =3D typhoon_translate_iommu, > }; >=20 > 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); >=20 > + s->iommu_ops.iommu_type =3D "intel"; > s->iommu_ops.translate =3D vtd_iommu_translate; > s->iommu_ops.notify_started =3D vtd_iommu_notify_started; > s->root =3D 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, > } >=20 > static MemoryRegionIOMMUOps pbm_iommu_ops =3D { > + .iommu_type =3D "pbm", > .translate =3D pbm_translate_iommu, > }; >=20 > 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_tab= le =3D { > }; >=20 > static MemoryRegionIOMMUOps spapr_iommu_ops =3D { > + .iommu_type =3D "spapr", > .translate =3D spapr_tce_translate_iommu, > .get_min_page_size =3D spapr_tce_get_min_page_size, > .notify_started =3D 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(MemoryRegio= n *iommu, hwaddr addr, > } >=20 > static const MemoryRegionIOMMUOps s390_iommu_ops =3D { > + .iommu_type =3D "s390", > .translate =3D s390_translate_iommu, > }; >=20 > 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; >=20 > + if (!strcmp(memory_region_iommu_type(section->mr), "intel")) { > + error_report("Device passthrough cannot work with Intel IOMM= U"); > + 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; >=20 > 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(MemoryRegi= on *mr) > return mr->iommu_ops; > } >=20 > +/** > + * 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 ""; > +} >=20 > /** > * memory_region_iommu_get_min_page_size: get minimum supported page size >=20 > ------>8-------- =20 >=20 > Thanks, >=20 > -- peterx