[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/5] sh_intc: convert interrupt controller to me
From: |
Avi Kivity |
Subject: |
Re: [Qemu-devel] [PATCH 4/5] sh_intc: convert interrupt controller to memory API |
Date: |
Thu, 17 Nov 2011 15:02:24 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1 |
On 11/17/2011 02:56 PM, Peter Maydell wrote:
> 2011/11/17 Benoît Canet <address@hidden>:
> > Signed-off-by: Benoit Canet <address@hidden>
> > --- 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
- [Qemu-devel] [PATCH 0/5] Convert remaining sh4 devices to memory API, Benoît Canet, 2011/11/17
- [Qemu-devel] [PATCH 1/5] sh7750: convert memory controller/ioport to memory API, Benoît Canet, 2011/11/17
- [Qemu-devel] [PATCH 3/5] sh_timer: convert to memory API, Benoît Canet, 2011/11/17
- [Qemu-devel] [PATCH 2/5] sh7750: convert cache and tlb to memory API, Benoît Canet, 2011/11/17
- [Qemu-devel] [PATCH 4/5] sh_intc: convert interrupt controller to memory API, Benoît Canet, 2011/11/17
- [Qemu-devel] [PATCH 5/5] sh_serial: convert to memory API, Benoît Canet, 2011/11/17