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?
> 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.
>
>
>> 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.
>
>> 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.
--
error compiling committee.c: too many arguments to function
