From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43772) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yffw7-0005ux-5c for qemu-devel@nongnu.org; Tue, 07 Apr 2015 22:41:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yffw5-0007Cv-E6 for qemu-devel@nongnu.org; Tue, 07 Apr 2015 22:41:51 -0400 Date: Wed, 8 Apr 2015 12:35:24 +1000 From: David Gibson Message-ID: <20150408023524.GE28909@voom.redhat.com> References: <1427779727-13353-1-git-send-email-aik@ozlabs.ru> <1427779727-13353-8-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="d8Lz2Tf5e5STOWUP" Content-Disposition: inline In-Reply-To: <1427779727-13353-8-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v5 07/12] spapr_iommu: Rework TCE table initialization 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 --d8Lz2Tf5e5STOWUP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 31, 2015 at 04:28:42PM +1100, Alexey Kardashevskiy wrote: > Currently TCE tables are created once at start and their size never > changes. 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 stub object. 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 spapr_tce_set_props() to set the table size, start and > page size. It only assigns the properties. It will be called at the owner > object creation OR later from the "ibm,create-pe-dma-window" RTAS handler > so the table's parameters can change. >=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() allocates the guest view of the TCE table > (in the user space or KVM). spapr_tce_table_disable() disposes the table. >=20 > Follow up patches will disable+enable tables on reset (system reset > or DDW 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 migration expects all QOM objects to exist at the receiver > so we have to have TCE table objects created when migration begins. >=20 > Signed-off-by: Alexey Kardashevskiy > --- > hw/ppc/spapr_iommu.c | 98 +++++++++++++++++++++++++++++++------------= ------ > hw/ppc/spapr_pci.c | 8 ++-- > hw/ppc/spapr_pci_vfio.c | 11 ++++-- > hw/ppc/spapr_vio.c | 10 ++--- > include/hw/ppc/spapr.h | 12 +++--- > 5 files changed, 87 insertions(+), 52 deletions(-) >=20 > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index a14cdc4..a015357 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -126,25 +126,6 @@ static MemoryRegionIOMMUOps spapr_iommu_ops =3D { > static int spapr_tce_table_realize(DeviceState *dev) > { > sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(dev); > - uint64_t window_size =3D (uint64_t)tcet->nb_table << tcet->page_shif= t; > - > - if (kvm_enabled() && !(window_size >> 32)) { > - tcet->table =3D kvmppc_create_spapr_tce(tcet->liobn, > - window_size, > - &tcet->fd, > - tcet->vfio_accel); > - } > - > - if (!tcet->table) { > - size_t table_size =3D tcet->nb_table * sizeof(uint64_t); > - tcet->table =3D g_malloc0(table_size); > - } > - > - trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd= ); > - > - memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops, > - "iommu-spapr", > - (uint64_t)tcet->nb_table << tcet->page_shif= t); > =20 > QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); > =20 > @@ -154,11 +135,7 @@ static int spapr_tce_table_realize(DeviceState *dev) > return 0; > } > =20 > -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, > - uint64_t bus_offset, > - uint32_t page_shift, > - uint32_t nb_table, > - bool vfio_accel) > +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn) > { > sPAPRTCETable *tcet; > char tmp[64]; > @@ -169,36 +146,87 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *own= er, 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->vfio_accel =3D vfio_accel; > =20 > snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn); > object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL); > =20 > object_property_set_bool(OBJECT(tcet), true, "realized", NULL); > =20 > + trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd= ); > + > return tcet; > } > =20 > -static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp) > +void spapr_tce_set_props(sPAPRTCETable *tcet, uint64_t bus_offset, > + uint32_t page_shift, uint32_t nb_table, > + bool vfio_accel) > { > - sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(dev); > + if (tcet->enabled) { > + return; > + } Since you can't change the properties while the table is enabled, why not just make these parameters to spapr_tce_table_enable(). It seems to me what this is really about is making a distinction between two objects: (1) is the TCE table as an abstract concept - it knows its liobn and its owner, and that's about it (2) the TCE table as a specific instantiated table - it has a specific size and current entries. (2) can't be a QOM object or migration breaks, but you can still think of it as a distinct entity at the C level. > + tcet->bus_offset =3D bus_offset; > + tcet->page_shift =3D page_shift; > + tcet->nb_table =3D nb_table; > + tcet->vfio_accel =3D vfio_accel; > +} > =20 > - QLIST_REMOVE(tcet, list); > +void spapr_tce_table_enable(sPAPRTCETable *tcet) > +{ > + uint64_t window_size =3D (uint64_t)tcet->nb_table << tcet->page_shif= t; > + > + if (tcet->enabled) { > + return; > + } > + > + if (!tcet->nb_table) { > + return; > + } > + > + if (kvm_enabled() && !(window_size >> 32)) { > + tcet->table =3D kvmppc_create_spapr_tce(tcet->liobn, > + window_size, > + &tcet->fd, > + tcet->vfio_accel); > + } > + > + if (!tcet->table) { > + size_t table_size =3D tcet->nb_table * sizeof(uint64_t); > + tcet->table =3D g_malloc0(table_size); > + } > + > + memory_region_init_iommu(&tcet->iommu, OBJECT(tcet), &spapr_iommu_op= s, > + "iommu-spapr", > + (uint64_t)tcet->nb_table << tcet->page_shif= t); > + > + tcet->enabled =3D true; > +} > + > +void spapr_tce_table_disable(sPAPRTCETable *tcet) > +{ > + if (!tcet->enabled) { > + return; > + } > =20 > if (!kvm_enabled() || > (kvmppc_remove_spapr_tce(tcet->table, tcet->fd, > tcet->nb_table) !=3D 0)) { > + tcet->fd =3D -1; > g_free(tcet->table); > } > + tcet->table =3D NULL; > + tcet->enabled =3D false; > + spapr_tce_set_props(tcet, 0, 0, 0, false); > +} > + > +static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp) > +{ > + sPAPRTCETable *tcet =3D SPAPR_TCE_TABLE(dev); > + > + QLIST_REMOVE(tcet, list); > + > + 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 52c5c73..acfdbe5 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -895,15 +895,17 @@ static void spapr_phb_finish_realize(sPAPRPHBState = *sphb, Error **errp) > sPAPRTCETable *tcet; > uint32_t nb_table; > =20 > - nb_table =3D SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT; > - tcet =3D spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, > - 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); > + tcet =3D spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn); > if (!tcet) { > error_setg(errp, "Unable to create TCE table for %s", > sphb->dtbusname); > return ; > } > =20 > + nb_table =3D SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT; > + spapr_tce_set_props(tcet, 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); > + spapr_tce_table_enable(tcet); > + > /* Register default 32bit DMA window */ > memory_region_add_subregion(&sphb->iommu_root, 0, > spapr_tce_get_iommu(tcet)); > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > index f8b503e..6c9adb5 100644 > --- a/hw/ppc/spapr_pci_vfio.c > +++ b/hw/ppc/spapr_pci_vfio.c > @@ -34,6 +34,7 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState= *sphb, Error **errp) > int ret; > sPAPRTCETable *tcet; > uint32_t liobn =3D svphb->phb.dma_liobn; > + uint32_t nb_table; > =20 > ret =3D vfio_container_ioctl(&svphb->phb.iommu_as, > VFIO_CHECK_EXTENSION, > @@ -52,16 +53,18 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBSta= te *sphb, Error **errp) > return; > } > =20 > - tcet =3D spapr_tce_new_table(DEVICE(sphb), liobn, info.dma32_window_= start, > - SPAPR_TCE_PAGE_SHIFT, > - info.dma32_window_size >> SPAPR_TCE_PAGE_= SHIFT, > - true); > + tcet =3D spapr_tce_new_table(DEVICE(sphb), liobn); > if (!tcet) { > error_setg(errp, "spapr-vfio: failed to create VFIO TCE table"); > return; > } > =20 > /* Register default 32bit DMA window */ > + nb_table =3D info.dma32_window_size >> SPAPR_TCE_PAGE_SHIFT; > + spapr_tce_set_props(tcet, info.dma32_window_start, SPAPR_TCE_PAGE_SH= IFT, > + nb_table, true); > + spapr_tce_table_enable(tcet); > + > memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > spapr_tce_get_iommu(tcet)); > } > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index 174033d..6394527 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -479,11 +479,11 @@ 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_set_props(dev->tcet, 0, SPAPR_TCE_PAGE_SHIFT, > + pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT, > + false); > + spapr_tce_table_enable(dev->tcet); > 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 7d9ab9d..6e33b9b 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -498,6 +498,7 @@ typedef struct sPAPRTCETable sPAPRTCETable; > =20 > struct sPAPRTCETable { > DeviceState parent; > + bool enabled; > uint32_t liobn; > uint32_t nb_table; > uint64_t bus_offset; > @@ -515,11 +516,12 @@ sPAPRTCETable *spapr_tce_find_by_liobn(uint32_t lio= bn); > void spapr_events_init(sPAPREnvironment *spapr); > void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq); > int spapr_h_cas_compose_response(target_ulong addr, target_ulong size); > -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, > - uint64_t bus_offset, > - uint32_t page_shift, > - uint32_t nb_table, > - bool vfio_accel); > +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn); > +void spapr_tce_set_props(sPAPRTCETable *tcet, uint64_t bus_offset, > + uint32_t page_shift, uint32_t nb_table, > + bool vfio_accel); > +void spapr_tce_table_enable(sPAPRTCETable *tcet); > +void spapr_tce_table_disable(sPAPRTCETable *tcet); > MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); > int spapr_dma_dt(void *fdt, int node_off, const char *propname, > uint32_t liobn, uint64_t window, uint32_t size); --=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 --d8Lz2Tf5e5STOWUP Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVJJPrAAoJEGw4ysog2bOSsZwQAIhr3ZCvxJmbJe5gYGYlcnsT L/rR2SJjCpbWkFMROc/q8qWg6d+czRMAok8ahF+uKzssdxf3eZQw+Rptm9Q3kyiz GcSxoJYu4c9IERsbk0L+aeZg62trfhAwUiMO9rpWL97uZbdcwRNj25I3mnefs4SP cmK7deU8ev3pDawu0pE6iI5AlmkOmR5N4Vi3tYYa9eEYKO5N/gGsw4Loomfp0sky TElusqTfsqDNki7epS5vNZIz7E8Y9w0TMe6B1tCz2TErPyu+U9meIc+aePosCSNw w6ambBczERdr7whzOw3O1xL9CnubQrCghjgaAtsJSLG/Lj+FM7lqAHQJLM4MSCJE bZpk6DCdBM8+r9dlP2gD7KuLhj9TIAcn3OPkoM7YepPgJwWjvbMJlAj8TDAnTSLb fbBi1DtlpgPaZcg8uzva97jF7/bRVvKO2gq3hr8Ih8CY99MhunQDZlaafYS1/azj QhgEZWOatKuVu5QZuRKyPXNomHvqrF3hHGtIkN1+NA5OkxOvXTVmvurdxSnuEuX6 hxjIaf/gUJ7HH785t2hk5jnUoOXU8ERlapqyMp/HLr7jDr6kui7fvvh+Pp3Dskn3 zH6g3OGsPXYunghyz7T2CnPqafCY77ldYpPMaxyubQqFPZr1JI+ErjzdGu92dYEj hyKvnnOleERSK3qb36VI =0+3t -----END PGP SIGNATURE----- --d8Lz2Tf5e5STOWUP--