From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QyNJP-0007Po-EN for qemu-devel@nongnu.org; Tue, 30 Aug 2011 08:21:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QyNJO-0000Oa-Ex for qemu-devel@nongnu.org; Tue, 30 Aug 2011 08:21:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11010) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QyNJN-0000OP-Tw for qemu-devel@nongnu.org; Tue, 30 Aug 2011 08:21:02 -0400 Message-ID: <4E5CD594.40903@redhat.com> Date: Tue, 30 Aug 2011 15:20:36 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1314634742-14219-1-git-send-email-chouteau@adacore.com> In-Reply-To: <1314634742-14219-1-git-send-email-chouteau@adacore.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Memory API conversion for mpic (openpic.c) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: qemu-devel@nongnu.org On 08/29/2011 07:19 PM, Fabien Chouteau wrote: > This patch converts mpic to the new memory API. > > -static CPUReadMemoryFunc * const mpic_int_read[] = { > -&openpic_buggy_read, > -&openpic_buggy_read, > -&mpic_src_int_read, > -}; > + switch (size) { > + case 4: > + default: > + DPRINTF("Invalid OPENPIC read access size:%d (must be 4)!\n", size); Here, you accept multiple sizes. > + } > + return retval; > +} > > -static CPUReadMemoryFunc * const mpic_msi_read[] = { > -&openpic_buggy_read, > -&openpic_buggy_read, > -&mpic_src_msi_read, > +static const MemoryRegionOps mpic_ops = { > + .read = mpic_read, > + .write = mpic_write, > + .endianness = DEVICE_BIG_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > }; Here, you reject them. One of the two is redundant. > > -qemu_irq *mpic_init (target_phys_addr_t base, int nb_cpus, > - qemu_irq **irqs, qemu_irq irq_out) > +qemu_irq *mpic_init(MemoryRegion *address_space, target_phys_addr_t base, > + int nb_cpus, qemu_irq **irqs, qemu_irq irq_out) > { > openpic_t *mpp; > int i; > - struct { > - CPUReadMemoryFunc * const *read; > - CPUWriteMemoryFunc * const *write; > - target_phys_addr_t start_addr; > - ram_addr_t size; > - } const list[] = { > - {mpic_glb_read, mpic_glb_write, MPIC_GLB_REG_START, MPIC_GLB_REG_SIZE}, > - {mpic_tmr_read, mpic_tmr_write, MPIC_TMR_REG_START, MPIC_TMR_REG_SIZE}, > - {mpic_ext_read, mpic_ext_write, MPIC_EXT_REG_START, MPIC_EXT_REG_SIZE}, > - {mpic_int_read, mpic_int_write, MPIC_INT_REG_START, MPIC_INT_REG_SIZE}, > - {mpic_msg_read, mpic_msg_write, MPIC_MSG_REG_START, MPIC_MSG_REG_SIZE}, > - {mpic_msi_read, mpic_msi_write, MPIC_MSI_REG_START, MPIC_MSI_REG_SIZE}, > - {mpic_cpu_read, mpic_cpu_write, MPIC_CPU_REG_START, MPIC_CPU_REG_SIZE}, > - }; Why aren't you doing a 1:1 conversion? (i.e. generate a MemoryRegion for every cpu_register_io_memory). I prefer those as being easier to review. However if you have the means to test and have high confidence in the patch's correctness, this way is acceptable. -- error compiling committee.c: too many arguments to function