Re: [Qemu-devel] [PATCH] Memory API conversion for mpic (openpic.c)

[Avi Kivity <avi@redhat.com>]

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