Re: [Qemu-devel] memory: could we add extra input param for memory_region_init_io()?

[Avi Kivity]

On 08/21/2012 07:48 AM, liu ping fan wrote:
> On Sun, Aug 19, 2012 at 7:36 PM, Avi Kivity <address@hidden> wrote:
>> On 08/19/2012 02:23 PM, Peter Maydell wrote:
>>> On 19 August 2012 11:12, Avi Kivity <address@hidden> wrote:
>>>> On 08/17/2012 10:41 AM, liu ping fan wrote:
>>>>> And something like omap_mpu_timer_init() in file hw/omap1.c , the
>>>>> opaque(omap_mpu_timer_s) is got from g_malloc0, which makes things
>>>>> even harder to handle.  And the DO_CAST can not work for such issue.
>>>>
>>>> IMO omap_mpu_timer_s should be a DeviceState.  Peter?
>>>
>>> Ideally, yes, but qemu is full of devices that haven't yet made the leap
>>> to QOM.
>>>
>>> omap1 is particularly tricky because I don't actually have any test images
>>> for it, so refactoring it is a leap in the dark. [I've tried asking on this 
>>> list
>>> if anybody had test images a couple of times without success.]
>>
>> Okay.  Most of the options in this thread don't involve wholesale
>> conversion of the opaque parameter in memory_region_init_io() to type
>> Object *, so it's not necessary to convert everything.
>>
> But I think if the mr belongs to mem address space, it can not avoid
> object_ref(Object *opaque) in mmio-dispatch code. ( If we use extra
> flag to indicate whether the opaque is Object or not, then we lose the
> meaning to transfer opaque to Object type, and maybe
> memory_region_set_object() is a better choice).  

I'm not sure I understand.  What do you mean by "ability to transfer
opaque to Object type"?

However, I think I agree that we have to object_ref() during mmio
dispatch.  At that point, there are no locks taken, so nothing gurantees
the opaque is valid.

We can't take the big qemu lock since we're holding the mem map lock,
which nests inside it.  One thing we can do is retry the walk with the
qemu lock held, but that's hardly nice.  Or we can use a trylock.

If we add memory_region_set_object() that is no help either, since we
have to convert every call site.  If we do that, I prefer that we change
opaque to an Object.


> And the only exempt
> is that mr belong to io address space.

Eventually, I'd like the io address space to be dispatched by the same
code, but for now we can ignore it.

> 
> After carefully reading the code, I think
> 1. we assume Object &opaque have 1:1 relationship, if not, the Object
> can be  refactored,
>    which means children of Object (composite kid device or bus) will
> be introduced. This
>    may cause the modeling problem(but currently, can not find the example)

Agree.  The best example is device assignment, where BARs will need to
turn into Objects.  It means that an assigned device can disappear while
one of its BARs remain, so the code will have to handle this case.

> 
> 2. Some MemoryRegionOps are shared by different type of Device.
>     For example, cirrus_vga_mem_ops will handle CirrusVGAState, and
> the related Object
>     can be ISACirrusVGAState or PCICirrusVGAState. So we need to remodel the
>     CirrusVGAState as composite device of its parent,  and introducing
>     qdev_create_in_place() just like qbus_create_in_place().
> 

Alternatively, VGACommonState can be made into an Object, as in option 3.

> 3. For the case, where opaque has no Object to relate with, simply
> embeded Object at
>     the head of them. (And there are lots of such issue in current code)
> 
> 4. For pio, currently, we can ignore them.

Yes.

Unfortunately, requiring a 1:1 opaque:Object relationship means huge
churn in the code.  But I don't think it can be avoided, even with
memory_region_set_object().

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