From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48023) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk4Xg-0005Or-1r for qemu-devel@nongnu.org; Wed, 14 Sep 2016 03:23:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bk4Xb-0001Uo-QI for qemu-devel@nongnu.org; Wed, 14 Sep 2016 03:23:34 -0400 Received: from ozlabs.org ([103.22.144.67]:58504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk4Xb-0001UW-8I for qemu-devel@nongnu.org; Wed, 14 Sep 2016 03:23:31 -0400 Date: Wed, 14 Sep 2016 17:22:40 +1000 From: David Gibson Message-ID: <20160914072240.GP15077@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="U0B5otXy6WXfork9" Content-Disposition: inline In-Reply-To: <20160914071243.GM3776@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 --U0B5otXy6WXfork9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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, VTDAddressSpace,= iommu); > >=20 > > Shouldn't this have a sanity check that the new flags doesn't include > > 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_NOTIFIER_= ALL) { > > > + spapr_tce_notify_started(iommu); > > > + } else if (old =3D=3D IOMMU_NOTIFIER_ALL && new =3D=3D IOMMU_NOT= IFIER_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" Yes, that should be right. > in the next version if you like, but AFAICT they are totally the > same. Again, only if you assume things about how the notifiers will be used, which is not a good look when designing an interface. >=20 > >=20 > > I'd also prefer to see notify_started and notify_stopped folded into > > this function. >=20 > I can do that for v5. >=20 > Thanks, >=20 > -- peterx >=20 --=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 --U0B5otXy6WXfork9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJX2PrAAAoJEGw4ysog2bOSkKcP+gKhxrf8dH91YEcCHGQDOBSV 66t3QHUppgQvhoucj/zHkcjPlxsrN1zwUst1C++b5kJFppngX8YeYvnWZY8vcGTH wF9cuXKFL2agtg0DSP+Cn41ynZqjvniKqXywvIupRtm0EP6Hj1NH1i2QqBewT/8r 8u4CgnXfkn7wmc3yaLw5T1gwBVF84BQ9RQj0K2i7ZDfIdKpqfq3cDGBe6omS+ZgV uHIBx2OUWCz6+6ipeJxluL2SH9Dz87Ieyi8xNDb7Q234bbmZscYQ2zZUFI1vle0E lR6x8LQn7eGoqSia0R/m3zaFWdNPhS9UnB3AvCvpJMmEtglYgYCm/E7BawWS0+mz QhnDQQgUHgBuDrnSwZthntHith7oHDnLMCgir0iGJAoN7W5fVJ4qNkKVi119Wt9k 2+rEaSzjU4JdDv0jxuGdCApbLkEO3zzzriK2Px89uQC8Vq8KcVvKeOxuDE0xDdDl m3WrOt4Vsb7kcfQBBURDXBlusnkjNmN4+Clux3TjL2xKEy8d3G9P0m+4Lh9GA0+l hfFe4FtwluCJqyts4DIOXNeT8hK94zaxS+E1kqLePRlmNXXiOXBZ/x2lq0B/SdAv zAaS1oBTuaHq0zfvPoNY9mGwzPDrKNQU6vhq808ZtWaBay3JT/d0uRg+gLnn7pT+ ywMM9PFRX1zAAgSpAzgW =gmHS -----END PGP SIGNATURE----- --U0B5otXy6WXfork9--