From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33562) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anyXB-0002DS-P1 for qemu-devel@nongnu.org; Wed, 06 Apr 2016 21:14:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1anyXA-0007q5-7F for qemu-devel@nongnu.org; Wed, 06 Apr 2016 21:14:57 -0400 Date: Thu, 7 Apr 2016 10:40:56 +1000 From: David Gibson Message-ID: <20160407004056.GE16485@voom.fritz.box> References: <1459762426-18440-1-git-send-email-aik@ozlabs.ru> <1459762426-18440-15-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="oIK88BWnNPSddvdK" Content-Disposition: inline In-Reply-To: <1459762426-18440-15-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v15 14/17] spapr_iommu, 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, Alexander Graf --oIK88BWnNPSddvdK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 04, 2016 at 07:33:43PM +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 > 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 vfio_start/vfio_stop callbacks in MemoryRegionIOMMUOps to > notifiy IOMMU about changing environment so it can reallocate the table > to/from KVM or (when available) hook the IOMMU groups with the logical > bus (LIOBN) in the KVM. >=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 > As there can be multiple containers attached to the same PHB/LIOBN, > this replaces the @need_vfio flag in sPAPRTCETable with the counter > of VFIO users. >=20 > Signed-off-by: Alexey Kardashevskiy This looks correct, but there's one remaining ugly. > --- > Changes: > v15: > * s/need_vfio/vfio-Users/g > --- > hw/ppc/spapr_iommu.c | 30 ++++++++++++++++++++---------- > hw/ppc/spapr_pci.c | 6 ------ > hw/vfio/common.c | 9 +++++++++ > include/exec/memory.h | 4 ++++ > include/hw/ppc/spapr.h | 2 +- > 5 files changed, 34 insertions(+), 17 deletions(-) >=20 > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index c945dba..ea09414 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -155,6 +155,16 @@ static uint64_t spapr_tce_get_page_sizes(MemoryRegio= n *iommu) > return 1ULL << tcet->page_shift; > } > =20 > +static void spapr_tce_vfio_start(MemoryRegion *iommu) > +{ > + spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), t= rue); > +} > + > +static void spapr_tce_vfio_stop(MemoryRegion *iommu) > +{ > + spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), f= alse); > +} > + > static void spapr_tce_table_do_enable(sPAPRTCETable *tcet); > static void spapr_tce_table_do_disable(sPAPRTCETable *tcet); > =20 > @@ -239,6 +249,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, > + .vfio_start =3D spapr_tce_vfio_start, > + .vfio_stop =3D spapr_tce_vfio_stop, Ok, so AFAICT these callbacks are called whenever a VFIO context is added / removed from the gIOMMU's address space, and it's up to the gIOMMU code to ref count that to see if there are any current vfio users. That makes "vfio_start" and "vfio_stop" not great names. But.. better than changing the names would be to move the refcounting to the generic code if you can manage it, so the individual gIOMMU backends don't need to - they just told when they need to start / stop providing VFIO support. > }; > =20 > static int spapr_tce_table_realize(DeviceState *dev) > @@ -248,7 +260,7 @@ static int spapr_tce_table_realize(DeviceState *dev) > char tmp[32]; > =20 > tcet->fd =3D -1; > - tcet->need_vfio =3D false; > + tcet->vfio_users =3D 0; > snprintf(tmp, sizeof(tmp), "tce-root-%x", tcet->liobn); > memory_region_init(&tcet->root, tcetobj, tmp, UINT64_MAX); > =20 > @@ -268,20 +280,18 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, b= ool need_vfio) > size_t table_size =3D tcet->nb_table * sizeof(uint64_t); > void *newtable; > =20 > - if (need_vfio =3D=3D tcet->need_vfio) { > - /* Nothing to do */ > - return; > - } > + tcet->vfio_users +=3D need_vfio ? 1 : -1; > + g_assert(tcet->vfio_users >=3D 0); > + g_assert(tcet->table); > =20 > - if (!need_vfio) { > + if (!tcet->vfio_users) { > /* FIXME: We don't support transition back to KVM accelerated > * TCEs yet */ > return; > } > =20 > - tcet->need_vfio =3D true; > - > - if (tcet->fd < 0) { > + if (tcet->vfio_users > 1) { > + g_assert(tcet->fd < 0); > /* Table is already in userspace, nothing to be do */ > return; > } > @@ -327,7 +337,7 @@ static void spapr_tce_table_do_enable(sPAPRTCETable *= tcet) > tcet->page_shift, > tcet->nb_table, > &tcet->fd, > - tcet->need_vfio); > + tcet->vfio_users !=3D 0); > =20 > memory_region_set_size(&tcet->iommu, > (uint64_t)tcet->nb_table << tcet->page_shift); > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 5497a18..f864fde 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1083,12 +1083,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 ea79311..5e5b77c 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -421,6 +421,9 @@ 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); > + if (section->mr->iommu_ops && section->mr->iommu_ops->vfio_start= ) { > + section->mr->iommu_ops->vfio_start(section->mr); > + } > memory_region_iommu_replay(giommu->iommu, &giommu->n, > false); > =20 > @@ -466,6 +469,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( > @@ -487,6 +491,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; > @@ -519,6 +524,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_stop) { > + iommu->iommu_ops->vfio_stop(section->mr); > + } > } > =20 > static const MemoryListener vfio_memory_listener =3D { > diff --git a/include/exec/memory.h b/include/exec/memory.h > index eb5ce67..f1de133f 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -152,6 +152,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 VFIO starts using this */ > + void (*vfio_start)(MemoryRegion *iommu); > + /* Called when VFIO stops using this */ > + void (*vfio_stop)(MemoryRegion *iommu); > }; > =20 > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 471eb4a..5c00e38 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -548,7 +548,7 @@ struct sPAPRTCETable { > uint32_t mig_nb_table; > uint64_t *mig_table; > bool bypass; > - bool need_vfio; > + int vfio_users; > int fd; > MemoryRegion root, iommu; > struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility = only */ --=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 --oIK88BWnNPSddvdK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXBayYAAoJEGw4ysog2bOSKMQP/05+vA4wJcAfJb0RpNcprFcN k8eQV0xxcq9Xd/NFP5Mavp4SMk3yojoK1LDoiGcvh3Op+gsu70fykQc0D524PgZO zjFefXkJMtlrVtQscJN8lqsHM6DpjQtB49EJbSjZr5Rr+JV4YA+IWHhqnfOnIVqF 5WlmtrlzkrrvM1+QjUALibeoqfaPOjIqfdJ+SzzmwGi1gBIdQBZK5GcfZPALD51B vMXJ8hs9hGEYe3eVwYkjR2xTl6gie3aGfvQmuuUe37dKEX3M5h/EMZCG4tm7otWX zbceVKq12oWKgyWs2Jppg4k52EgFmfKrp/RsFDq3WPa+SeS74BMsnGru/1WQI6ZS biazxA6QYigKnON1mQmwsRAnMYSLidpz+HSfq7QVU9JvKf42DEANT3xmtDPFIGCO wPcXZ38EtLzrENKT2BNSO2mQEfxUCnpPV+Trr47lLLRYELzW8q7DmYakUiFwXg/V 1swK5B0kIBZ31TBZFTHYGuQgS4/nF2DLKCEswuTw3cWzwwAKSJ2uqTy2j3jOZ9Rl WXm2rx42Pf8u7K+SECPZ1KKCblmOa/rn1tBSMcOCA3ungzYxSJpQcWD+MfFkOcH8 /KmYUkc6O1J8+GcMUA7lCv5XvmhYIODxINfHULMIz3fOI9QITGWUJurxH5JEpvgD AMrCRJtTx0LOj2+fQeS9 =+7Dh -----END PGP SIGNATURE----- --oIK88BWnNPSddvdK--