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

[Avi Kivity <avi@redhat.com>]

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


-- 
error compiling committee.c: too many arguments to function