From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52009) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6CiI-0004oA-8j for qemu-devel@nongnu.org; Fri, 27 May 2016 04:01:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b6Ci6-0006yC-Ap for qemu-devel@nongnu.org; Fri, 27 May 2016 04:01:45 -0400 Received: from mail-pf0-x241.google.com ([2607:f8b0:400e:c00::241]:35005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6Ci6-0006xs-0U for qemu-devel@nongnu.org; Fri, 27 May 2016 04:01:34 -0400 Received: by mail-pf0-x241.google.com with SMTP id f144so5164458pfa.2 for ; Fri, 27 May 2016 01:01:32 -0700 (PDT) References: <1462344751-28281-1-git-send-email-aik@ozlabs.ru> <1462344751-28281-9-git-send-email-aik@ozlabs.ru> <20160526033902.GA17226@voom.fritz.box> From: Alexey Kardashevskiy Message-ID: <628d642c-d935-e33b-8c6d-8c3511d734fc@ozlabs.ru> Date: Fri, 27 May 2016 18:01:25 +1000 MIME-Version: 1.0 In-Reply-To: <20160526033902.GA17226@voom.fritz.box> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Sq4K7cORuRgesgAxbnBB3qcC2LQhnoliM" Subject: Re: [Qemu-devel] [PATCH qemu v16 08/19] spapr_iommu: Introduce "enabled" state for TCE table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alexander Graf , Alex Williamson , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Sq4K7cORuRgesgAxbnBB3qcC2LQhnoliM From: Alexey Kardashevskiy To: David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alexander Graf , Alex Williamson , Paolo Bonzini Message-ID: <628d642c-d935-e33b-8c6d-8c3511d734fc@ozlabs.ru> Subject: Re: [PATCH qemu v16 08/19] spapr_iommu: Introduce "enabled" state for TCE table References: <1462344751-28281-1-git-send-email-aik@ozlabs.ru> <1462344751-28281-9-git-send-email-aik@ozlabs.ru> <20160526033902.GA17226@voom.fritz.box> In-Reply-To: <20160526033902.GA17226@voom.fritz.box> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: quoted-printable On 26/05/16 13:39, David Gibson wrote: > On Wed, May 04, 2016 at 04:52:20PM +1000, 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 windo= ws >> 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 creati= on. >> It still will be called once at the owner object (VIO or PHB) creation= =2E >> >> 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 initializatio= n >> 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. >> >> 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_ena= ble() >> as later it will be called at the sPAPRTCETable post-migration stage w= hen >> it already has all the properties set after the migration; the same is= >> done for spapr_tce_table_disable(). >> >> Signed-off-by: Alexey Kardashevskiy >> Reviewed-by: David Gibson >> --- >> Changes: >> v15: >> * made adjustments after removing spapr_phb_dma_window_enable() >> >> v14: >> * added spapr_tce_table_do_disable(), will make difference in followin= g >> patch with fully dynamic table migration >> >> # Conflicts: >> # hw/ppc/spapr_pci.c >> --- >> hw/ppc/spapr_iommu.c | 86 ++++++++++++++++++++++++++++++++++++-----= --------- >> hw/ppc/spapr_pci.c | 8 +++-- >> hw/ppc/spapr_vio.c | 8 ++--- >> include/hw/ppc/spapr.h | 10 +++--- >> 4 files changed, 75 insertions(+), 37 deletions(-) >> >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >> index 8132f64..9bcd3f6 100644 >> --- a/hw/ppc/spapr_iommu.c >> +++ b/hw/ppc/spapr_iommu.c >> @@ -17,6 +17,7 @@ >> * License along with this library; if not, see . >> */ >> #include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> #include "hw/hw.h" >> #include "sysemu/kvm.h" >> #include "hw/qdev.h" >> @@ -174,15 +175,9 @@ static int spapr_tce_table_realize(DeviceState *d= ev) >> 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); >> - >> + tcet->need_vfio =3D false; >> memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_= ops, >> - "iommu-spapr", >> - (uint64_t)tcet->nb_table << tcet->page_s= hift); >> + "iommu-spapr", 0); >> =20 >> QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); >> =20 >> @@ -224,14 +219,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet= , bool 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 +230,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *o= wner, 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 +241,69 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *= owner, 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_shi= ft); >> + >> + tcet->enabled =3D true; >> +} >> + >> +void spapr_tce_table_enable(sPAPRTCETable *tcet, >> + uint32_t page_shift, uint64_t bus_offset,= >> + uint32_t nb_table) >> +{ >> + if (tcet->enabled) { >> + error_report("Warning: trying to enable already enabled TCE t= able"); >> + return; >> + } >> + >> + tcet->bus_offset =3D bus_offset; >> + tcet->page_shift =3D page_shift; >> + tcet->nb_table =3D nb_table; >> + >> + spapr_tce_table_do_enable(tcet); >> +} >> + >> +static void spapr_tce_table_do_disable(sPAPRTCETable *tcet) >> +{ >> + 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; >> +} >> + >> +static void spapr_tce_table_disable(sPAPRTCETable *tcet) >> +{ >> + if (!tcet->enabled) { >> + error_report("Warning: trying to disable already disabled TCE= table"); >> + return; >> + } >> + spapr_tce_table_do_disable(tcet); >> +} >> + >> 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 > This should probably be do_disable(), or you'll get a spurious error > if you start and stop a VM but don't enable the table in between, or > if the guest disables all the tables before the shutdown. Well, I'll change this as you say (seems more correct) but since unrealize() can only be called on PHB hotunplug and we do not support thi= s, this code won't execute. --=20 Alexey --Sq4K7cORuRgesgAxbnBB3qcC2LQhnoliM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXR/7VAAoJEIYTPdgrwSC5uJwP/3jKwdjZ9Rlc/FmyOnmrZCSm b2MS6zM3lPJxdyUZmwd1ho24t96gexClrlJSHLazmrDhgW0srsbXocTngxe53tZI NYnchEh3K9gkl+XhthexxU71mlSAVGddVy6IBBV3N3qt1WvJhGV9P4cS9U5QBXcA ZJYdwRrOX55aL/tsuxlo4vN5YMoVBs5rCYCsyfmxr+paB+YQsUM7XItMixrukeEB ODoYESieTDWrRH99+Ul9MtmbU6SkR1zWIzYQ/FeW7GGCyC2attyFEolxPLwvWRcp +LlHC/6L9kpyHZCdZOW57K14kM3wFBXPZzysd6YmBDdMx5Mp+8qe4jGt83+Dyyjr biJCrhj3IBgxq9SygT5hTuRgqrTu/zuMbxi+OAxHEL5WoAY2DsXaTqdIM34JFKoi d+CFsFYCEb2DPzLGK8HVnCwKF4KGZ3hrcumsR2heM0Ku0z0AmEdd89Djn6KGcMLu +g0AqFEEXKAXUKRNhfcD3V143zCl0IxyMUg3L4267MGKKqavwq5jbfO4vFFonXsR xWvYClh1Q+Lks6cKtTvu5sdwfZW4RKu9Auv3mnnbbeQf+luF1rxPJJd55rg/XHp7 BKfNCMSHurQtjdvRdjFOjvURj9gM/CnLyGERaG0FX06kNqsL0tGW7jvOBdoujnSB SubOoVFxRJ2kev+53dzC =14FH -----END PGP SIGNATURE----- --Sq4K7cORuRgesgAxbnBB3qcC2LQhnoliM--