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

[liu ping fan]

On Tue, Aug 21, 2012 at 6:13 PM, Avi Kivity <address@hidden> wrote:
> On 08/21/2012 12:25 PM, liu ping fan wrote:
>> 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.
>
> That is what I meant.
>
>> 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.
>
> Option 2, for example, had MemoryRegionOps::object(MemoryRegion *) which
> can be used to convert the opaque to an Object, or return NULL.  With
> that option, there would be no corruption:
>
If we did not pass extra flag to indicate whether opaque is Object or
not, we can not implement MemoryRegionOps::object().  Because we lack
of something like "try,catch", and object_dynamic_cast() can not work.
So besides memory_region_init_io(), we need extra interface to pass that flag?

>   qemu_mutex_lock(&mem_map_lock);
>   MemoryRegionSection mrs = walk(&phys_map);
>   Object *object = mrs.mr->ops->object(mrs.mr);
>   if (object) {
>       object_ref(object);
>   }
>   qemu_mutex_unlock(&mem_map_unlock);
>
> So there is no memory corruption.  However, as I point out below, we
> also lack the reference count.  Maybe we could do something like:
>
I think the hotplug issue is only for limited type of bus. And
fortunately, for pci, they have already adopt Object.
So for other SoC, maybe we can ignore this issue at current step

>   qemu_mutex_lock(&mem_map_lock);
>   MemoryRegionSection mrs = walk(&phys_map);
>   Object *object = mrs.mr->ops->object(mrs.mr);
>   if (object) {
>       object_ref(object);
>   } else {
>       goto retry_with_qemu_lock_held;
>   }
>   qemu_mutex_unlock(&mem_map_unlock);
>
> But that's very inelegant.
>
>>>
>>> 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?
>
> If we can avoid a cycle.  Won't the device need to hold refs to the BAR?

I will check the code, but I has thought BAR is implemented by
assigned device?  Will study closely.

Thanks and regards,
pingfan

>>> 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.
>
> Ok.  One one hand, I think the conversion is unavoidable, and is also
> beneficial: I never liked opaques.
>
> On the other hand, those conversions are very tedious, introduce
> regressions, and delay the result.
>
> Anthony, any insight on this?
>
> --
> error compiling committee.c: too many arguments to function