From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abJZV-0000ui-IH for qemu-devel@nongnu.org; Wed, 02 Mar 2016 22:05:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abJZT-0003qZ-Nv for qemu-devel@nongnu.org; Wed, 02 Mar 2016 22:05:01 -0500 Date: Thu, 3 Mar 2016 14:00:02 +1100 From: David Gibson Message-ID: <20160303030002.GE1620@voom.redhat.com> References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-5-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/2994txjAzEdQwm5" Content-Disposition: inline In-Reply-To: <1456823441-46757-5-git-send-email-aik@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 --/2994txjAzEdQwm5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 creation. > It still will be called once at the owner object (VIO or PHB) creation. >=20 > 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. >=20 > 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 all > and create the default one on a system reset. >=20 > No visible change in behaviour is expected except the actual table > will be reallocated every reset. We might optimize this later. >=20 > 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. >=20 > spapr_tce_table_do_enable() is separated from from spapr_tce_table_enable= () > as later it will be called at the sPAPRTCETable post-migration stage when > it already has all the properties set after the migration. >=20 > 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(-) >=20 > 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 *dev) > sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(dev); > =20 > 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_shif= t); > + "iommu-spapr", 0); > =20 > QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); > =20 > @@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, b= ool need_vfio) > tcet->table =3D newtable; > } > =20 > -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]; > =20 > 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 *owne= r, uint32_t liobn, > return NULL; > } > =20 > - 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; > =20 > 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 *own= er, uint32_t liobn, > return tcet; > } > =20 > +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_shift); > + > + 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. > + } > + > + 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) { > + return; > + } > + > + memory_region_set_size(&tcet->iommu, 0); > + > + spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table); > + tcet->fd =3D -1; > + tcet->table =3D NULL; > + tcet->enabled =3D false; > + tcet->bus_offset =3D 0; > + tcet->page_shift =3D 0; > + tcet->nb_table =3D 0; > + tcet->need_vfio =3D false; > +} > + > static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp) > { > sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(dev); > =20 > QLIST_REMOVE(tcet, list); > =20 > - spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table); > - tcet->fd =3D -1; > + spapr_tce_table_disable(tcet); > } > =20 > MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet) > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 248f20a..c34a906 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -815,12 +815,13 @@ static int spapr_phb_dma_window_enable(sPAPRPHBStat= e *sphb, > return -1; > } > =20 > - tcet =3D spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, > - page_shift, nb_table, false); > + tcet =3D spapr_tce_find_by_liobn(liobn); > if (!tcet) { > return -1; > } > =20 > + spapr_tce_table_enable(tcet, page_shift, window_addr, nb_table, fals= e); > + > memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > spapr_tce_get_iommu(tcet)); > return 0; > @@ -1251,6 +1252,7 @@ static void spapr_phb_realize(DeviceState *dev, Err= or **errp) > int i; > PCIBus *bus; > uint64_t msi_window_size =3D 4096; > + sPAPRTCETable *tcet; > =20 > if (sphb->index !=3D (uint32_t)-1) { > hwaddr windows_base; > @@ -1402,11 +1404,18 @@ static void spapr_phb_realize(DeviceState *dev, E= rror **errp) > } > } > =20 > + /* DMA setup */ > + tcet =3D spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn); > + if (!tcet) { > + error_report("No default TCE table for %s", sphb->dtbusname); > + return; > + } > + > /* Register default 32bit DMA window */ > - if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAG= E_SHIFT, > - sphb->dma_win_addr, sphb->dma_win_si= ze)) { > - error_setg(errp, "Unable to create TCE table for %s", sphb->dtbu= sname); > - } > + spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, > + SPAPR_TCE_PAGE_SHIFT, > + sphb->dma_win_addr, > + sphb->dma_win_size); > =20 > sphb->msi =3D g_hash_table_new_full(g_int_hash, g_int_equal, g_free,= g_free); > } > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index 0f61a55..a745884 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -481,11 +481,10 @@ static void spapr_vio_busdev_realize(DeviceState *q= dev, Error **errp) > memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbyp= ass, 1); > address_space_init(&dev->as, &dev->mrroot, qdev->id); > =20 > - dev->tcet =3D spapr_tce_new_table(qdev, liobn, > - 0, > - SPAPR_TCE_PAGE_SHIFT, > - pc->rtce_window_size >> > - SPAPR_TCE_PAGE_SHIFT, false); > + dev->tcet =3D spapr_tce_new_table(qdev, liobn); > + spapr_tce_table_enable(dev->tcet, SPAPR_TCE_PAGE_SHIFT, 0, > + pc->rtce_window_size >> SPAPR_TCE_PAGE_SH= IFT, > + false); > dev->tcet->vdev =3D dev; > memory_region_add_subregion_overlap(&dev->mrroot, 0, > spapr_tce_get_iommu(dev->tce= t), 2); > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 098d85d..3e6bb84 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -539,6 +539,7 @@ typedef struct sPAPRTCETable sPAPRTCETable; > =20 > struct sPAPRTCETable { > DeviceState parent; > + bool enabled; > uint32_t liobn; > uint32_t nb_table; > uint64_t bus_offset; > @@ -566,11 +567,10 @@ void spapr_events_fdt_skel(void *fdt, uint32_t epow= _irq); > int spapr_h_cas_compose_response(sPAPRMachineState *sm, > target_ulong addr, target_ulong size, > bool cpu_update, bool memory_update); > -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); > +void spapr_tce_table_enable(sPAPRTCETable *tcet, > + uint32_t page_shift, uint64_t bus_offset, > + uint32_t nb_table, bool vfio_accel); > void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio); > =20 > MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); --=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 --/2994txjAzEdQwm5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW16ixAAoJEGw4ysog2bOSe4wQAMO56ZhTwYsTW3oUT24uA81e QoJ8vouSr03OxEkZpm2UNs+6HFNnyPO6L8ktlPqCgzvCZlrFOA07haJQ1bb/sDaR ni1Vd/KOsxweQkkEV7OCNb1g9q4w0lFVDbYbx31CY0+nBPAFXULhKohgq6O8pwWa 08HE0JZp3YMW/96etWC6FEt75P6sF7EpTZWEvJY9ibEgz2p6m6QJ+4e3DTZKZHKD 6AKFdY1soSeas282K/1hwEhMdySJQOjfhS0BZTV6LIGNkwZOsL4sVafGZKEjYAKP WoAacPgVnk+opIBmIZUGcF5P4DO+FcYsZeaIpoMTd5rhTCgD6h18dwP5GeGq2gK6 WPPV32mvHPHihdJ93OCMILg1KypM40GQ9MS7jExNh/7dwkUq0YISQxfcwc0zja3B NzYtVCvdWIQg5EsUKCqkzAsxOwZRlmbqG+iTFqPHcHRtfvu9NXVftkHFlyOmDEkA wKE7NST9rioij3zuIIY1M9ZXzeaWUE1zrKLadB3LBXDtAp2XHxafCOlfYWpqyVVk AaDrEc7m4XewkX/x1zNUGRYNHy2uZt7W+DsZ8h84ZVdyrbGru8C/2t7srl8FvreK XBR0dQu1W0R1Bdh1hxpskszB2sINNJwyI/Quf0XNlmOv/fTQjQhbfXxOolJoBwjp hcHwr1snM2o+ICs5IbUS =eIQ/ -----END PGP SIGNATURE----- --/2994txjAzEdQwm5--