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

[liu ping fan]

On Tue, Aug 21, 2012 at 4:57 PM, Avi Kivity <address@hidden> wrote:
> 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"?
>
Maybe I misunderstand your meaning of "Okay.  Most of the options in
this thread don't involve wholesale ..., so it's not necessary to
convert everything"
I think you mean that "omap_mpu_timer_s" need NOT to convert.  But as
it will also take the code path which has object_ref(Object*), so it
has to convert, otherwise the code will corrupt.
That is what I want to express.

> 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.
>
OK
>>
>> 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.
>
How about the initialization of BAR inc ref of assigned device; and
finalization of BAR dec it?
>>
>> 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.
>
NO, as for case 2, I think the composite device will hold ref of its
parent. Otherwise, we can not prevent  the PCICirrusVGAState from
disappearing when mmio-dispatch.  But as for case 3, it just work
around in current step. And usually, the Object is isolated. Maybe in
future, we can fix them.

>> 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().
>
Yeah, and I am studying the different case to see how to resolve and
make plans about the huge conversion.

Regards,
pingfan
> --
> error compiling committee.c: too many arguments to function