From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxGH4-000467-Gy for qemu-devel@nongnu.org; Tue, 26 May 2015 10:56:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxGH1-0004qV-9M for qemu-devel@nongnu.org; Tue, 26 May 2015 10:56:10 -0400 Received: from e19.ny.us.ibm.com ([129.33.205.209]:48087) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxGH0-0004q9-W4 for qemu-devel@nongnu.org; Tue, 26 May 2015 10:56:07 -0400 Received: from /spool/local by e19.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 26 May 2015 10:56:06 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <55648239.7070905@redhat.com> References: <1429964684-23872-1-git-send-email-aik@ozlabs.ru> <55646C18.4000303@redhat.com> <5564750C.8000100@ozlabs.ru> <556475BD.50401@redhat.com> <55647843.4040609@ozlabs.ru> <556479B6.1010501@redhat.com> <55647C75.5000704@ozlabs.ru> <55647D4C.6060008@redhat.com> <55648086.3010804@ozlabs.ru> <55648239.7070905@redhat.com> Message-ID: <20150526145557.4646.3678@loki> Date: Tue, 26 May 2015 09:55:57 -0500 Subject: Re: [Qemu-devel] [PATCH qemu v7 06/14] spapr_iommu: Introduce "enabled" state for TCE table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Alexey Kardashevskiy , David Gibson Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf Quoting Paolo Bonzini (2015-05-26 09:24:57) > = > = > On 26/05/2015 16:17, Alexey Kardashevskiy wrote: > > On 05/27/2015 12:03 AM, Paolo Bonzini wrote: > >> > >> > >> On 26/05/2015 16:00, Alexey Kardashevskiy wrote: > >>> On 05/26/2015 11:48 PM, Paolo Bonzini wrote: > >>>> > >>>> > >>>> On 26/05/2015 15:42, Alexey Kardashevskiy wrote: > >>>>> > >>>>> > >>>>> The next patch of this patchset changes: > >>>>> spapr_tce_table_do_enable() > >>>>> memory_region_init_iommu(&iommu) > >>>>> memory_region_add_subregion(&root, &iommu) > >>>>> > >>>>> spapr_tce_table_disable() > >>>>> memory_region_del_subregion(&root, &iommu) > >>>>> object_unref(&iommu) > >>>>> > >>>>> These spapr_tce_xxx are called by request from the guest. &root is a > >>>>> container and exists as long as sPAPRTCETable exists. > >>>>> > >>>>> Where do I get a leaking child property here? > >>>> > >>>> When you unref iommu and not unparent it. The next > >>>> memory_region_init_iommu creates a second child property, and the fi= rst > >>>> is gone. > >>> > >>> But when do I get this child property? In memory_region_add_subregion= ()? > >>> And memory_region_del_subregion() does not do the opposite thing > >>> (unparent)? > >> > >> In memory_region_init_iommu. > > = > > Ah. So I need at least s/object_unref/object_unparent/ in my current > > code, right? > = > Yes, and then you hit the situation documented in docs/memory.txt. > = > >> Why do you need different regions? Why can't you have always the same > >> IOMMU regions, and either: > > = > > They may change a size. > = > That's not a problem, there's memory_region_set_size for that. What on earth, I could've sworn I looked for this... yes I think that would solve the issue here. mr_add/mr_del can handle the change in offsets, set size can deal with the change and size, and we can then move to using an MR allocated at IOMMU creation time. > = > > These are dynamic DMA windows, guest may remove > > all and create randomly. Each region is backed by a separate TCE table > > with different page size. > = > Okay. > = > >> 1) create/destroy an alias to that region > > = > > How does this change things compared to iommus in regard to parenting? > = > Aliases do not have the same restriction. But this doesn't help your > case if you have separate TCE tables etc. > = > >> 2) change the behavior of the translation function, while keeping a > >> single region? > > = > > Have one sPAPRTCETable object with 0, 1 or 2 (and potentially more) > > actual TCE tables? I can do that too but I thought subregions are just > > natural for that. > = > They may be. You may need more than one though. > = > What guest actions trigger the change? Is it a hypercall? If so, what > hypercall is it so I can look at the documentation? > = > > I even wanted to create sPAPRTCETable' dynamically but > > this would break migration (because we cannot start QEMU with an > > additional sPAPRTCETable if it exists in the source which is not always > > the case). > = > Creating sPAPRTCETables dynamically would be a fix as well. You _can_ > unparent the sPAPRTCETable whenever you want. But it's not necessarily > the right solution. Yah, I think this would work too, simply resizing the IOMMU MR seems more straightforward in our case though. > = > Why does it break migration? There is only one migration handler for > all htabs, I think. Or is this a different thing than the htabs? I think the issue was that migration expects all objects in destination to be instantiated prior to the start of migration, so any scheme where the IOMMU objects are creating/destroyed at essentially random times causes problems in terms of figuring out where to load in the migrated TCE tables. > = > The sPAPRTCETable would be created in its parent device's post_load handl= er. > = > > Ok. I'll redo this thing again and try using less QOM objects... > = > Wait, I haven't understood the problem yet. AFAIK you've given us an ideal solution using memory_region_set_size() so we can avoid the dynamic MR creation during reset. Not sure if there's anything else that's missing. > = > Paolo >=20