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

[Avi Kivity]

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.

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.

4) add memory_region_set_object(MemoryRegion *, Object *)

Like your proposal, but avoids adding an extra paramter and changing all
call sites.

-- 
error compiling committee.c: too many arguments to function