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

[Jan Kiszka <jan.kiszka@siemens.com>]

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.

Jan

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