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 first > >>>> 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 handler. > > > 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 >
