From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abhif-000173-MN for qemu-devel@nongnu.org; Thu, 03 Mar 2016 23:52:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abhid-0006s5-I2 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 23:52:05 -0500 Date: Fri, 4 Mar 2016 15:01:02 +1100 From: David Gibson Message-ID: <20160304040102.GX1620@voom.redhat.com> References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-8-git-send-email-aik@ozlabs.ru> <20160303052820.GH1620@voom.redhat.com> <56D7D33B.8000904@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0btr6/5+bhBdui6J" Content-Disposition: inline In-Reply-To: <56D7D33B.8000904@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 --0btr6/5+bhBdui6J Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 03, 2016 at 05:01:31PM +1100, Alexey Kardashevskiy wrote: > On 03/03/2016 04:28 PM, David Gibson wrote: > >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 own= er) > >>that VFIO started using the IOMMU. This is used by the pseries machine = to > >>enable/disable in-kernel acceleration of TCE hypercalls. > >> > >>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? >=20 > Sorry, I am not following you here. What hook do you mean here? >=20 > My hook fixes the case when I want to enable/disable KVM acceleration, > without these patches, I need to re-count how many vfio-pci devices are > there and it is more ugly with PCI hotplug/unplug... >=20 >=20 > >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(-) > >> > >>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(Mem= oryRegion *iommu, hwaddr addr, > >> return ret; > >> } > >> > >>+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? >=20 >=20 > I am pretty sure I am not allowed to use the QOM parent, this is why there > is no object_get_parent() helper. Hmm.. I thought I had this discussion before and accessing the qom parent from qmp was bad, but it was ok for internal code use. But I may be getting muddled with older qdev stuff. >=20 > > > >>+} > >>+ > >> 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_t= able =3D { > >> > >> static MemoryRegionIOMMUOps spapr_iommu_ops =3D { > >> .translate =3D spapr_tce_translate_iommu, > >>+ .vfio_notify =3D spapr_tce_vfio_notify, > >> }; > >> > >> static int spapr_tce_table_realize(DeviceState *dev) > >>@@ -235,6 +243,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *own= er, uint32_t liobn) > >> > >> tcet =3D SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE)); > >> tcet->liobn =3D liobn; > >>+ tcet->owner =3D owner; > >> > >> 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; > >> } > >> > >>+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. >=20 >=20 > Yeah, I need to clean this, I removed spapr_tce_vfio_notify_owner() from = my > working branch already and call spapr_tce_set_need_vfio() directly from > spapr_tce_vfio_notify(). Ok. >=20 >=20 > > > >>+ return 0; > >>+} > >>+ > >> /* create OF node for pci device and required OF DT properties */ > >> static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *d= ev, > >> void *fdt, int node_offset) > >>@@ -1118,12 +1126,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConn= ector *drc, > >> void *fdt =3D NULL; > >> int fdt_start_offset =3D 0, fdt_size; > >> > >>- 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_nex= t); > >> > >> 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(contain= er), > >> false); > >>@@ -430,6 +431,7 @@ static void vfio_listener_region_del(MemoryListener= *listener, > >> VFIOContainer *container =3D container_of(listener, VFIOContainer= , listener); > >> hwaddr iova, end; > >> int ret; > >>+ MemoryRegion *iommu =3D NULL; > >> > >> 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(MemoryListene= r *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 >=20 > When VFIO device is removed, its listener gets removed and this is suppos= ed > to end up calling vfio_listener_region_del(). Ah, ok region_del() gets called on all existing regions when the listener is removed. I guess that makes sense since region_add() is called on the existing regions when the listener is registered. >=20 >=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 MemoryRegionIOM= MUOps; > >> 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); > >> }; > >> > >> 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 compatibili= ty only */ > >>+ DeviceState *owner; > >> QLIST_ENTRY(sPAPRTCETable) list; > >> }; > >> > >>@@ -629,4 +630,7 @@ int spapr_rng_populate_dt(void *fdt); > >> */ > >> #define SPAPR_LMB_FLAGS_ASSIGNED 0x00000008 > >> > >>+int spapr_tce_vfio_notify_owner(DeviceState *dev, sPAPRTCETable *tcet, > >>+ bool attached); > >>+ > >> #endif /* !defined (__HW_SPAPR_H__) */ > > >=20 >=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 --0btr6/5+bhBdui6J Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW2Qh+AAoJEGw4ysog2bOSNU8P/RH3c5twBng/Hi5aKfgBTpmr 3wRUnM72Hy6SMev2DJQg0mC3S8ABykglgbK5XTib4izJDiRGFTeoKwi8naqtH05A A6i6gntccHJ5auhRzGxV26SNPvGBn9N5B2zeOmv0FlH3pDDOWn3N5T1W8AncdHbw EQglXrQQKBBtrI0dYT7YOZHWfYhxktmpiS1FoGKoooZRgylrKmJOswpi66WNJS3M NlZBqYsUWZgOKfpKVjdhrY8DVXHfCMQJfdOVoqsCrJsiUx9ga1FLnW+vBZuHZg1B lnQWuqSa12wVa4ZCKKfAe8CvkYBFacLTJ3LYvJTu08Fb+DSqkeHkeBz3dhZRfUhv jlDMoP0dRJQdIOwRqUhQ02YoNoJrtABNAnfkFoWIQk3AFC3m7t0OzgP5kPSlaSAT Md/wx/aNU/AtUrVC3qeTxfGlIYAX9nW5hSarr4V7TBjYFEdH7LLSDw+/4wAuWBtq OP1tpqbikS7MxUI9F3gsnkJ3SYVG7YbOyEYQuxUvNNyrLQTHIwIi3hlt4bHQonMf u37TwXHvxHpo73c4w233g2XM/pxQqa48USLf+Ur7Qp56Dz7kqQuKktHFrvC8m6B3 VHETsRf+6K2Kr3UTZsAUerdGuzdH68SptCX9b9FCCoD5orMVLdsrC+fZ+D7OHofx ejOZTn0+G7mTGgFN1/xC =ynnf -----END PGP SIGNATURE----- --0btr6/5+bhBdui6J--