From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58076) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC9oh-00079h-Dz for qemu-devel@nongnu.org; Mon, 06 Jul 2015 13:04:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZC9oc-0004bd-Eo for qemu-devel@nongnu.org; Mon, 06 Jul 2015 13:04:27 -0400 Date: Mon, 6 Jul 2015 19:04:10 +0200 From: Thomas Huth Message-ID: <20150706190410.6fac8db1@thh440s> In-Reply-To: <1436148670-6592-6-git-send-email-aik@ozlabs.ru> References: <1436148670-6592-1-git-send-email-aik@ozlabs.ru> <1436148670-6592-6-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v10 05/14] spapr_iommu: Introduce "enabled" state for TCE table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Michael Roth , qemu-devel@nongnu.org, Gavin Shan , Alex Williamson , qemu-ppc@nongnu.org, David Gibson On Mon, 6 Jul 2015 12:11:01 +1000 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. > > 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. > > 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 and allocates > a guest view of the TCE table (in the user space or KVM). > spapr_tce_table_disable() disposes the table. > > Follow up patches will disable+enable tables on reset (system reset > or DDW 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 migration 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_enable() > as later it will be called at the sPAPRTCETable post-migration stage when > it has all the properties set after the migration. > > Signed-off-by: Alexey Kardashevskiy > --- ... > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index c1ca13d..3ddd72f 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -821,13 +821,12 @@ static int spapr_phb_dma_init_window(sPAPRPHBState *sphb, > uint64_t window_size) > { > uint64_t bus_offset = sphb->dma32_window_start; > - sPAPRTCETable *tcet; > + sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn); > > - tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift, > - window_size >> page_shift, > - false); > - > - return tcet ? 0 : -1; > + spapr_tce_table_enable(tcet, bus_offset, page_shift, > + window_size >> page_shift, > + false); > + return 0; > } Would it be possible that this function is called with a window_size where window_size >> page_shift results in 0? For example triggered by a guest with a bad value for the RTAS call in rtas_ibm_create_pe_dma_window() ? In that case, the enablement would fail, but you'd still return 0 for success here. ==> Maybe add a check for window_size >> page_shift == 0 and return an error code in that case? Thomas