Re: [Qemu-devel] [PATCH 4/5] sh_intc: convert interrupt controller to memory API

[Avi Kivity <avi@redhat.com>]

On 11/17/2011 02:56 PM, Peter Maydell wrote:
> 2011/11/17 Benoît Canet <benoit.canet@gmail.com>:
> > Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
> > --- a/hw/sh7750.c
> > +++ b/hw/sh7750.c
> > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem)
> >                           "cache-and-tlb", 0x08000000);
> >     memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem);
> >
> > -    sh_intc_init(&s->intc, NR_SOURCES,
> > +    sh_intc_init(sysmem, &s->intc, NR_SOURCES,
> >                 _INTC_ARRAY(mask_registers),
> >                 _INTC_ARRAY(prio_registers));
>
> This would be nicer as a sysbus device so we didn't have to hand
> it the sysmem, but that can be done later if we care enough I guess.

Later, yes.

>
> > +    iomem = &desc->iomem;
> > +    iomem_p4 = desc->iomem_aliases + index;
> > +    iomem_a7 = iomem_p4 + 1;
> > +
> > +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
> > +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
> > +    memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4);
> > +    memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
> > +
> > +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
> > +    memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4);
> > +    memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
> > +#undef SH_INTC_IOMEM_FORMAT
> > +
> > +    /* used to increment aliases index */
> > +    return 2;
>
> This is going to give us 6 * 2 * 2 = 24 four-byte memory regions,
> incidentally. That should be OK, but one memory region per register
> is an interesting arrangement.

In fact if we introduce a Register class there's no reason it won't be a
MemoryRegion.  So any Register would be a MemoryRegion, we could have
thousands in a system.  I don't see anything wrong with it, do you?

>
> > @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc,
> >     desc->nr_mask_regs = nr_mask_regs;
> >     desc->prio_regs = prio_regs;
> >     desc->nr_prio_regs = nr_prio_regs;
> > +    /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */
> > +    desc->iomem_aliases = g_new0(MemoryRegion,
> > +                                 (nr_mask_regs + nr_prio_regs) * 4);
>
> This should be exactly the right size...
>
> > +    /* free unused MemoryRegions */
> > +    desc->iomem_aliases = g_realloc(desc->iomem_aliases,
> > +                                    sizeof(MemoryRegion)*j);
>
> making this realloc unnecessary; or am I missing something?
>

Not all calls to sh_intc_register() return 2.

However, calling realloc() in a MemoryRegion array is not a good idea, since the pointers may leak (memory_region_add_subregion() does this).  It's true that a size-reducing realloc doesn't change the pointer, yet it makes me uncomfortable.


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