diff mbox

Memory API conversion for mpic (openpic.c)

Message ID 1314634742-14219-1-git-send-email-chouteau@adacore.com
State New
Headers show

Commit Message

Fabien Chouteau Aug. 29, 2011, 4:19 p.m. UTC 
This patch converts mpic to the new memory API.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 hw/openpic.c           |  208 +++++++++++++++++++++++-------------------------
 hw/openpic.h           |    8 +-
 hw/ppce500_mpc8544ds.c |    3 +-
 3 files changed, 105 insertions(+), 114 deletions(-)

Comments

Avi Kivity Aug. 30, 2011, 12:20 p.m. UTC | #1 
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.
Fabien Chouteau Aug. 30, 2011, 1:46 p.m. UTC | #2 
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?
Avi Kivity Aug. 30, 2011, 1:48 p.m. UTC | #3 
On 08/30/2011 04:46 PM, Fabien Chouteau wrote:
> Is it OK to use MemoryRegionOps.old_mmio in this case or should we avoid this deprecated
> interface?
>

old_mmio is fine, esp. for the initial conversion.
diff mbox

Patch

diff --git a/hw/openpic.c b/hw/openpic.c
index 26c96e2..72be30c 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -116,18 +116,25 @@  enum {
 
 #define MPIC_GLB_REG_START        0x0
 #define MPIC_GLB_REG_SIZE         0x10F0
+#define MPIC_GLB_REG_END          (MPIC_GLB_REG_START + MPIC_GLB_REG_SIZE - 1)
 #define MPIC_TMR_REG_START        0x10F0
 #define MPIC_TMR_REG_SIZE         0x220
+#define MPIC_TMR_REG_END          (MPIC_TMR_REG_START + MPIC_TMR_REG_SIZE - 1)
 #define MPIC_EXT_REG_START        0x10000
 #define MPIC_EXT_REG_SIZE         0x180
+#define MPIC_EXT_REG_END          (MPIC_EXT_REG_START + MPIC_EXT_REG_SIZE - 1)
 #define MPIC_INT_REG_START        0x10200
 #define MPIC_INT_REG_SIZE         0x800
+#define MPIC_INT_REG_END          (MPIC_INT_REG_START + MPIC_INT_REG_SIZE - 1)
 #define MPIC_MSG_REG_START        0x11600
 #define MPIC_MSG_REG_SIZE         0x100
+#define MPIC_MSG_REG_END          (MPIC_MSG_REG_START + MPIC_MSG_REG_SIZE - 1)
 #define MPIC_MSI_REG_START        0x11C00
 #define MPIC_MSI_REG_SIZE         0x100
+#define MPIC_MSI_REG_END          (MPIC_MSI_REG_START + MPIC_MSI_REG_SIZE - 1)
 #define MPIC_CPU_REG_START        0x20000
 #define MPIC_CPU_REG_SIZE         0x100
+#define MPIC_CPU_REG_END          (MPIC_CPU_REG_START + MPIC_CPU_REG_SIZE - 1)
 
 enum mpic_ide_bits {
     IDR_EP     = 0,
@@ -1148,8 +1155,8 @@  static void openpic_irq_raise(openpic_t *opp, int n_CPU, IRQ_src_t *src)
     qemu_irq_raise(opp->dst[n_CPU].irqs[OPENPIC_OUTPUT_INT]);
 }
 
-qemu_irq *openpic_init (PCIBus *bus, MemoryRegion **pmem, int nb_cpus,
-                        qemu_irq **irqs, qemu_irq irq_out)
+qemu_irq *openpic_init(PCIBus *bus, MemoryRegion **pmem, int nb_cpus,
+                       qemu_irq **irqs, qemu_irq irq_out)
 {
     openpic_t *opp;
     uint8_t *pci_conf;
@@ -1537,108 +1544,103 @@  static uint32_t mpic_src_msi_read (void *opaque, target_phys_addr_t addr)
     return retval;
 }
 
-static CPUWriteMemoryFunc * const mpic_glb_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &openpic_gbl_write,
-};
-
-static CPUReadMemoryFunc * const mpic_glb_read[] = {
-    &openpic_buggy_read,
-    &openpic_buggy_read,
-    &openpic_gbl_read,
-};
-
-static CPUWriteMemoryFunc * const mpic_tmr_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &mpic_timer_write,
-};
-
-static CPUReadMemoryFunc * const mpic_tmr_read[] = {
-    &openpic_buggy_read,
-    &openpic_buggy_read,
-    &mpic_timer_read,
-};
-
-static CPUWriteMemoryFunc * const mpic_cpu_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &openpic_cpu_write,
-};
-
-static CPUReadMemoryFunc * const mpic_cpu_read[] = {
-    &openpic_buggy_read,
-    &openpic_buggy_read,
-    &openpic_cpu_read,
-};
-
-static CPUWriteMemoryFunc * const mpic_ext_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &mpic_src_ext_write,
-};
-
-static CPUReadMemoryFunc * const mpic_ext_read[] = {
-    &openpic_buggy_read,
-    &openpic_buggy_read,
-    &mpic_src_ext_read,
-};
-
-static CPUWriteMemoryFunc * const mpic_int_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &mpic_src_int_write,
-};
+static uint64_t mpic_read(void *opaque, target_phys_addr_t addr,
+                          unsigned size)
+{
+    uint64_t retval = (uint64_t)-1;
 
-static CPUReadMemoryFunc * const mpic_int_read[] = {
-    &openpic_buggy_read,
-    &openpic_buggy_read,
-    &mpic_src_int_read,
-};
+    switch (size) {
+    case 4:
+        addr &= 0x3FFFF;
+        DPRINTF("%s: offset %08x\n", __func__, (int)addr);
+        switch (addr) {
+        case MPIC_GLB_REG_START ... MPIC_GLB_REG_END:
+            retval = openpic_gbl_read(opaque, addr & 0xffff);
+            break;
+        case MPIC_TMR_REG_START ... MPIC_TMR_REG_END:
+            retval = mpic_timer_read(opaque, addr & 0xfff);
+            break;
+        case MPIC_EXT_REG_START ... MPIC_EXT_REG_END:
+            retval = mpic_src_ext_read(opaque, addr & 0xfff);
+            break;
+        case MPIC_INT_REG_START ... MPIC_INT_REG_END:
+            retval = mpic_src_int_read(opaque, addr & 0xfff);
+            break;
+        case MPIC_MSG_REG_START ... MPIC_MSG_REG_END:
+            retval = mpic_src_msg_read(opaque, addr & 0xfff);
+            break;
+        case MPIC_MSI_REG_START ... MPIC_MSI_REG_END:
+            retval = mpic_src_msi_read(opaque, addr & 0xfff);
+            break;
+        case MPIC_CPU_REG_START ... MPIC_CPU_REG_END:
+            retval = openpic_cpu_read(opaque, addr & 0xfff);
+            break;
+        default:
+            DPRINTF("Invalid OPENPIC read access !\n");
+        }
+        break;
+    default:
+        DPRINTF("Invalid OPENPIC read access size:%d (must be 4)!\n", size);
+    }
+    return retval;
+}
 
-static CPUWriteMemoryFunc * const mpic_msg_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &mpic_src_msg_write,
-};
+static void mpic_write(void *opaque, target_phys_addr_t addr,
+                       uint64_t data, unsigned size)
+{
+    switch (size) {
+    case 4:
+        addr &= 0x3FFFF;
+        DPRINTF("%s: offset %08x val: %08x\n", __func__,
+                (int)addr, (unsigned int)data);
+        switch (addr) {
+        case MPIC_GLB_REG_START ... MPIC_GLB_REG_END:
+            openpic_gbl_write(opaque, addr & 0xffff, data);
+            break;
+        case MPIC_TMR_REG_START ... MPIC_TMR_REG_END:
+            mpic_timer_write(opaque, addr & 0xfff, data);
+            break;
+        case MPIC_EXT_REG_START ... MPIC_EXT_REG_END:
+            mpic_src_ext_write(opaque, addr & 0xfff, data);
+            break;
+        case MPIC_INT_REG_START ... MPIC_INT_REG_END:
+            mpic_src_int_write(opaque, addr & 0xfff, data);
+            break;
+        case MPIC_MSG_REG_START ... MPIC_MSG_REG_END:
+            mpic_src_msg_write(opaque, addr & 0xfff, data);
+            break;
+        case MPIC_MSI_REG_START ... MPIC_MSI_REG_END:
+            mpic_src_msi_write(opaque, addr & 0xfff, data);
+            break;
+        case MPIC_CPU_REG_START ... MPIC_CPU_REG_END:
+            openpic_cpu_write(opaque, addr & 0xfff, data);
+            break;
+        default:
+            DPRINTF("Invalid OPENPIC write access !\n");
+        }
+        break;
 
-static CPUReadMemoryFunc * const mpic_msg_read[] = {
-    &openpic_buggy_read,
-    &openpic_buggy_read,
-    &mpic_src_msg_read,
-};
-static CPUWriteMemoryFunc * const mpic_msi_write[] = {
-    &openpic_buggy_write,
-    &openpic_buggy_write,
-    &mpic_src_msi_write,
-};
+    default:
+        DPRINTF("Invalid OPENPIC write access size:%d (must be 4)!\n", size);
+        return;
+    }
+}
 
-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,
+    },
 };
 
-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},
-    };
 
     /* XXX: for now, only one CPU is supported */
     if (nb_cpus != 1)
@@ -1646,17 +1648,9 @@  qemu_irq *mpic_init (target_phys_addr_t base, int nb_cpus,
 
     mpp = g_malloc0(sizeof(openpic_t));
 
-    for (i = 0; i < sizeof(list)/sizeof(list[0]); i++) {
-        int mem_index;
+    memory_region_init_io(&mpp->mem, &mpic_ops, mpp, "mpic", 0x40000);
 
-        mem_index = cpu_register_io_memory(list[i].read, list[i].write, mpp,
-                                           DEVICE_BIG_ENDIAN);
-        if (mem_index < 0) {
-            goto free;
-        }
-        cpu_register_physical_memory(base + list[i].start_addr,
-                                     list[i].size, mem_index);
-    }
+    memory_region_add_subregion(address_space, base, &mpp->mem);
 
     mpp->nb_cpus = nb_cpus;
     mpp->max_irq = MPIC_MAX_IRQ;
@@ -1674,8 +1668,4 @@  qemu_irq *mpic_init (target_phys_addr_t base, int nb_cpus,
     qemu_register_reset(mpic_reset, mpp);
 
     return qemu_allocate_irqs(openpic_set_irq, mpp, mpp->max_irq);
-
-free:
-    g_free(mpp);
-    return NULL;
 }
diff --git a/hw/openpic.h b/hw/openpic.h
index 75de361..a9352c8 100644
--- a/hw/openpic.h
+++ b/hw/openpic.h
@@ -11,8 +11,8 @@  enum {
     OPENPIC_OUTPUT_NB,
 };
 
-qemu_irq *openpic_init (PCIBus *bus, MemoryRegion **pmem, int nb_cpus,
-                        qemu_irq **irqs, qemu_irq irq_out);
-qemu_irq *mpic_init (target_phys_addr_t base, int nb_cpus,
-                        qemu_irq **irqs, qemu_irq irq_out);
+qemu_irq *openpic_init(PCIBus *bus, MemoryRegion **pmem, 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);
 #endif /* __OPENPIC_H__ */
diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index 274b37c..7884627 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -272,7 +272,8 @@  static void mpc8544ds_init(ram_addr_t ram_size,
     irqs = g_malloc0(sizeof(qemu_irq) * OPENPIC_OUTPUT_NB);
     irqs[OPENPIC_OUTPUT_INT] = ((qemu_irq *)env->irq_inputs)[PPCE500_INPUT_INT];
     irqs[OPENPIC_OUTPUT_CINT] = ((qemu_irq *)env->irq_inputs)[PPCE500_INPUT_CINT];
-    mpic = mpic_init(MPC8544_MPIC_REGS_BASE, 1, &irqs, NULL);
+    mpic = mpic_init(get_system_memory(), MPC8544_MPIC_REGS_BASE,
+                     1, &irqs, NULL);
 
     /* Serial */
     if (serial_hds[0]) {