From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afiTy-0006m6-4n for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afiTw-0006OG-7k for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:30 -0400 Date: Tue, 15 Mar 2016 16:32:14 +1100 From: David Gibson Message-ID: <20160315053214.GG15272@voom.fritz.box> References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-5-git-send-email-aik@ozlabs.ru> <20160303030002.GE1620@voom.redhat.com> <56E124BB.4010207@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0rSojgWGcpz+ezC3" Content-Disposition: inline In-Reply-To: <56E124BB.4010207@ozlabs.ru> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 04/16] spapr_iommu: Introduce "enabled" state for TCE table 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 --0rSojgWGcpz+ezC3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 10, 2016 at 06:39:39PM +1100, Alexey Kardashevskiy wrote: > On 03/03/2016 02:00 PM, David Gibson wrote: > >On Tue, Mar 01, 2016 at 08:10:29PM +1100, Alexey Kardashevskiy wrote: > >>Currently TCE tables are created once at start and their sizes never > >>change. We are going to change that by introducing a Dynamic DMA windows > >>support where DMA configuration may change during the guest execution. > >> > >>This changes spapr_tce_new_table() to create an empty zero-size IOMMU > >>memory region (IOMMU MR). Only LIOBN is assigned by the time of creatio= n. > >>It still will be called once at the owner object (VIO or PHB) creation. > >> > >>This introduces an "enabled" state for TCE table objects with two > >>helper functions - spapr_tce_table_enable()/spapr_tce_table_disable(). > >>- spapr_tce_table_enable() receives TCE table parameters, allocates > >>a guest view of the TCE table (in the user space or KVM) and > >>sets the correct size on the IOMMU MR. > >>- spapr_tce_table_disable() disposes the table and resets the IOMMU MR > >>size. > >> > >>This changes the PHB reset handler to do the default DMA initialization > >>instead of spapr_phb_realize(). This does not make differenct now but > >>later with more than just one DMA window, we will have to remove them a= ll > >>and create the default one on a system reset. > >> > >>No visible change in behaviour is expected except the actual table > >>will be reallocated every reset. We might optimize this later. > >> > >>The other way to implement this would be dynamically create/remove > >>the TCE table QOM objects but this would make migration impossible > >>as the migration code expects all QOM objects to exist at the receiver > >>so we have to have TCE table objects created when migration begins. > >> > >>spapr_tce_table_do_enable() is separated from from spapr_tce_table_enab= le() > >>as later it will be called at the sPAPRTCETable post-migration stage wh= en > >>it already has all the properties set after the migration. > >> > >>Signed-off-by: Alexey Kardashevskiy > > > >Reviewed-by: David Gibson > > > >Although there's one nit that could be improved: > > > > > >>--- > >> hw/ppc/spapr_iommu.c | 80 +++++++++++++++++++++++++++++++++++------= --------- > >> hw/ppc/spapr_pci.c | 21 +++++++++---- > >> hw/ppc/spapr_vio.c | 9 +++--- > >> include/hw/ppc/spapr.h | 10 +++---- > >> 4 files changed, 80 insertions(+), 40 deletions(-) > >> > >>diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > >>index 8132f64..e66e128 100644 > >>--- a/hw/ppc/spapr_iommu.c > >>+++ b/hw/ppc/spapr_iommu.c > >>@@ -174,15 +174,8 @@ static int spapr_tce_table_realize(DeviceState *de= v) > >> sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(dev); > >> > >> tcet->fd =3D -1; > >>- tcet->table =3D spapr_tce_alloc_table(tcet->liobn, > >>- tcet->page_shift, > >>- tcet->nb_table, > >>- &tcet->fd, > >>- tcet->need_vfio); > >>- > >> memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_= ops, > >>- "iommu-spapr", > >>- (uint64_t)tcet->nb_table << tcet->page_sh= ift); > >>+ "iommu-spapr", 0); > >> > >> QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); > >> > >>@@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet,= bool need_vfio) > >> tcet->table =3D newtable; > >> } > >> > >>-sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, > >>- uint64_t bus_offset, > >>- uint32_t page_shift, > >>- uint32_t nb_table, > >>- bool need_vfio) > >>+sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn) > >> { > >> sPAPRTCETable *tcet; > >>- char tmp[64]; > >>+ char tmp[32]; > >> > >> if (spapr_tce_find_by_liobn(liobn)) { > >> fprintf(stderr, "Attempted to create TCE table with duplicate" > >>@@ -239,16 +228,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *ow= ner, uint32_t liobn, > >> return NULL; > >> } > >> > >>- if (!nb_table) { > >>- return NULL; > >>- } > >>- > >> tcet =3D SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE)); > >> tcet->liobn =3D liobn; > >>- tcet->bus_offset =3D bus_offset; > >>- tcet->page_shift =3D page_shift; > >>- tcet->nb_table =3D nb_table; > >>- tcet->need_vfio =3D need_vfio; > >> > >> snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn); > >> object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL); > >>@@ -258,14 +239,65 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *o= wner, uint32_t liobn, > >> return tcet; > >> } > >> > >>+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet) > >>+{ > >>+ if (!tcet->nb_table) { > >>+ return; > >>+ } > >>+ > >>+ tcet->table =3D spapr_tce_alloc_table(tcet->liobn, > >>+ tcet->page_shift, > >>+ tcet->nb_table, > >>+ &tcet->fd, > >>+ tcet->need_vfio); > >>+ > >>+ memory_region_set_size(&tcet->iommu, > >>+ (uint64_t)tcet->nb_table << tcet->page_shif= t); > >>+ > >>+ tcet->enabled =3D true; > >>+} > >>+ > >>+void spapr_tce_table_enable(sPAPRTCETable *tcet, > >>+ uint32_t page_shift, uint64_t bus_offset, > >>+ uint32_t nb_table, bool need_vfio) > >>+{ > >>+ if (tcet->enabled) { > >>+ return; > > > >If the given parameters are different from the current ones, treating > >this as a no-op is rather misleading. I gather that to resize the > >window you're expected to disable, then re-enable. In which case I > >think it would be safer to actually throw some kind of error on a > >double enable. >=20 > I'll add here >=20 > error_report("Warning: trying to enable already enabled TCE table"); >=20 > ... >=20 >=20 >=20 >=20 > > > > > >>+ } > >>+ > >>+ tcet->bus_offset =3D bus_offset; > >>+ tcet->page_shift =3D page_shift; > >>+ tcet->nb_table =3D nb_table; > >>+ tcet->need_vfio =3D need_vfio; > >>+ > >>+ spapr_tce_table_do_enable(tcet); > >>+} > >>+ > >>+static void spapr_tce_table_disable(sPAPRTCETable *tcet) > >>+{ > >>+ if (!tcet->enabled) { >=20 > ...and >=20 > error_report("Warning: trying to disable already disabled TCE table"); That sounds good. > or g_assert()? Erm.. only if this can't be triggered by user actions. --=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 --0rSojgWGcpz+ezC3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW555eAAoJEGw4ysog2bOS2kEQANIdGGs7Cvkm5JjYWGXbzwYn dgliwlTFwn6oH1KTbYtDs7+R4r1aK/sCR2vvuUa/GMmMDZewwNoxses8jlgzM9K5 DYrxYOMfNV9lFXHkc9JC/t7S8uxzkjgrDphczHU2/DUHhe9i7n8q+jUWFcySnP/z nwdXFthAQ1LYXbGkA+EfM17vI2oAEBXgws7VpJKEAZ1aB20Uy/KnvY6Rv1QpdM0R jRRupwiUUgIGJU8mobyMU4WGQA6hr2wvkBtbP+Qaxh2Nx+lRbiWZd9XlZ19NWGnq EwmzCCsjGX4X8ddUE79DF6UoLngVyM9cNgXpZPhrvDllLu3BZubGvGIeJi8mNN/x CxccFA2P5I+gL/6olX+vNUJNoCevN3B1V2x44JCUdFIfiYeSfe08nE8Ez4voRUzc 01MTmCFUcQFnjrCoyFHYdco4OSpgfLnz3d7xLm/0OHqFxr7gepJ5tWd/2EhTDYBK gEDhk6fRUH1W6cbt75CeTE2HCgyF3FjdoSfzwvNFmPfGYbmG2DHkF1SYtESLPBH5 XjinbsSWZMo/XvJEEq4EVIbO40+4wxL/s/YqGiIvXsJ3XLoSw2gUm/cD4NSNfOws EPwt9Zpqimd8eXxvMvkqMKoUX3mv9GhrAarFfAM+QePvmSSCJB38z+Lk7zQmA6B/ gnjIbsCxMNL5uNOIISHf =UK0x -----END PGP SIGNATURE----- --0rSojgWGcpz+ezC3--