From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abLtl-00036I-9g for qemu-devel@nongnu.org; Thu, 03 Mar 2016 00:34:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abLth-0003hV-7j for qemu-devel@nongnu.org; Thu, 03 Mar 2016 00:34:05 -0500 Date: Thu, 3 Mar 2016 16:28:20 +1100 From: David Gibson Message-ID: <20160303052820.GH1620@voom.redhat.com> References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-8-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="l06SQqiZYCi8rTKz" Content-Disposition: inline In-Reply-To: <1456823441-46757-8-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 07/16] vfio, memory: Notify IOMMU about starting/stopping being used by VFIO List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --l06SQqiZYCi8rTKz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 01, 2016 at 08:10:32PM +1100, Alexey Kardashevskiy wrote: > This adds a vfio_votify() callback to inform an IOMMU (and then its owner) > that VFIO started using the IOMMU. This is used by the pseries machine to > enable/disable in-kernel acceleration of TCE hypercalls. >=20 > Signed-off-by: Alexey Kardashevskiy Hmm.. the current approach of having a hook when vfio-pci devices are attached is pretty ugly, but what exactly the case that it doesn't handle and this approach does? This two tiered notify system for a single bit is also kinda ugly. > --- > hw/ppc/spapr_iommu.c | 9 +++++++++ > hw/ppc/spapr_pci.c | 14 ++++++++------ > hw/vfio/common.c | 7 +++++++ > include/exec/memory.h | 2 ++ > include/hw/ppc/spapr.h | 4 ++++ > 5 files changed, 30 insertions(+), 6 deletions(-) >=20 > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 8a88a74..67a8356 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -136,6 +136,13 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(Memor= yRegion *iommu, hwaddr addr, > return ret; > } > =20 > +static int spapr_tce_vfio_notify(MemoryRegion *iommu, bool attached) > +{ > + sPAPRTCETable *tcet =3D container_of(iommu, sPAPRTCETable, iommu); > + > + return spapr_tce_vfio_notify_owner(tcet->owner, tcet, attached); I'm guessing the "owner" is the PHB, but I'm not entirely clear. Could you use the QOM parent to get the the PHB instead of storing it explicitly? > +} > + > static int spapr_tce_table_post_load(void *opaque, int version_id) > { > sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(opaque); > @@ -167,6 +174,7 @@ static const VMStateDescription vmstate_spapr_tce_tab= le =3D { > =20 > static MemoryRegionIOMMUOps spapr_iommu_ops =3D { > .translate =3D spapr_tce_translate_iommu, > + .vfio_notify =3D spapr_tce_vfio_notify, > }; > =20 > static int spapr_tce_table_realize(DeviceState *dev) > @@ -235,6 +243,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner= , uint32_t liobn) > =20 > tcet =3D SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE)); > tcet->liobn =3D liobn; > + tcet->owner =3D owner; > =20 > snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn); > object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL); > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index ee0fecf..b0cd148 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1084,6 +1084,14 @@ static int spapr_populate_pci_child_dt(PCIDevice *= dev, void *fdt, int offset, > return 0; > } > =20 > +int spapr_tce_vfio_notify_owner(DeviceState *dev, sPAPRTCETable *tcet, > + bool attached) > +{ > + spapr_tce_set_need_vfio(tcet, attached); Hmm.. you go to the trouble of storing owner in dev, then don't actually use it. > + return 0; > +} > + > /* create OF node for pci device and required OF DT properties */ > static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev, > void *fdt, int node_offset) > @@ -1118,12 +1126,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 9bf4c3b..ca3fd47 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -384,6 +384,7 @@ static void vfio_listener_region_add(MemoryListener *= listener, > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > =20 > memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); > + giommu->iommu->iommu_ops->vfio_notify(section->mr, true); > memory_region_iommu_replay(giommu->iommu, &giommu->n, > vfio_container_granularity(container), > false); > @@ -430,6 +431,7 @@ static void vfio_listener_region_del(MemoryListener *= listener, > VFIOContainer *container =3D container_of(listener, VFIOContainer, l= istener); > hwaddr iova, end; > int ret; > + MemoryRegion *iommu =3D NULL; > =20 > if (vfio_listener_skipped_section(section)) { > trace_vfio_listener_region_del_skip( > @@ -451,6 +453,7 @@ static void vfio_listener_region_del(MemoryListener *= listener, > QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) { > if (giommu->iommu =3D=3D section->mr) { > memory_region_unregister_iommu_notifier(&giommu->n); > + iommu =3D giommu->iommu; > QLIST_REMOVE(giommu, giommu_next); > g_free(giommu); > break; > @@ -483,6 +486,10 @@ static void vfio_listener_region_del(MemoryListener = *listener, > "0x%"HWADDR_PRIx") =3D %d (%m)", > container, iova, end - iova, ret); > } > + > + if (iommu && iommu->iommu_ops && iommu->iommu_ops->vfio_notify) { > + iommu->iommu_ops->vfio_notify(section->mr, false); > + } So, if an IOMMU is removed from the guest, this will turn off VFIO enablement. However, IIUC this won't get caled in the more likely case that the address space stays the same, but the VFIO device is removed. > } > =20 > static const MemoryListener vfio_memory_listener =3D { > diff --git a/include/exec/memory.h b/include/exec/memory.h > index d5284c2..9f82629 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -150,6 +150,8 @@ typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMU= Ops; > struct MemoryRegionIOMMUOps { > /* Return a TLB entry that contains a given address. */ > IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is= _write); > + /* Called when VFIO starts/stops using this */ > + int (*vfio_notify)(MemoryRegion *iommu, bool attached); > }; > =20 > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 8aa0c45..5d2f8f4 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -550,6 +550,7 @@ struct sPAPRTCETable { > int fd; > MemoryRegion root, iommu; > struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility = only */ > + DeviceState *owner; > QLIST_ENTRY(sPAPRTCETable) list; > }; > =20 > @@ -629,4 +630,7 @@ int spapr_rng_populate_dt(void *fdt); > */ > #define SPAPR_LMB_FLAGS_ASSIGNED 0x00000008 > =20 > +int spapr_tce_vfio_notify_owner(DeviceState *dev, sPAPRTCETable *tcet, > + bool attached); > + > #endif /* !defined (__HW_SPAPR_H__) */ --=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 --l06SQqiZYCi8rTKz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW18t0AAoJEGw4ysog2bOSW4MQAN7rnQFNswYX8TenhfCcbuov Vj7s4qIjXJApSwrCTKdwLMxY6wsz5jzrsBDyKn9WFMpf3Abc+yyWNAoGzGPJqYvE V9j7v9srTD7c5rT8+h7tuwGaj3q6RzdgGhEqXzwNBjqLMYbRI9rLkQyFsZzfY3jr KMe6oBjxe/sL7mxlwglyNfD8gT221CRDyGMk81NWqiwOn+3J6dtR1+bjM2MOiY5V Xmug0aTOffy7dF3VWhTfxUVsEUZ3dtFoWe+EvhPYFqxs22fYemdi2q2qmyyKlyqM Mt8tiULVWcerinYJ7bqijDYl4qaSvvinb5BSvSSdHD5zgVAs8ag8KsnsSxBm0Hnn 3oYOzTGML0GCA1KGPweDMMtuy9n70Dw5u0+FJz1C2OZLZje43jnOVPPr1n/lYLWS 1hy2dZACp99cSuWDzOAd791TrVEFLCIeOzoZWNa0ULT9ZUnJxzPbVhRGIvEbkXx+ He4wF9tc685C1oYPlQxKnBYT6ngKB1YD3tHyT7pAGwr8DS+hXw6YGlVYlXiJZQPd SKikF5+LBI+IveNesL4n+3jirB0EbUYTfvFWFwbzgEOrXvCUnMIddbxw0hASPQKD 7yWrRP+vtzgCpQli/x411upYsGLc6HO1UIDLHvg6odOQ0zQ2P5i+DBFujW7IveSo WIaIAPB2txhFKArVECL4 =ZYDy -----END PGP SIGNATURE----- --l06SQqiZYCi8rTKz--