On 08/21/2012 12:25 PM, liu ping fan wrote: > On Tue, Aug 21, 2012 at 4:57 PM, Avi Kivity <a...@redhat.com> wrote: >> On 08/21/2012 07:48 AM, liu ping fan wrote: >>> On Sun, Aug 19, 2012 at 7:36 PM, Avi Kivity <a...@redhat.com> wrote: >>>> On 08/19/2012 02:23 PM, Peter Maydell wrote: >>>>> On 19 August 2012 11:12, Avi Kivity <a...@redhat.com> 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: 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: 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? >> 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