From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58190) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkpJp-00024w-CZ for qemu-devel@nongnu.org; Wed, 22 Apr 2015 03:43:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkpJm-00073u-At for qemu-devel@nongnu.org; Wed, 22 Apr 2015 03:43:37 -0400 Date: Wed, 22 Apr 2015 16:39:03 +1000 From: David Gibson Message-ID: <20150422063903.GP31815@voom.redhat.com> References: <1428679484-15451-1-git-send-email-aik@ozlabs.ru> <1428679484-15451-12-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="dJyWBSYfjLochwFK" Content-Disposition: inline In-Reply-To: <1428679484-15451-12-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v6 11/15] spapr_pci: Do complete reset of DMA config when resetting PHB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alexander Graf , Michael Roth , qemu-devel@nongnu.org, Alex Williamson , qemu-ppc@nongnu.org, Gavin Shan --dJyWBSYfjLochwFK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Apr 11, 2015 at 01:24:40AM +1000, Alexey Kardashevskiy wrote: > On a system reset, DMA configuration has to reset too. At the moment > it clears the table content. This is enough for the single table case > but with DDW, we will also have to disable all DMA windows except > the default one. Furthermore according to sPAPR, if the guest removed > the default window and created a huge one at the same zero offset on > a PCI bus, the reset handler has to recreate the default window with > the default properties (2GB big, 4K pages). >=20 > This reworks SPAPR PHB code to disable the existing DMA window on reset > and then configure and enable the default window. > Without DDW that means that the same window will be disabled and then > enabled with no other change in behaviour. >=20 > This changes the table creation to do it in one place in PHB (VFIO PHB > just inherits the behaviour from PHB). The actual table allocation is > done from the reset handler and this is where finish_realize() is called. Looks like your commit message needs an update - you already got rid of finish_realize() at this point in the series. > This disables all DMA windows on a PHB reset. It does not make any > difference now as there is just one DMA window but it will later with DDW > patches. >=20 > spapr_phb_dma_reset() and spapr_phb_dma_remove_window() will be used > in following patch so make it public. >=20 > Signed-off-by: Alexey Kardashevskiy > --- > hw/ppc/spapr_pci.c | 45 ++++++++++++++++++++++++++++++++++++---= ------ > hw/ppc/spapr_pci_vfio.c | 6 ------ > include/hw/pci-host/spapr.h | 3 +++ > 3 files changed, 39 insertions(+), 15 deletions(-) >=20 > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 664687c..3d40f5b 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -736,7 +736,6 @@ static void spapr_phb_realize(DeviceState *dev, Error= **errp) > SysBusDevice *s =3D SYS_BUS_DEVICE(dev); > sPAPRPHBState *sphb =3D SPAPR_PCI_HOST_BRIDGE(s); > PCIHostState *phb =3D PCI_HOST_BRIDGE(s); > - sPAPRPHBClass *info =3D SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s); > char *namebuf; > int i; > PCIBus *bus; > @@ -887,14 +886,6 @@ static void spapr_phb_realize(DeviceState *dev, Erro= r **errp) > return; > } > =20 > - info->dma_capabilities_update(sphb); > - info->dma_init_window(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, > - sphb->dma32_window_size); > - tcet =3D spapr_tce_find_by_liobn(sphb->dma_liobn); > - if (!tcet) { > - error_setg(errp, "failed to create TCE table"); > - return; > - } > memory_region_add_subregion(&sphb->iommu_root, 0, > spapr_tce_get_iommu(tcet)); > =20 > @@ -923,6 +914,40 @@ static int spapr_phb_dma_init_window(sPAPRPHBState *= sphb, > return 0; > } > =20 > +int spapr_phb_dma_remove_window(sPAPRPHBState *sphb, > + sPAPRTCETable *tcet) The only caller of this is just below, why is it not static? > +{ > + spapr_tce_table_disable(tcet); > + > + return 0; > +} > + > +static int spapr_phb_disable_dma_windows(Object *child, void *opaque) > +{ > + sPAPRPHBState *sphb =3D SPAPR_PCI_HOST_BRIDGE(opaque); > + sPAPRTCETable *tcet =3D (sPAPRTCETable *) > + object_dynamic_cast(child, TYPE_SPAPR_TCE_TABLE); > + > + if (tcet) { > + spapr_phb_dma_remove_window(sphb, tcet); > + } > + > + return 0; > +} > + > +int spapr_phb_dma_reset(sPAPRPHBState *sphb) > +{ > + const uint32_t liobn =3D SPAPR_PCI_LIOBN(sphb->index, 0); > + sPAPRPHBClass *spc =3D SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + > + spc->dma_capabilities_update(sphb); /* Refresh @has_vfio status */ > + object_child_foreach(OBJECT(sphb), spapr_phb_disable_dma_windows, sp= hb); > + spc->dma_init_window(sphb, liobn, SPAPR_TCE_PAGE_SHIFT, > + sphb->dma32_window_size); > + > + return 0; > +} > + > static int spapr_phb_children_reset(Object *child, void *opaque) > { > DeviceState *dev =3D (DeviceState *) object_dynamic_cast(child, TYPE= _DEVICE); > @@ -936,6 +961,8 @@ static int spapr_phb_children_reset(Object *child, vo= id *opaque) > =20 > static void spapr_phb_reset(DeviceState *qdev) > { > + spapr_phb_dma_reset(SPAPR_PCI_HOST_BRIDGE(qdev)); > + > /* Reset the IOMMU state */ > object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL); > } > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > index a5b97d0..f89e053 100644 > --- a/hw/ppc/spapr_pci_vfio.c > +++ b/hw/ppc/spapr_pci_vfio.c > @@ -58,11 +58,6 @@ static int spapr_phb_vfio_dma_init_window(sPAPRPHBStat= e *sphb, > return 0; > } > =20 > -static void spapr_phb_vfio_reset(DeviceState *qdev) > -{ > - /* Do nothing */ > -} > - > static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, > unsigned int addr, int option) > { > @@ -176,7 +171,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *kl= ass, void *data) > sPAPRPHBClass *spc =3D SPAPR_PCI_HOST_BRIDGE_CLASS(klass); > =20 > dc->props =3D spapr_phb_vfio_properties; > - dc->reset =3D spapr_phb_vfio_reset; > spc->dma_capabilities_update =3D spapr_phb_vfio_dma_capabilities_upd= ate; > spc->dma_init_window =3D spapr_phb_vfio_dma_init_window; > spc->eeh_set_option =3D spapr_phb_vfio_eeh_set_option; > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > index 3074145..7fda78e 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -140,5 +140,8 @@ void spapr_pci_rtas_init(void); > sPAPRPHBState *spapr_pci_find_phb(sPAPREnvironment *spapr, uint64_t buid= ); > PCIDevice *spapr_pci_find_dev(sPAPREnvironment *spapr, uint64_t buid, > uint32_t config_addr); > +int spapr_phb_dma_remove_window(sPAPRPHBState *sphb, > + sPAPRTCETable *tcet); > +int spapr_phb_dma_reset(sPAPRPHBState *sphb); > =20 > #endif /* __HW_SPAPR_PCI_H__ */ --=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 --dJyWBSYfjLochwFK Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVN0IGAAoJEGw4ysog2bOSlmMQALrjGNKoOTfVaP5YKQ2G3CUi 5/PWMQVe1INKg1toKibPWfheAUhEX4qMRvNdS/GubqYd0MSnLVLNf3FRakdvkDoR OcuYGL2/4/HjDF/ijWu7z/QfEORmUY3vsgm/LAEJs1b3lZvd5fkfRC3bt4/mqy3k 0b/onBaLEXtDWfcWd8XvgQRCHgTt0UuzzSgkN7SFPuAhKyLd1qomtssqLFqajVGI G7vUErDD5hwx/U9Zu+qToqjDtoMUas/1v2AIa5ub1EUPwScasQHE91RY18oQPN57 PiD95gn3ZzZJqn8pPOf1b0vbJdBOKnKtTZe9b0B6Yi5lv29QTqLgibzIMX5AkPcy TshC2Qvo+R8MZDn9nla8ucxzCnFGJZad6Cqh/Z02VhHGNqoFtDoD62p4MSx50H4S Xff9w8nHXBminrOuVvMgkXOUYIU7CCd8ss0/nXM6BgDFUR3PHHJYXMH2nu9QQwyh iUANb4xnUxWwNpzh/nsW/lil/RGl/PME8weLgbnb9FFxB1gNy7oXYQrnzVkpl3tE LbAvZ2pbnd5Ec1OPhFNuYBoxQxbFsg30UfX0ZyE7rmiC17tzPkGhe1DGWXH1gXs/ K7rp9/smidhItBI5BdklwwWi42rq22kDM0/HQYpp1KUgUJ4gLqPHoXIHV3Jt1JKz D0BdhP57d2tx6U7spxzH =tR4t -----END PGP SIGNATURE----- --dJyWBSYfjLochwFK--