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. 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
