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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Alexey Kardashevskiy (2015-05-26 09:17:42)
> 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?

I've actually tried that. In that case, FlatView still holds a
reference to the region, and when RCU thread finally unrefs, it sees
that the MR no longer as an owner in the code below:

void memory_region_unref(MemoryRegion *mr)
{
    Object *obj = OBJECT(mr);
    if (obj && obj->parent) {
        object_unref(obj->parent);
    } else {
        object_unref(obj);
    }
    g_free(ref_tag);
}

Since the region, prior to the object_unparent(), had an owner, it
gave it's ownership over to the owner (which takes a ref on it as
a child property) and unref'd itself. Now that it's been orphaned,
any attempts by RCU to call memory_region_unref() on it result in
it MR attempting to unref itself, rather than it's owner, but it's
ref is already 0: object_unparent() caused it to be finalized already,
so we actually end up triggering the same assertion as with
object_unref: g_assert(obj->ref > 0);

When we use object_unref() instead of object_unparent(), we hit that
same assertion when the owner get's finalized, since it still sees the
MR as a child and attempts to unref it again.

> 
> >>> What is different between the various IOMMU regions, so that you cannot
> >>> create just one?
> >>
> >> There are two DMA windows on the same PCI bus (in hardware too), at
> >> different offset and with a different page size.
> >
> > Why do you need different regions?  Why can't you have always the same
> > IOMMU regions, and either:
> 
> They may change a size. 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.
> 
> > 1) create/destroy an alias to that region
> 
> How does this change things compared to iommus in regard to parenting?
> 
> 
> > 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. 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).
> 
> Ok. I'll redo this thing again and try using less QOM objects...
> 
> 
> -- 
> Alexey
>