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 03:02 PM, Jan Kiszka wrote:
> 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.
> 

I forgot to mention that my clever rewrite above actually breaks this -
it needs to be wrapped in a transaction, otherwise we have a
move-then-disable pattern.

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

It's possible of course, but the snippet above isn't susceptible on its own.

I don't think it's solvable.  To serialize, you must hold the device
lock, but we don't want to take the device lock during dispatch.

Users can protect against them by checking for ->enabled:

void foo_io_read(...)
{
   FooState *s = container_of(mr, ...);

   qemu_mutex_lock(&s->lock);
   if (!memory_region_enabled(mr)) {
       *data = ~(uint64_t)0;
       goto out;
   }
   ...
out:
   qemu_mutex_unlock(&s->lock);
}

Non-converted users will be naturally protected since we will take the
bql on their behalf.

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