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

[Fabien Chouteau]

On 30/08/2011 14:20, Avi Kivity wrote:
> 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.
>

Right, I'll remove the second part and keep size handling in openpic.c as in
the current implementation.

>
>>
>> -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.

And more efficient than my dispatching, I guess.

Is it OK to use MemoryRegionOps.old_mmio in this case or should we avoid this 
deprecated
interface?

-- 
Fabien Chouteau