From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk3Cw-000464-J3 for qemu-devel@nongnu.org; Wed, 14 Sep 2016 01:58:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bk3Cr-0003uh-CX for qemu-devel@nongnu.org; Wed, 14 Sep 2016 01:58:05 -0400 Received: from ozlabs.org ([103.22.144.67]:46303) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk3Cq-0003rE-LI for qemu-devel@nongnu.org; Wed, 14 Sep 2016 01:58:01 -0400 Date: Wed, 14 Sep 2016 15:55:28 +1000 From: David Gibson Message-ID: <20160914055528.GM15077@voom.fritz.box> References: <1473389864-19694-1-git-send-email-peterx@redhat.com> <1473389864-19694-3-git-send-email-peterx@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="W4pDZ/VvazBYHhxQ" Content-Disposition: inline In-Reply-To: <1473389864-19694-3-git-send-email-peterx@redhat.com> 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 --W4pDZ/VvazBYHhxQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 09, 2016 at 10:57:43AM +0800, Peter Xu wrote: > The new interface can be used to replace the old notify_started() and > notify_stopped(). Meanwhile it provides explicit flags so that IOMMUs > can know what kind of notifications it is requested for. >=20 > Signed-off-by: Peter Xu > --- > hw/i386/intel_iommu.c | 6 ++++-- > hw/ppc/spapr_iommu.c | 14 ++++++++++++-- > include/exec/memory.h | 9 +++++---- > memory.c | 29 +++++++++++++++++++++-------- > 4 files changed, 42 insertions(+), 16 deletions(-) >=20 > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 28c31a2..9d49be7 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1974,7 +1974,9 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegi= on *iommu, hwaddr addr, > return ret; > } > =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, iom= mu); Shouldn't this have a sanity check that the new flags doesn't include MAP actions? > @@ -2348,7 +2350,7 @@ static void vtd_init(IntelIOMMUState *s) > memset(s->womask, 0, DMAR_REG_SIZE); > =20 > s->iommu_ops.translate =3D vtd_iommu_translate; > - s->iommu_ops.notify_started =3D vtd_iommu_notify_started; > + s->iommu_ops.notify_flag_changed =3D vtd_iommu_notify_flag_changed; > s->root =3D 0; > s->root_extended =3D false; > s->dmar_enabled =3D false; > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 6bc4d4d..313f53f 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -166,6 +166,17 @@ static void spapr_tce_notify_stopped(MemoryRegion *i= ommu) > spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), f= alse); > } > =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_NOTIFIE= R_NONE) { > + spapr_tce_notify_stopped(iommu); > + } 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. I'd also prefer to see notify_started and notify_stopped folded into this function. > +} > + > static int spapr_tce_table_post_load(void *opaque, int version_id) > { > sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(opaque); > @@ -246,8 +257,7 @@ static const VMStateDescription vmstate_spapr_tce_tab= le =3D { > static MemoryRegionIOMMUOps spapr_iommu_ops =3D { > .translate =3D spapr_tce_translate_iommu, > .get_min_page_size =3D spapr_tce_get_min_page_size, > - .notify_started =3D spapr_tce_notify_started, > - .notify_stopped =3D spapr_tce_notify_stopped, > + .notify_flag_changed =3D spapr_tce_notify_flag_changed, > }; > =20 > static int spapr_tce_table_realize(DeviceState *dev) > diff --git a/include/exec/memory.h b/include/exec/memory.h > index e69e984..e04b752 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -174,10 +174,10 @@ struct MemoryRegionIOMMUOps { > IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is= _write); > /* Returns minimum supported page size */ > uint64_t (*get_min_page_size)(MemoryRegion *iommu); > - /* Called when the first notifier is set */ > - void (*notify_started)(MemoryRegion *iommu); > - /* Called when the last notifier is removed */ > - void (*notify_stopped)(MemoryRegion *iommu); > + /* Called when IOMMU Notifier flag changed */ > + void (*notify_flag_changed)(MemoryRegion *iommu, > + IOMMUNotifierFlag old_flags, > + IOMMUNotifierFlag new_flags); > }; > =20 > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -223,6 +223,7 @@ struct MemoryRegion { > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > QLIST_HEAD(, IOMMUNotifier) iommu_notify; > + IOMMUNotifierFlag iommu_notify_flags; > }; > =20 > /** > diff --git a/memory.c b/memory.c > index f65c600..4faf1d9 100644 > --- a/memory.c > +++ b/memory.c > @@ -1419,6 +1419,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > mr->iommu_ops =3D ops, > mr->terminates =3D true; /* then re-forwards */ > QLIST_INIT(&mr->iommu_notify); > + mr->iommu_notify_flags =3D IOMMU_NOTIFIER_NONE; > } > =20 > static void memory_region_finalize(Object *obj) > @@ -1513,16 +1514,31 @@ bool memory_region_is_logging(MemoryRegion *mr, u= int8_t client) > return memory_region_get_dirty_log_mask(mr) & (1 << client); > } > =20 > +static void memory_region_update_iommu_notify_flags(MemoryRegion *mr) > +{ > + IOMMUNotifierFlag flags =3D IOMMU_NOTIFIER_NONE; > + IOMMUNotifier *iommu_notifier; > + > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + flags |=3D iommu_notifier->notifier_flags; > + } > + > + if (flags !=3D mr->iommu_notify_flags && > + mr->iommu_ops->notify_flag_changed) { > + mr->iommu_ops->notify_flag_changed(mr, mr->iommu_notify_flags, > + flags); > + } > + > + mr->iommu_notify_flags =3D flags; > +} > + > void memory_region_register_iommu_notifier(MemoryRegion *mr, > IOMMUNotifier *n) > { > /* We need to register for at least one bitfield */ > assert(n->notifier_flags !=3D IOMMU_NOTIFIER_NONE); > - if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_started(mr); > - } > QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > + memory_region_update_iommu_notify_flags(mr); > } > =20 > uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) > @@ -1560,10 +1576,7 @@ void memory_region_unregister_iommu_notifier(Memor= yRegion *mr, > IOMMUNotifier *n) > { > QLIST_REMOVE(n, node); > - if (mr->iommu_ops->notify_stopped && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_stopped(mr); > - } > + memory_region_update_iommu_notify_flags(mr); > } > =20 > void memory_region_notify_iommu(MemoryRegion *mr, --=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 --W4pDZ/VvazBYHhxQ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJX2OZQAAoJEGw4ysog2bOSziQP/1sgQmd9bPTY0TySaX3N9OoK jhLwjhwXFpK40b8ZSy20MT0QnPd/u9GnIXIe+ZUWp2mOukEzzFrPjLo7eQf9X3Z0 +iI1EPjteoNlZJINKc4ddgWMkkxyvDOfbxZVmICmoobqCJgtno7wZ53V9hqBGxx4 pI1Xfdb8ufdC4WftnbrKSDeXzlh5xhmjbPhtSZkJh55BzDTqvdVOtaul+ww/6mbL C7Pc9+sIbkGgx0yeSjVFHAHgElK3cVmfrGSbHMNE2Y1SgK6wWAToO+Mcn+mlicsL Z5SjXwOnzqcjN1FX2fg8A5ukYa8z5TEhTy6k8FztlHd4gixsSRfhQRKZzfmcBF96 4hCS2w4cP3y84VZOxWSD2oRUw1YHWydy+WJ8cZOIggposZZyNuRfX81h9b1tuIAN XcZ076FO9Ptcnonmm0LsV8QUWD28HKXhGKQqD5yfkdj8uxQxMgyj76fDfciUJSAs RlFfub73b83evUcuoIsnMRyVTJQG64NqrmnvG2sa9QIOWPrfx+zHD4YIMsW6jxEE nZWbo7w5eo8h3D2KelsfFA+OwX2ai8/LlIVPnRsQ0fKyxOpavFiJIK1kEpzjm6jm znK2XgPCY7YhfVYdx91s6aqdaNYQw79ZP5LZhB5jELtd0vbSs11STpgw9vbTaRIj wPtkYaVPQGOBRCAiLmWn =siUw -----END PGP SIGNATURE----- --W4pDZ/VvazBYHhxQ--