Re: [Qemu-devel] [PATCH v7 0/7] push mmio dispatch out of big lock

[Paolo Bonzini]

Il 06/05/2013 13:11, Jan Kiszka ha scritto:
> On 2013-05-06 12:58, Paolo Bonzini wrote:
>> Il 06/05/2013 12:56, Jan Kiszka ha scritto:
>>>> The problem is that even if I/O for a region is supposed to happen
>>>> within the BQL, lookup can happen outside the BQL.  Lookup will use the
>>>> region even if it is just to discard it:
>>>>
>>>>            VCPU thread (under BQL)              device thread
>>>>  
>>>> --------------------------------------------------------------------------------------
>>>>                                                 flatview_ref
>>>>                                                 memory_region_find returns 
>>>> d->mr
>>>>                                                 memory_region_ref(d->mr) 
>>>> /* nop */
>>>>            qdev_free(d)
>>>>              object_unparent(d)
>>>>                unrealize(d)
>>>>                  memory_region_del_subregion(d->mr)
>>>>                    FlatView updated, d->mr not in the new view
>>>>
>>>>                                                 flatview_unref
>>>>                                                   
>>>> memory_region_unref(d->mr)
>>>>                                                     object_unref(d)
>>>>                                                       free(d)
>>>>                                                 if (!d->mr->is_ram) {      
>>>>   /* BAD! */
>>>>                                                   
>>>> memory_region_unref(d->mr) /* nop */
>>>>                                                   return error
>>>>                                                 }
>>>>
>>>>
>>>> Here, the memory region is dereferenced *before* we know that it is 
>>>> BQL-free
>>>> (in fact, exactly to ascertain whether it is BQL-free).
>>>
>>> Both flatview update and lookup *plus* locking type evaluation (i.e.
>>> memory region dereferencing) always happen under the address space lock.
>>> See Pingfan's patch.
>>
>> That's true of address_space_rw/map, but I don't think it holds for
>> memory_region_find.
> 
> It has to, or it would be broken: Either it is called on a region that
> supports reference counting

You cannot know that in advance, can you?  The address is decided by the
guest.

> and, thus, increments the counter before
> returning, or it has to be called with the BQL held.

... or we need to support reference counting on all regions, so that the
other possibility is automatically true.

Strictly speaking, only regions that can be unplugged need to support
reference counting.


Paolo