Patchwork Memory API conversion for mpic (openpic.c)

login
register
mail settings
Submitter Fabien Chouteau
Date Aug. 29, 2011, 4:19 p.m.
Message ID <1314634742-14219-1-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/112109/
State New
Headers show

Comments

Fabien Chouteau - Aug. 29, 2011, 4:19 p.m.
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(-)
Avi Kivity - Aug. 30, 2011, 12:20 p.m.
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.
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.
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.

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]) {