On Thu, Aug 16, 2012 at 5:23 PM, Avi Kivity <a...@redhat.com> wrote:
> On 08/16/2012 06:22 AM, liu ping fan wrote:
>> On Tue, Aug 14, 2012 at 6:49 PM, Avi Kivity <a...@redhat.com> 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),
Object=@d, 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.
>>
>>> 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
