qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] memory: could we add extra input param for memory_regio


From: liu ping fan
Subject: Re: [Qemu-devel] memory: could we add extra input param for memory_region_init_io()?
Date: Fri, 17 Aug 2012 15:41:12 +0800

On Fri, Aug 17, 2012 at 10:52 AM, liu ping fan <address@hidden> wrote:
> On Thu, Aug 16, 2012 at 5:23 PM, Avi Kivity <address@hidden> wrote:
>> On 08/16/2012 06:22 AM, liu ping fan wrote:
>>> On Tue, Aug 14, 2012 at 6:49 PM, Avi Kivity <address@hidden> wrote:
>>>> On 08/14/2012 11:30 AM, liu ping fan wrote:
>>>>> To make memoryRegion survive without the protection of qemu big lock,
>>>>> we need to pin its based Object.
>>>>> In current code, the type of mr->opaque are quite different.  Lots of
>>>>> them are Object, but quite of them are not yet.
>>>>>
>>>>> The most challenge for changing from memory_region_init_io(..., void
>>>>> *opaque, ...) to memory_region_init_io(..., Object *opaque,...) is
>>>>> such codes:
>>>>> hw/ide/cmd646.c:
>>>>> static void setup_cmd646_bar(PCIIDEState *d, int bus_num)
>>>>> {
>>>>>     IDEBus *bus = &d->bus[bus_num];
>>>>>     CMD646BAR *bar = &d->cmd646_bar[bus_num];
>>>>>
>>>>>     bar->bus = bus;
>>>>>     bar->pci_dev = d;
>>>>>     memory_region_init_io(&bar->cmd, &cmd646_cmd_ops, bar, "cmd646-cmd", 
>>>>> 4);
>>>>>     memory_region_init_io(&bar->data, &cmd646_data_ops, bar, 
>>>>> "cmd646-data", 8);
>>>>> }
>>>>> If we passed in mr's based Object @d to substitute @bar, then we can
>>>>> not pass the extra info @bus_num.
>>>>>
>>>>> To solve such issue, introduce extra member "Object *base" for 
>>>>> MemoryRegion.
>>>>>
>>>>> diff --git a/memory.c b/memory.c
>>>>> index 643871b..afd5dea 100644
>>>>> --- a/memory.c
>>>>> +++ b/memory.c
>>>>> @@ -931,6 +931,7 @@ static void memory_region_dispatch_write(MemoryRegion 
>>>>> *mr,
>>>>>
>>>>>  void memory_region_init_io(MemoryRegion *mr,
>>>>>                             const MemoryRegionOps *ops,
>>>>> +                           Object *base,
>>>>>                             void *opaque,
>>>>>                             const char *name,
>>>>>                             uint64_t size)
>>>>> @@ -938,6 +939,7 @@ void memory_region_init_io(MemoryRegion *mr,
>>>>>      memory_region_init(mr, name, size);
>>>>>      mr->ops = ops;
>>>>>      mr->opaque = opaque;
>>>>> +    mr->base = base;
>>>>>      mr->terminates = true;
>>>>>      mr->destructor = memory_region_destructor_iomem;
>>>>>      mr->ram_addr = ~(ram_addr_t)0;
>>>>> diff --git a/memory.h b/memory.h
>>>>> index bd1bbae..2746e70 100644
>>>>> --- a/memory.h
>>>>> +++ b/memory.h
>>>>> @@ -120,6 +120,7 @@ struct MemoryRegion {
>>>>>      /* All fields are private - violators will be prosecuted */
>>>>>      const MemoryRegionOps *ops;
>>>>>      void *opaque;
>>>>> +    Object *base;
>>>>>      MemoryRegion *parent;
>>>>>      Int128 size;
>>>>>      target_phys_addr_t addr;
>>>>>
>>>>>
>>>>> Any comment?
>>>>>
>>>>
>>>> I prefer that we convert the third parameter (opaque) to be an Object.
>>>> That is a huge change, but I think it will improve the code base overall.
>>>>
>>> Object may be many different opaque, and each has different
>>> MemoryRegionOps. We need to pass in both object and opaque.
>>
>> Why?  Usually there's a 1:1 mapping between object and opaque.  Can you
>> show cases where there isn't?
>>
> As mentioned ahead, setup_cmd646_bar(PCIIDEState *d, int bus_num),
> address@hidden, but opaque are
> d->cmd646_bar[bus_num], so that is 1:n mapping. And when I browsing
> the code, this is the main issue prevent us to transfer from void* to
> Object* for memory_region_init_io()
>
>>> Maybe we can use Object's property to store the pair (mr, opaque),
>>> then we can use mr as key to get opaque in mmio-dispatch, but the
>>> property's query will hurt the performance.
>>> Or define a new struct X {Object *base, void *opaque}, and pass it to
>>> memory_region_init_io() to substitute "void *opaque" . Finally,
>>> reclaim X in memory_region_destroy().
>>
>> Usually the access callback can just cast the object to the real type.
>> That's all that's needed.
>>
> OK, I see
>>>
>>>
>>>> Other options are:
>>>>
>>>> 1) add MemoryRegionOps::ref(MemoryRegion *) and ::unref(MemoryRegion *)
>>>>
>>>> If NULL, these callbacks are ignored.  If not, they are called with the
>>>> MemoryRegion as a parameter.  Their responsibility is to derive the
>>>> Object from the MemoryRegion (through the opaque or using
>>>> container_of()) and ref or unref it respectively.
>>>>
>>>> 2) add Object *MemoryRegionOps::object(MemoryRegion *)
>>>>
>>>> Similar; if NULL it is ignored, otherwise it is used to derive the
>>>> Object, which the memory core will ref and unref.
>>>>
>>>> 3) add bool MemoryRegionOps::opaque_is_object
>>>>
>>>> Tells the memory core that it is safe to cast the opaque into an Object.
>>>>
>>> Above methods, the  process of derive the Object will be hard, we can
>>> not tell opaque is Object or not without something like try&catch
>>
>> Take for example e1000.  It passes E1000State as the opaque, which is a
>> PCIDevice, which is a DeviceState, which is an Object.  So for that
>> device, nothing needs to be done.
>>
> The same example, in setup_cmd646_bar(PCIIDEState *d, int bus_num),  I
> think we can not decide which is the type for @bar.  If using
> object_dynamic_cast(@bar, TYPE_OBJECT) to tell whether it is Object or
> not, it will raise exception.
>
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.

Regards, pingfan
>>>
>>>> 4) add memory_region_set_object(MemoryRegion *, Object *)
>>>>
>>>> Like your proposal, but avoids adding an extra paramter and changing all
>>>> call sites.
>>>>
>>> Yeah, this seems the easy one.
>>
>> Easy but wrong, IMO.
>
> Yeah, I am trying to avoid to do such things, and still try to find
> another way out.
>
> Thanx, pingfan
>>
>> --
>> error compiling committee.c: too many arguments to function



reply via email to

[Prev in Thread] Current Thread [Next in Thread]