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

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

On 2013-05-03 09:37, liu ping fan wrote:
> On Fri, May 3, 2013 at 12:58 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Hi Pingfan,
>>
>> On 2012-12-06 08:28, liu ping fan wrote:
>>> Any suggestion? Or new design idea for this?
>>
>> Finally... I'm getting back to this. I'm currently trying to make use of
>> this series, adapting it to my needs (selective BQL-free dispatching of
>> PIO regions).
>>
> Glad that you are back :)
> 
>> Is there a newer version available on your side? This one obviously no
> 
> No, but I can see the code and rebase next week.

I've already rebased and started to fix/cleanup. Today I will look into
the ref/unref vs. Object topic and try a first unlocked device model. I
can share afterward so that we do not need to do the work twice.

> 
>> longer applies due to all the code movements in QEMU. But it also seems
>> to contain some bugs, at least in patch 5 (mixed up page number vs. page
>> address around for address_space_section_lookup_ref).
>>
> Will pay some time to see it.

Fixed. Specifically subpages were broken. As I've converted portio to
use the memory core dispatcher, I'm getting a lot of them now and
triggered the bugs immediately.

There is still some nesting issue around coalesced MMIO flushing. Need
to look into this today as well. So far I've worked around it by
assuming that nesting is fine if we enter the dispatcher with BQL held.

> 
>> Then we should get rid of the ref/unref callbacks. Making a memory
>> region BQL-free must be as simple as setting a flag or (more likely)
>> adding a reference to the owning QOM object in the region.
>> Reimplementing ref/unref in device models over and over again is clearly
>> a no-go. Maybe I'm currently forgetting a use case where overloading the
> 
> At the beginning, Avi suggest to enforce mr->opaque to be Device
> object, but due to the nested embedded Object, we fail. And finally
> Avi suggest ref/unref interface.
> From my point,  we can save lots of reimplementing ref/unref in device
> models by telling whether mr->opauque is Object or not.  And leave not
> object case to reimplement ref/unref.

We can't change the semantics of opaque as long as old_mmio / old_portio
are around. But we need a flag anyway to indicate if a region is
depending on BQL or not. Adding a separate "Object *owner" to
MemoryRegion can serve both purposes. Then we define something like

void memory_region_set_local_locking(MemoryRegion *mr,
                                     bool local_locking,
                                     Object *owner);

to control the property (if local_locking is true, owner must be
non-NULL, of course). That's quite similar to my old prototype here that
had memory_region_set/clear_global_locking.

Jan

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