Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-28 05:38, liu ping fan wrote:
> On Tue, Aug 28, 2012 at 11:09 AM, liu ping fan <address@hidden> wrote:
>> On Tue, Aug 28, 2012 at 3:38 AM, Jan Kiszka <address@hidden> wrote:
>>> On 2012-08-27 20:52, Avi Kivity wrote:
>>>> On 08/27/2012 11:39 AM, Jan Kiszka wrote:
>>>>> On 2012-08-27 20:20, Avi Kivity wrote:
>>>>>> On 08/27/2012 11:17 AM, Jan Kiszka wrote:
>>>>>>> On 2012-08-27 20:09, Avi Kivity wrote:
>>>>>>>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>>>>>>>>
>>>>>>>>>> Deregistration is fine, the problem is destruction.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It isn't as you access memory region states that can change after
>>>>>>>>> deregistration. Devices can remove memory regions from the mapping,
>>>>>>>>> alter and then reinsert them. The last to steps must not happen while
>>>>>>>>> anyone is still using a reference to that region.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Why not?  If the guest is accessing an mmio region while reconfiguring
>>>>>>>> it in a way that changes its meaning, either the previous or the next
>>>>>>>> meaning is valid.
>>>>>>>
>>>>>>> If the memory region owner sets the content to zero or even releases it
>>>>>>> (nothing states a memory region can only live inside a device
>>>>>>> structure), we will crash. Restricting how a memory region can be
>>>>>>> created and handled after it was once registered somewhere is an
>>>>>>> unnatural interface, waiting to cause subtle bugs.
>>>>>>
>>>>>> Using an Object * allows the simple case to be really simple (object ==
>>>>>> device) and the hard cases to be doable.
>>>>>>
>>>>>> What would you suggest as a better interface?
>>>>>
>>>>> To protect the life cycle of the object we manage in the memory layer:
>>>>> regions. We don't manage devices there. If there is any implementation
>>>>> benefit in having a full QOM object, then make memory regions objects.
>>>>
>>>> I see and sympathise with this point of view.
>>>>
>>>> The idea to use opaque and convert it to Object * is that often a device
>>>> has multiple regions but no interest in managing them separately.  So
>>>> managing regions will cause the device model authors to create
>>>> boilerplate code to push region management to device management.
>>>>
>>>> I had this experience with the first iterations of the region interface,
>>>> where instead of opaque we had MemoryRegion *.  Device code would use
>>>> container_of() in the simple case, but the non-simple case caused lots
>>>> of pain.  I believe it is the natural interface, but in practice it
>>>> turned out to cause too much churn.
>>>>
>>>>> I simply don't like this indirection, having the memory layer pick up
>>>>> the opaque value of the region and interpret it.
>>>>
>>>> That's just an intermediate step.  The idea is that eventually opaque
>>>> will be changed to Object *.
>>>>
>>>>> Even worse, apply
>>>>> restrictions on how the dispatched objects, the regions, have to be
>>>>> treated because of this.
>>>>
>>>> Please elaborate.
>>>
>>> The fact that you can't manipulate a memory region object arbitrarily
>>> after removing it from the mapping because you track the references to
>>> the object that the region points to, not the region itself. The region
>>> remains in use by the dispatching layer and potentially the called
>>> device, even after deregistration.
>>
>> Why can it in use by dispatching layer? Memory region just servers as
>> interface for mem lookup, when dispatching in mr's MemoryRegionOps,
>> why do we need to access it?  Which means that read/write can cause
>> register/deregister mr?  Example?
> 
> I found example in pci_update_mappings(), but but my following point
> still stand.
>> Also I think MemoryRegionOps will eventually operate on Object (mr is
>> only interface). MRs is only represent of Object in the view of
>> memory(so their life cycle can be managed by Object). And there could
>> be some intermediate mr like those lie in subpage_t which are not
>> based on Object. But we can walk down to the terminal point and
>> extract the real Object,
>> so when dispatching, we only need to ensure that Object and its direct
>> mr are alive.

Let's not talk about devices or MMIO dispatching. I think the problem is
way more generic, and we will face it multiple times in QEMU. Besides
MMIO and PIO dispatching, it will haunt us for file or event handlers,
any kind of callbacks etc.

Context A                               Context B
---------                               ---------
                                        object = lookup()
deregister(object)
modify(object) -> invalid state
...                                     use(object)
modify(object) -> valid state
register(object)

And with "object" I'm not talking about QOM but any data structure.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux