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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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

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?

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.

Paolo