From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49021) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1at5rS-0003a8-1B for qemu-devel@nongnu.org; Thu, 21 Apr 2016 00:05:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1at5rQ-0005KI-OT for qemu-devel@nongnu.org; Thu, 21 Apr 2016 00:05:01 -0400 Date: Thu, 21 Apr 2016 13:59:54 +1000 From: David Gibson Message-ID: <20160421035954.GH1133@voom> References: <1459762426-18440-1-git-send-email-aik@ozlabs.ru> <1459762426-18440-15-git-send-email-aik@ozlabs.ru> <20160407004056.GE16485@voom.fritz.box> <571748A3.4070105@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="5vjQsMS/9MbKYGLq" Content-Disposition: inline In-Reply-To: <571748A3.4070105@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: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alex Williamson , Alexander Graf --5vjQsMS/9MbKYGLq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Apr 20, 2016 at 07:15:15PM +1000, Alexey Kardashevskiy wrote: > On 04/07/2016 10:40 AM, David Gibson wrote: > >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. > >> > >>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. > >> > >>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. > >> > >>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. > >> > >>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(-) > >> > >>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(MemoryReg= ion *iommu) > >> return 1ULL << tcet->page_shift; > >> } > >> > >>+static void spapr_tce_vfio_start(MemoryRegion *iommu) > >>+{ > >>+ spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu),= true); > >>+} > >>+ > >>+static void spapr_tce_vfio_stop(MemoryRegion *iommu) > >>+{ > >>+ spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu),= false); > >>+} > >>+ > >> static void spapr_tce_table_do_enable(sPAPRTCETable *tcet); > >> static void spapr_tce_table_do_disable(sPAPRTCETable *tcet); > >> > >>@@ -239,6 +249,8 @@ static const VMStateDescription vmstate_spapr_tce_t= able =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 > Everything is manageable... >=20 > This referencing is needed for the case of >=3D2 containers so > 2xvfio_listener_region_add will create 2xVFIOGuestIOMMU as they are per > VFIOContainer so VFIOGuestIOMMU is not the right place for the reference > counting, VFIOAddressSpace seems to be that place (=3D> add list of IOMMU= MRs > with refcounter). Or even IOMMU MR. Or move VFIOGuestIOMMU list from > VFIOContainer to VFIOAddressSpace and then gIOMMU can handle > refcounting? I'm having a lot of trouble parsing that. I think the ref parsing has to be per-giommu (because individual giommus could, in theory, be mapped or unmapped from an address space). But I think that should be in the vfio core, rather than being necessary in every giommu implementation. --=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 --5vjQsMS/9MbKYGLq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXGFA6AAoJEGw4ysog2bOSXpAP/Rm78yaVihGyDkqECHPamTmK c7Z2MHv3ZqVXOjX0cO6YdSxQW+9XhA/OCQ/+3l5sHcf66aAHvcGHPmPUdN4vdK4a yHyLhgibzCraF71BwbQLzV2qpDMbtjoQOY3eqhL6nWgtNAFHMofBABk76yMGbIg0 1g9kexsImtvdkzlKagG22tq9l39GX+ktVOxZIKe2td/JLKWiAVjBhq7hA9tk0a2E 7gIv4p8JxUe/2APNVINPHz5mI7SOugt+bhB/On82HedeIkiRAImYicuLsFcqZk4q WmbmR0CV6VS6nNC28d44phHn8AP8BWYFQhVl6NV2z2fMATbGGd5y/lK+jzHNyrgo hcL0RSfgqvReo5w3zSvMITrIc/5Bu4N6dxTeMIDkr2fowsVNQKTwfAUS7j7h5yXj s6Z+qZx6wDDsbqVOwOMiGewosD/+OS8tXUv9wYW+mvl+ui1CIpCQ1KO7WJjquHoH f3AIHiVY1YYBt33q39TmL3KqkFgzGpW9CPm/7PoFt5gycPoBlVGo3cZBCugzSADB 3iFhsuRubw+qC2Dfm3wx3SQxizuDc320AZXm1CqMdHYcP5i9ZWR7VI1GCn9iXtaK 6KezS5RRujFDnab3DUr/VD9s9ppvjpBG88Y0Ip3+p9DntjmbtLVNhrDNVWd8ulO1 juO4wFmPjoii15JdDjLt =AVLU -----END PGP SIGNATURE----- --5vjQsMS/9MbKYGLq--