From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk7nm-0000wj-HF for qemu-devel@nongnu.org; Wed, 14 Sep 2016 06:52:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bk7nj-0002UL-F9 for qemu-devel@nongnu.org; Wed, 14 Sep 2016 06:52:25 -0400 Received: from ozlabs.org ([103.22.144.67]:58212) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk7ni-0002Tp-Ou for qemu-devel@nongnu.org; Wed, 14 Sep 2016 06:52:23 -0400 Date: Wed, 14 Sep 2016 20:37:34 +1000 From: David Gibson Message-ID: <20160914103734.GQ15077@voom.fritz.box> References: <1473389864-19694-1-git-send-email-peterx@redhat.com> <1473389864-19694-3-git-send-email-peterx@redhat.com> <20160914055528.GM15077@voom.fritz.box> <20160914071243.GM3776@pxdev.xzpeter.org> <20160914072240.GP15077@voom.fritz.box> <20160914074941.GO3776@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Pes7OZCOzfZhFQfq" Content-Disposition: inline In-Reply-To: <20160914074941.GO3776@pxdev.xzpeter.org> Subject: Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: qemu-devel@nongnu.org, mst@redhat.com, jasowang@redhat.com, vkaplans@redhat.com, alex.williamson@redhat.com, wexu@redhat.com, pbonzini@redhat.com, cornelia.huck@de.ibm.com, dgibson@redhat.com --Pes7OZCOzfZhFQfq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 14, 2016 at 03:49:41PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 05:22:40PM +1000, David Gibson wrote: > > On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > > > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > >=20 > > > [...] > > >=20 > > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > > > + IOMMUNotifierFlag old, > > > > > + IOMMUNotifierFlag new) > > > > > { > > > > > VTDAddressSpace *vtd_as =3D container_of(iommu, VTDAddressSp= ace, iommu); > > > >=20 > > > > Shouldn't this have a sanity check that the new flags doesn't inclu= de > > > > MAP actions? > > >=20 > > > See your r-b for patch 3, thanks! So skipping this one. > > >=20 > > > [...] > > >=20 > > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > > > + IOMMUNotifierFlag old, > > > > > + IOMMUNotifierFlag new) > > > > > +{ > > > > > + if (old =3D=3D IOMMU_NOTIFIER_NONE && new =3D=3D IOMMU_NOTIF= IER_ALL) { > > > > > + spapr_tce_notify_started(iommu); > > > > > + } else if (old =3D=3D IOMMU_NOTIFIER_ALL && new =3D=3D IOMMU= _NOTIFIER_NONE) { > > > > > + spapr_tce_notify_stopped(iommu); > > > > > + } > > > >=20 > > > > This is wrong. We need to do the notify_start and stop actions if > > > > *any* bits are set in the new/old flags, not just if all of them are > > > > set. > > >=20 > > > Power should need both, right? I can switch all > > >=20 > > > "=3D=3D IOMMU_NOTIFIER_ALL" > > >=20 > > > into: > > >=20 > > > "!=3D IOMMU_NOTIFIER_NONE" > >=20 > > Yes, that should be right. > >=20 > > > in the next version if you like, but AFAICT they are totally the > > > same. > >=20 > > Again, only if you assume things about how the notifiers will be used, > > which is not a good look when designing an interface. >=20 > This should not be related to the interface at all? >=20 > I was based on the assumption that "Power cannot support either one of > MAP/UNMAP, but only if both exist". Huh? I have no idea what you mean by that. Power can support notifying both map and unmap events just fine - but in order to provide *any* notifications, it has to disable KVM acceleration of the guest-side IOMMU (otherwise qemu won't know about any changes to the IOMMU state). So the change you you suggested before to !=3D IOMMU_NOTIFIER_NONE is exactly correct, anything else is not. > To be more specific, we possibly > can have this at the beginning of flags_changed(): >=20 > assert(old =3D=3D IOMMU_NOTIFIER_NONE || old =3D=3D IOMMU_NOTIFIER_ALL); > assert(new =3D=3D IOMMU_NOTIFIER_NONE || new =3D=3D IOMMU_NOTIFIER_ALL); >=20 > To make sure nothing will go wrong. Hmm.. I really get the feeling you're confusing constraints of the guest side with constraints of the host side. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --Pes7OZCOzfZhFQfq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJX2ShtAAoJEGw4ysog2bOSuv4P/jiQjrb61Ntx6jQi9SvTcyoP bwy6FdldLCvANChcPADJGHM37e4Iz5DAjJytd4rbRbVg/0ANAxQPlwUX5Op842jg uOx2FOjx+E+PstoCwGMUWL/1bS4pVkUqgbsdYa4mjqf1uDCvE3xWUFg7/g/F9wvb 2SYCttUhJ6fWa6KWu+P4xpb8JgzbN4q5x3ArqLIS7TZqg3/BJtgJ2RIYpHtpAv49 I6DRT94lr2YrOmPuy3s1qwAmfPADc7VmBCsc9+7kYsp+2SplknqZVSRO43Tbck+8 DPfRKWz+1pkctX2naEE1D+i0lpzb8U/ojjDQiVKmDSO0tpeyj3wXDmYeogBuDd5I vXAV4g8FhgtxIdtxrcQoSn1vgS2cqHLnq05vJ15mOcuiBDHgG96iGGvzBFzixDKP r1yR/TZgQxSlR1HuMuYW67c0ZH7u5g2tQNONCjFafbg0WSXxo1aQfbbldmiPYGoK IWerT661xYrtCfut3fRIGtRdNZS6JRaK0fSma4UeYoi5KtGA9e8Ig/5onBp+t9PC A511HuRDoaAcyRpTWCkEbxgVTRyFkZGdA5iGciw+VNYAPZBJNMLIgYj/F0EnRFZ7 0KeOxFaziTfXeDyUvWkTK4rIAlxO6NQPlaOE4CbZd9XloeRbUCgKDJlMiA8KmI88 12p/mkOIvcsIXE7+JvVL =wcT2 -----END PGP SIGNATURE----- --Pes7OZCOzfZhFQfq--