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

[liu ping fan]

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.
>
> Regards,
> pingfan
>>
>> Jan
>>
>> --
>> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
>> Corporate Competence Center Embedded Linux