From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b65sg-0002ud-R6 for qemu-devel@nongnu.org; Thu, 26 May 2016 20:44:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b65sc-0000b7-NC for qemu-devel@nongnu.org; Thu, 26 May 2016 20:44:02 -0400 Date: Fri, 27 May 2016 10:43:48 +1000 From: David Gibson Message-ID: <20160527004348.GL17226@voom.fritz.box> References: <20160516141333.7ea67e20@t450s.home> <1463731482-688-1-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="RelyVQTGfPeWBXwZ" Content-Disposition: inline In-Reply-To: <1463731482-688-1-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [RFC PATCH qemu] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping listening List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alex Williamson , Paolo Bonzini --RelyVQTGfPeWBXwZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 20, 2016 at 06:04:42PM +1000, Alexey Kardashevskiy wrote: > The sPAPR TCE tables manage 2 copies when VFIO is using an IOMMU - > a guest view of the table and a hardware TCE table. If there is no VFIO > presense in the address space, then just the guest view is used, if Nit: s/presense/presence/ > this is the case, it is allocated in the KVM. However since there is no > support yet for VFIO in KVM TCE hypercalls, when we start using VFIO, > we need to move the guest view from KVM to the userspace; and we need > to do this for every IOMMU on a bus with VFIO devices. >=20 > This adds notify_started/notify_stopped callbacks in MemoryRegionIOMMUOps > to notify IOMMU that listeners were set/removed. This allows IOMMU to > take necessary steps before actual notifications happen and do proper > cleanup when the last notifier is removed. >=20 > This implements the callbacks for the sPAPR IOMMU - notify_started() > reallocated the guest view to the user space, notify_stopped() does > the opposite. >=20 > This removes explicit spapr_tce_set_need_vfio() call from PCI hotplug > path as the new callbacks do this better - they notify IOMMU at > the exact moment when the configuration is changed, and this also > includes the case of PCI hot unplug. >=20 > This adds MemoryRegion* to memory_region_unregister_iommu_notifier() > as we need iommu_ops to call notify_stopped() and Notifier* does not > store the owner. >=20 > Signed-off-by: Alexey Kardashevskiy > --- >=20 >=20 > Is this any better? If so, I'll repost as a part of "v17". Thanks. I agree with Alex that this is a much better approach. I'm sad I didn't think of it earlier. Reviewed-by: David Gibson > --- > Changes: > v17: > * replaced IOMMU users counting with simple QLIST_EMPTY() > * renamed the callbacks > * removed requirement for region_del() to be called on memory_listener_un= register() >=20 > v16: > * added a use counter in VFIOAddressSpace->VFIOIOMMUMR >=20 > v15: > * s/need_vfio/vfio-Users/g > --- > hw/ppc/spapr_iommu.c | 12 ++++++++++++ > hw/ppc/spapr_pci.c | 6 ------ > hw/vfio/common.c | 5 +++-- > include/exec/memory.h | 8 +++++++- > memory.c | 10 +++++++++- > 5 files changed, 31 insertions(+), 10 deletions(-) >=20 > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 73bc26b..fd38006 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -156,6 +156,16 @@ static uint64_t spapr_tce_get_page_sizes(MemoryRegio= n *iommu) > return 1ULL << tcet->page_shift; > } > =20 > +static void spapr_tce_notify_started(MemoryRegion *iommu) > +{ > + spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), t= rue); > +} > + > +static void spapr_tce_notify_stopped(MemoryRegion *iommu) > +{ > + spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), f= alse); > +} > + At some point we probably want to use this cleanup to remove the "need_vfio" names inside the spapr code, but I don't think that's reasonably within the scope of this patch. > static void spapr_tce_table_do_enable(sPAPRTCETable *tcet); > static void spapr_tce_table_do_disable(sPAPRTCETable *tcet); > =20 > @@ -240,6 +250,8 @@ static const VMStateDescription vmstate_spapr_tce_tab= le =3D { > static MemoryRegionIOMMUOps spapr_iommu_ops =3D { > .translate =3D spapr_tce_translate_iommu, > .get_page_sizes =3D spapr_tce_get_page_sizes, > + .notify_started =3D spapr_tce_notify_started, > + .notify_stopped =3D spapr_tce_notify_stopped, > }; > =20 > static int spapr_tce_table_realize(DeviceState *dev) > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 68e77b0..7c2c622 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1089,12 +1089,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnec= tor *drc, > void *fdt =3D NULL; > int fdt_start_offset =3D 0, fdt_size; > =20 > - if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) { > - sPAPRTCETable *tcet =3D spapr_tce_find_by_liobn(phb->dma_liobn); > - > - spapr_tce_set_need_vfio(tcet, true); > - } > - > if (dev->hotplugged) { > fdt =3D create_device_tree(&fdt_size); > fdt_start_offset =3D spapr_create_pci_child_dt(phb, pdev, fdt, 0= ); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 2e4f703..d1fa9ab 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -523,7 +523,8 @@ static void vfio_listener_region_del(MemoryListener *= listener, > =20 > QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) { > if (giommu->iommu =3D=3D section->mr) { > - memory_region_unregister_iommu_notifier(&giommu->n); > + memory_region_unregister_iommu_notifier(giommu->iommu, > + &giommu->n); > QLIST_REMOVE(giommu, giommu_next); > g_free(giommu); > break; > @@ -1040,7 +1041,7 @@ static void vfio_disconnect_container(VFIOGroup *gr= oup) > QLIST_REMOVE(container, next); > =20 > QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next,= tmp) { > - memory_region_unregister_iommu_notifier(&giommu->n); > + memory_region_unregister_iommu_notifier(giommu->iommu, &giom= mu->n); > QLIST_REMOVE(giommu, giommu_next); > g_free(giommu); > } > diff --git a/include/exec/memory.h b/include/exec/memory.h > index bfb08d4..1c41eb6 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -151,6 +151,10 @@ struct MemoryRegionIOMMUOps { > IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is= _write); > /* Returns supported page sizes */ > uint64_t (*get_page_sizes)(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); > }; > =20 > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -620,9 +624,11 @@ void memory_region_iommu_replay(MemoryRegion *mr, No= tifier *n, bool is_write); > * memory_region_unregister_iommu_notifier: unregister a notifier for > * changes to IOMMU translation entries. > * > + * @mr: the memory region which was observed and for which notity_stoppe= d() > + * needs to be called > * @n: the notifier to be removed. > */ > -void memory_region_unregister_iommu_notifier(Notifier *n); > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier = *n); > =20 > /** > * memory_region_name: get a memory region's name > diff --git a/memory.c b/memory.c > index d22cf5e..fcf978a 100644 > --- a/memory.c > +++ b/memory.c > @@ -1512,6 +1512,10 @@ bool memory_region_is_logging(MemoryRegion *mr, ui= nt8_t client) > =20 > void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > { > + if (mr->iommu_ops->notify_started && > + QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + mr->iommu_ops->notify_started(mr); > + } > notifier_list_add(&mr->iommu_notify, n); > } > =20 > @@ -1545,9 +1549,13 @@ void memory_region_iommu_replay(MemoryRegion *mr, = Notifier *n, bool is_write) > } > } > =20 > -void memory_region_unregister_iommu_notifier(Notifier *n) > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier = *n) > { > notifier_remove(n); > + if (mr->iommu_ops->notify_stopped && > + QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + mr->iommu_ops->notify_stopped(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 --RelyVQTGfPeWBXwZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXR5hEAAoJEGw4ysog2bOS4sEP/jLVWNg7LxUocPQBQrWFArRC 3TUZal+/I1sfLj+yz5HkFCTDgwxz5FZJwzXny52sTwGADqdlp5RX5sUvWgYVmTeb UgUQfjsiJ069T9XmQxlsX7yo80oi2hxzhNdQN5f6F8TZCHbkRUIPujLBm5ICzX2S eP9qoC1mxVoOzpOS8c6LlYPvoCXUd406r1HrEzm8FETnvuBH815wQ4DzWd54vv9F KN7uDREO1XlYnCHBAwuvreGcMoFSlecd9LoNqJ8LsD0mp/FjxxhoEnGNpnDuI1hd vD/LVwD1TGfJmb/7zy4nJSZkpObvEDJlUYWVxyMPh4uaGhfS8NHS92lDPJjoCyWD CvtsILhsoqdRN9lZd+/FESScmnvLtgySfxCioA1Hs3wrrXoKQnTyi1m8GJi+s6qR 4/NCqIAjncohJZfq7suYzdAfYcVC7EE8AJ9ELog915Uku2UqhysgRbO/m5/LL0Bm DCshpKkSECMExlBX8Z6GpID9icBLLZvS4b68PmX/h/gHjKRHWef73ZY9vx9rFLUN /OVtxVcDcDy675MSMEBWz6/xVDiLT+bivTNkPprTcY/9w9Y9CvwZIDhi1ai72TX5 pIkGm6jmT9HbLHoiY4is1LrBt3WikZk9VutXUgChJ8DcAxpUqCwRFPw6PjNcoVuT ejgYTbMLzP9ebM6FhzpF =4OqW -----END PGP SIGNATURE----- --RelyVQTGfPeWBXwZ--