Re: [Qemu-ppc] [PATCH qemu v7 06/14] spapr_iommu: Introduce "enabled" state for TCE table

[Michael Roth]

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
>