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

[liu ping fan]

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


> 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

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

Thanks and regards,
pingfan
> --
> error compiling committee.c: too many arguments to function