From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40685) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5lMU-0000au-3p for qemu-devel@nongnu.org; Thu, 18 Jun 2015 21:44:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5lMQ-00082x-TP for qemu-devel@nongnu.org; Thu, 18 Jun 2015 21:44:54 -0400 Date: Fri, 19 Jun 2015 11:45:10 +1000 From: David Gibson Message-ID: <20150619014510.GI13352@voom.redhat.com> References: <1429964684-23872-1-git-send-email-aik@ozlabs.ru> <1429964684-23872-14-git-send-email-aik@ozlabs.ru> <20150505124940.GS14090@voom.redhat.com> <5582AD10.3070400@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="wluVThW9QgEmL6s2" Content-Disposition: inline In-Reply-To: <5582AD10.3070400@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v7 13/14] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW) 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 --wluVThW9QgEmL6s2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 18, 2015 at 09:35:44PM +1000, Alexey Kardashevskiy wrote: > On 05/05/2015 10:49 PM, David Gibson wrote: > >On Sat, Apr 25, 2015 at 10:24:43PM +1000, Alexey Kardashevskiy wrote: > >>This adds support for Dynamic DMA Windows (DDW) option defined by > >>the SPAPR specification which allows to have additional DMA window(s) > >> > >>This implements DDW for emulated and VFIO devices. As all TCE root regi= ons > >>are mapped at 0 and 64bit long (and actual tables are child regions), > >>this replaces memory_region_add_subregion() with _overlap() to make > >>QEMU memory API happy. > >> > >>This reserves RTAS token numbers for DDW calls. > >> > >>This implements helpers to interact with VFIO kernel interface. > >> > >>This changes the TCE table migration descriptor to support dynamic > >>tables as from now on, PHB will create as many stub TCE table objects > >>as PHB can possibly support but not all of them might be initialized at > >>the time of migration because DDW might or might not be requested by > >>the guest. > >> > >>The "ddw" property is enabled by default on a PHB but for compatibility > >>the pseries-2.3 machine and older disable it. > >> > >>This implements DDW for VFIO. The host kernel support is required. > >>This adds a "levels" property to PHB to control the number of levels > >>in the actual TCE table allocated by the host kernel, 0 is the default > >>value to tell QEMU to calculate the correct value. Current hardware > >>supports up to 5 levels. > >> > >>The existing linux guests try creating one additional huge DMA window > >>with 64K or 16MB pages and map the entire guest RAM to. If succeeded, > >>the guest switches to dma_direct_ops and never calls TCE hypercalls > >>(H_PUT_TCE,...) again. This enables VFIO devices to use the entire RAM > >>and not waste time on map/unmap later. > >> > >>This adds 4 RTAS handlers: > >>* ibm,query-pe-dma-window > >>* ibm,create-pe-dma-window > >>* ibm,remove-pe-dma-window > >>* ibm,reset-pe-dma-window > >>These are registered from type_init() callback. > >> > >>These RTAS handlers are implemented in a separate file to avoid polluti= ng > >>spapr_iommu.c with PCI. > >> > >>Signed-off-by: Alexey Kardashevskiy > > > >Reviewed-by: David Gibson >=20 > I saw this and decided there are no more coments but I was wrong :) Right. Note that if I add a Reviewed-by but also make comments, then those comments are seeking clarification and maybe suggesting later cleanups, but I think the problems are small enough that the patch is still ready to go as it is. [snip] > >>+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet); > >>+ > >> static int spapr_tce_table_post_load(void *opaque, int version_id) > >> { > >> sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(opaque); > >>@@ -98,22 +107,42 @@ static int spapr_tce_table_post_load(void *opaque,= int version_id) > >> spapr_vio_set_bypass(tcet->vdev, tcet->bypass); > >> } > >> > >>+ if (!tcet->migtable) { > > > >What's the case where migtable will be NULL? IIUC an old->new > >migration will result in the data saved for "table" being loaded into > >"migtable". > > > >So "migtable" should only be NULL, when tce->enabled is also false? >=20 >=20 > Seems to be true and this is just extra precaution. Remove? Yes, remove as a cleanup, but that can be done later, I won't hold up the main patch series for this. --=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 --wluVThW9QgEmL6s2 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVg3QmAAoJEGw4ysog2bOStxsP+wYV3ZNr7oknDikREn9YTMgN QykXyU+yPHJM+alZ+u3ehhwSPgmcRTiAuT9OfQNVpr1FIK9iJ71+g8AWtNMnNxr6 7ph3xopOK8NJ3zIdKVU41lDXPyCL+/1Y4hS4jGs/+fg8JiXyiGsy98ziF3S9O6E4 dLhIHOyfRg/nIriTOyw1Xz8P7YvDjPIVsm0FeciYFshNXQ3gvwAR0YeMMe2XaUHI xuGbpEl/wggyW6XboGRO1awZd3JQX+v/FzZTl39M6VL/UnD2ZmKVIrKSczTpNpQw I+Oy4nY/UOJDtH3CqCGDgmteMivun02py8U8Tner9zfeu/Sxfu97jJSARHW2ixKm YWCz0XGYOVqKoiTDmtpAiUxKTyWkJSP8SqUj+Bn6Rdpj8uejMeMPvgGnLHTWf8yE WYzJsOkhQgvlYnA1twgRSWt+V4JdXGrRg44DvCsrPko6WkwItf69jdljxAD9wSWt IcMiyu/ZU1QgWM2eitvEksdKzyBvGrUy0WdrVchtM9bfnhhc5FCAMw/S3LFITJBm c9uSoBcSUhmEheaqYihy+V0uRwjLcriVWIG9EcymTs3JHMF7TyPgAAeprTY89iFf OIQf0Em15WnL/bWLElxEDJDS6Eb+XpI026a/bLEApOaqi0wp7NOd7atQt5fj2MbC tpbfvAdP6+001st+XAzt =gwQC -----END PGP SIGNATURE----- --wluVThW9QgEmL6s2--