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

[Avi Kivity <avi@redhat.com>]

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