From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57954) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RR1br-00026u-M0 for qemu-devel@nongnu.org; Thu, 17 Nov 2011 08:02:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RR1bq-0000wq-D9 for qemu-devel@nongnu.org; Thu, 17 Nov 2011 08:02:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63999) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RR1bq-0000wk-4v for qemu-devel@nongnu.org; Thu, 17 Nov 2011 08:02:30 -0500 Message-ID: <4EC505E0.9000701@redhat.com> Date: Thu, 17 Nov 2011 15:02:24 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1321532700-8929-1-git-send-email-benoit.canet@gmail.com> <1321532700-8929-5-git-send-email-benoit.canet@gmail.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/5] sh_intc: convert interrupt controller to memory API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org On 11/17/2011 02:56 PM, Peter Maydell wrote: > 2011/11/17 Beno=C3=AEt Canet : > > Signed-off-by: Benoit Canet > > --- a/hw/sh7750.c > > +++ b/hw/sh7750.c > > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, Memor= yRegion *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 =3D &desc->iomem; > > + iomem_p4 =3D desc->iomem_aliases + index; > > + iomem_a7 =3D 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 =3D 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 =3D nr_mask_regs; > > desc->prio_regs =3D prio_regs; > > desc->nr_prio_regs =3D nr_prio_regs; > > + /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases)= . */ > > + desc->iomem_aliases =3D g_new0(MemoryRegion, > > + (nr_mask_regs + nr_prio_regs) * 4); > > This should be exactly the right size... > > > + /* free unused MemoryRegions */ > > + desc->iomem_aliases =3D 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, si= nce 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 ma= kes me uncomfortable. --=20 error compiling committee.c: too many arguments to function