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

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

On 2012-09-05 13:25, Avi Kivity wrote:
> On 09/05/2012 02:11 PM, Jan Kiszka wrote:
>> On 2012-09-05 12:53, Avi Kivity wrote:
>>> On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>>>>>
>>>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>>>>> corresponding unref), which has the following requirements:
>>>>>
>>>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>>>>> - if the refcount is nonzero, the MemoryRegion itself is stable.
>>>>
>>>> The second point means that the memory subsystem will cache the region
>>>> state and apply changes only after leaving a handler that performed them?
>>>
>>> No.  I/O callbacks may be called after a region has been disabled.
>>>
>>> I guess we can restrict this to converted regions, so we don't introduce
>>> regressions.
>>
>> s/can/have to/. This property change will require some special care,
>> hopefully mostly at the memory layer. A simple scenario from recent patches:
>>
>>     if (<enable>) {
>>         memory_region_set_address(&s->pm_io, pm_io_base);
>>         memory_region_set_enabled(&s->pm_io, true);
>>     } else {
>>         memory_region_set_enabled(&s->pm_io, false);
>>     }
> 
> I am unable to avoid pointing out that this can be collapsed to
> 
>   memory_region_set_address(&s->pm_io, pm_io_base);
>   memory_region_set_enabled(&s->pm_io, <enable>);
> 
> as the address is meaningless when disabled. Sorry.
> 
>>
>> We will have to ensure that set_enabled(..., true) will never cause a
>> dispatch using an outdated base address.
> 
> No, this is entirely safe.  If the guest changes a region offset
> concurrently with issuing mmio on it, then it must expect either the old
> or new offset to be used during dispatch.

You assume disable, reprogram, enable, ignoring that there could be
multiple, invalid states between disable and enable. Real hardware takes
care of the ordering.

>  In either case, the correct
> intra-region offset will be provided to the I/O callback (no volatile
> MemoryRegion fields except ->readable (IIRC) are used during dispatch -
> the rest are all copied into data structures used during dispatch (this
> is part of what makes the whole thing so rcu friendly).
> 
>> I think discussing semantics and usage patterns of the new memory API -
>> outside of the BQL - will be the next big topic. ;)
> 
> I hope it won't prove to be that complicated.
> 

Yeah, let's hope this...

Jan

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