Patchwork [4/5] sh_intc: convert interrupt controller to memory API

login
register
mail settings
Submitter Benoit Canet
Date Nov. 17, 2011, 12:24 p.m.
Message ID <1321532700-8929-5-git-send-email-benoit.canet@gmail.com>
Download mbox | patch
Permalink /patch/126198/
State New
Headers show

Comments

Benoit Canet - Nov. 17, 2011, 12:24 p.m.
Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 hw/sh7750.c         |    2 +-
 hw/sh_intc.c        |   87 ++++++++++++++++++++++++++++++++++----------------
 hw/sh_intc.h        |    7 +++-
 target-sh4/helper.c |    3 ++
 4 files changed, 68 insertions(+), 31 deletions(-)
Peter Maydell - Nov. 17, 2011, 12:56 p.m.
2011/11/17 Benoît Canet <benoit.canet@gmail.com>:
> Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
> --- a/hw/sh7750.c
> +++ b/hw/sh7750.c
> @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem)
>                           "cache-and-tlb", 0x08000000);
>     memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem);
>
> -    sh_intc_init(&s->intc, NR_SOURCES,
> +    sh_intc_init(sysmem, &s->intc, NR_SOURCES,
>                 _INTC_ARRAY(mask_registers),
>                 _INTC_ARRAY(prio_registers));

This would be nicer as a sysbus device so we didn't have to hand
it the sysmem, but that can be done later if we care enough I guess.

> +    iomem = &desc->iomem;
> +    iomem_p4 = desc->iomem_aliases + index;
> +    iomem_a7 = iomem_p4 + 1;
> +
> +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
> +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
> +    memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4);
> +    memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
> +
> +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
> +    memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4);
> +    memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
> +#undef SH_INTC_IOMEM_FORMAT
> +
> +    /* used to increment aliases index */
> +    return 2;

This is going to give us 6 * 2 * 2 = 24 four-byte memory regions,
incidentally. That should be OK, but one memory region per register
is an interesting arrangement.

> @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc,
>     desc->nr_mask_regs = nr_mask_regs;
>     desc->prio_regs = prio_regs;
>     desc->nr_prio_regs = nr_prio_regs;
> +    /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */
> +    desc->iomem_aliases = g_new0(MemoryRegion,
> +                                 (nr_mask_regs + nr_prio_regs) * 4);

This should be exactly the right size...

> +    /* free unused MemoryRegions */
> +    desc->iomem_aliases = g_realloc(desc->iomem_aliases,
> +                                    sizeof(MemoryRegion)*j);

making this realloc unnecessary; or am I missing something?

-- PMM
Avi Kivity - Nov. 17, 2011, 1:02 p.m.
On 11/17/2011 02:56 PM, Peter Maydell wrote:
> 2011/11/17 Benoît Canet <benoit.canet@gmail.com>:
> > Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
> > --- a/hw/sh7750.c
> > +++ b/hw/sh7750.c
> > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem)
> >                           "cache-and-tlb", 0x08000000);
> >     memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem);
> >
> > -    sh_intc_init(&s->intc, NR_SOURCES,
> > +    sh_intc_init(sysmem, &s->intc, NR_SOURCES,
> >                 _INTC_ARRAY(mask_registers),
> >                 _INTC_ARRAY(prio_registers));
>
> This would be nicer as a sysbus device so we didn't have to hand
> it the sysmem, but that can be done later if we care enough I guess.

Later, yes.

>
> > +    iomem = &desc->iomem;
> > +    iomem_p4 = desc->iomem_aliases + index;
> > +    iomem_a7 = iomem_p4 + 1;
> > +
> > +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
> > +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
> > +    memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4);
> > +    memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
> > +
> > +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
> > +    memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4);
> > +    memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
> > +#undef SH_INTC_IOMEM_FORMAT
> > +
> > +    /* used to increment aliases index */
> > +    return 2;
>
> This is going to give us 6 * 2 * 2 = 24 four-byte memory regions,
> incidentally. That should be OK, but one memory region per register
> is an interesting arrangement.

In fact if we introduce a Register class there's no reason it won't be a
MemoryRegion.  So any Register would be a MemoryRegion, we could have
thousands in a system.  I don't see anything wrong with it, do you?

>
> > @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc,
> >     desc->nr_mask_regs = nr_mask_regs;
> >     desc->prio_regs = prio_regs;
> >     desc->nr_prio_regs = nr_prio_regs;
> > +    /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */
> > +    desc->iomem_aliases = g_new0(MemoryRegion,
> > +                                 (nr_mask_regs + nr_prio_regs) * 4);
>
> This should be exactly the right size...
>
> > +    /* free unused MemoryRegions */
> > +    desc->iomem_aliases = g_realloc(desc->iomem_aliases,
> > +                                    sizeof(MemoryRegion)*j);
>
> making this realloc unnecessary; or am I missing something?
>

Not all calls to sh_intc_register() return 2.

However, calling realloc() in a MemoryRegion array is not a good idea, since the pointers may leak (memory_region_add_subregion() does this).  It's true that a size-reducing realloc doesn't change the pointer, yet it makes me uncomfortable.
Benoit Canet - Nov. 17, 2011, 1:11 p.m.
Yes, allocating the exact size would make the realloc unnecessary. But it
involve more code to walk one more time through the mask_reg and prio_reg
array before allocating.
I made it this way to make code shorter.

The freed MemoryRegion are not initialized at all.

However as realloc seems to be a bad idea I need something to compute the
exact size in a minimum of code.

2011/11/17 Peter Maydell <peter.maydell@linaro.org>

> 2011/11/17 Benoît Canet <benoit.canet@gmail.com>:
> > Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
> > --- a/hw/sh7750.c
> > +++ b/hw/sh7750.c
> > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu,
> MemoryRegion *sysmem)
> >                           "cache-and-tlb", 0x08000000);
> >     memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem);
> >
> > -    sh_intc_init(&s->intc, NR_SOURCES,
> > +    sh_intc_init(sysmem, &s->intc, NR_SOURCES,
> >                 _INTC_ARRAY(mask_registers),
> >                 _INTC_ARRAY(prio_registers));
>
> This would be nicer as a sysbus device so we didn't have to hand
> it the sysmem, but that can be done later if we care enough I guess.
>
> > +    iomem = &desc->iomem;
> > +    iomem_p4 = desc->iomem_aliases + index;
> > +    iomem_a7 = iomem_p4 + 1;
> > +
> > +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
> > +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action,
> "p4");
> > +    memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address),
> 4);
> > +    memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
> > +
> > +    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action,
> "a7");
> > +    memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address),
> 4);
> > +    memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
> > +#undef SH_INTC_IOMEM_FORMAT
> > +
> > +    /* used to increment aliases index */
> > +    return 2;
>
> This is going to give us 6 * 2 * 2 = 24 four-byte memory regions,
> incidentally. That should be OK, but one memory region per register
> is an interesting arrangement.
>
> > @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc,
> >     desc->nr_mask_regs = nr_mask_regs;
> >     desc->prio_regs = prio_regs;
> >     desc->nr_prio_regs = nr_prio_regs;
> > +    /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */
> > +    desc->iomem_aliases = g_new0(MemoryRegion,
> > +                                 (nr_mask_regs + nr_prio_regs) * 4);
>
> This should be exactly the right size...
>
> > +    /* free unused MemoryRegions */
> > +    desc->iomem_aliases = g_realloc(desc->iomem_aliases,
> > +                                    sizeof(MemoryRegion)*j);
>
> making this realloc unnecessary; or am I missing something?
>
> -- PMM
>

Patch

diff --git a/hw/sh7750.c b/hw/sh7750.c
index e181305..930d212 100644
--- a/hw/sh7750.c
+++ b/hw/sh7750.c
@@ -756,7 +756,7 @@  SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem)
                           "cache-and-tlb", 0x08000000);
     memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem);
 
-    sh_intc_init(&s->intc, NR_SOURCES,
+    sh_intc_init(sysmem, &s->intc, NR_SOURCES,
 		 _INTC_ARRAY(mask_registers),
 		 _INTC_ARRAY(prio_registers));
 
diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index e07424f..38cefc9 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -219,7 +219,8 @@  static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
 #endif
 }
 
-static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset)
+static uint64_t sh_intc_read(void *opaque, target_phys_addr_t offset,
+                             unsigned size)
 {
     struct intc_desc *desc = opaque;
     intc_enum *enum_ids = NULL;
@@ -238,7 +239,7 @@  static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset)
 }
 
 static void sh_intc_write(void *opaque, target_phys_addr_t offset,
-			  uint32_t value)
+                          uint64_t value, unsigned size)
 {
     struct intc_desc *desc = opaque;
     intc_enum *enum_ids = NULL;
@@ -282,16 +283,10 @@  static void sh_intc_write(void *opaque, target_phys_addr_t offset,
 #endif
 }
 
-static CPUReadMemoryFunc * const sh_intc_readfn[] = {
-    sh_intc_read,
-    sh_intc_read,
-    sh_intc_read
-};
-
-static CPUWriteMemoryFunc * const sh_intc_writefn[] = {
-    sh_intc_write,
-    sh_intc_write,
-    sh_intc_write
+static const struct MemoryRegionOps sh_intc_ops = {
+    .read = sh_intc_read,
+    .write = sh_intc_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id)
@@ -302,15 +297,36 @@  struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id)
     return NULL;
 }
 
-static void sh_intc_register(struct intc_desc *desc, 
-			     unsigned long address)
+static unsigned int sh_intc_register(MemoryRegion *sysmem,
+                             struct intc_desc *desc,
+                             const unsigned long address,
+                             const char *type,
+                             const char *action,
+                             const unsigned int index)
 {
-    if (address) {
-        cpu_register_physical_memory_offset(P4ADDR(address), 4,
-                                            desc->iomemtype, INTC_A7(address));
-        cpu_register_physical_memory_offset(A7ADDR(address), 4,
-                                            desc->iomemtype, INTC_A7(address));
+    char name[60];
+    MemoryRegion *iomem, *iomem_p4, *iomem_a7;
+
+    if (!address) {
+        return 0;
     }
+
+    iomem = &desc->iomem;
+    iomem_p4 = desc->iomem_aliases + index;
+    iomem_a7 = iomem_p4 + 1;
+
+#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
+    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
+    memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4);
+    memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
+
+    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
+    memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4);
+    memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
+#undef SH_INTC_IOMEM_FORMAT
+
+    /* used to increment aliases index */
+    return 2;
 }
 
 static void sh_intc_register_source(struct intc_desc *desc,
@@ -415,14 +431,15 @@  void sh_intc_register_sources(struct intc_desc *desc,
     }
 }
 
-int sh_intc_init(struct intc_desc *desc,
+int sh_intc_init(MemoryRegion *sysmem,
+         struct intc_desc *desc,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
 		 struct intc_prio_reg *prio_regs,
 		 int nr_prio_regs)
 {
-    unsigned int i;
+    unsigned int i, j;
 
     desc->pending = 0;
     desc->nr_sources = nr_sources;
@@ -430,7 +447,11 @@  int sh_intc_init(struct intc_desc *desc,
     desc->nr_mask_regs = nr_mask_regs;
     desc->prio_regs = prio_regs;
     desc->nr_prio_regs = nr_prio_regs;
+    /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */
+    desc->iomem_aliases = g_new0(MemoryRegion,
+                                 (nr_mask_regs + nr_prio_regs) * 4);
 
+    j = 0;
     i = sizeof(struct intc_source) * nr_sources;
     desc->sources = g_malloc0(i);
 
@@ -442,15 +463,19 @@  int sh_intc_init(struct intc_desc *desc,
 
     desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
  
-    desc->iomemtype = cpu_register_io_memory(sh_intc_readfn,
-					     sh_intc_writefn, desc,
-                                             DEVICE_NATIVE_ENDIAN);
+    memory_region_init_io(&desc->iomem, &sh_intc_ops, desc,
+                          "interrupt-controller", 0x100000000ULL);
+
+#define INT_REG_PARAMS(reg_struct, type, action, j) \
+        reg_struct->action##_reg, #type, #action, j
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
 	    struct intc_mask_reg *mr = desc->mask_regs + i;
 
-	    sh_intc_register(desc, mr->set_reg);
-	    sh_intc_register(desc, mr->clr_reg);
+        j += sh_intc_register(sysmem, desc,
+                              INT_REG_PARAMS(mr, mask, set, j));
+        j += sh_intc_register(sysmem, desc,
+                              INT_REG_PARAMS(mr, mask, clr, j));
 	}
     }
 
@@ -458,10 +483,16 @@  int sh_intc_init(struct intc_desc *desc,
         for (i = 0; i < desc->nr_prio_regs; i++) {
 	    struct intc_prio_reg *pr = desc->prio_regs + i;
 
-	    sh_intc_register(desc, pr->set_reg);
-	    sh_intc_register(desc, pr->clr_reg);
+        j += sh_intc_register(sysmem, desc,
+                              INT_REG_PARAMS(pr, prio, set, j));
+        j += sh_intc_register(sysmem, desc,
+                              INT_REG_PARAMS(pr, prio, clr, j));
 	}
     }
+#undef INT_REG_PARAMS
+    /* free unused MemoryRegions */
+    desc->iomem_aliases = g_realloc(desc->iomem_aliases,
+                                    sizeof(MemoryRegion)*j);
 
     return 0;
 }
diff --git a/hw/sh_intc.h b/hw/sh_intc.h
index c117d6f..8916e8c 100644
--- a/hw/sh_intc.h
+++ b/hw/sh_intc.h
@@ -3,6 +3,7 @@ 
 
 #include "qemu-common.h"
 #include "irq.h"
+#include "exec-memory.h"
 
 typedef unsigned char intc_enum;
 
@@ -46,6 +47,8 @@  struct intc_source {
 };
 
 struct intc_desc {
+    MemoryRegion iomem;
+    MemoryRegion *iomem_aliases;
     qemu_irq *irqs;
     struct intc_source *sources;
     int nr_sources;
@@ -53,7 +56,6 @@  struct intc_desc {
     int nr_mask_regs;
     struct intc_prio_reg *prio_regs;
     int nr_prio_regs;
-    int iomemtype;
     int pending; /* number of interrupt sources that has pending set */
 };
 
@@ -68,7 +70,8 @@  void sh_intc_register_sources(struct intc_desc *desc,
 			      struct intc_group *groups,
 			      int nr_groups);
 
-int sh_intc_init(struct intc_desc *desc,
+int sh_intc_init(MemoryRegion *sysmem,
+         struct intc_desc *desc,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 5a1e15e..f4dda48 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -24,7 +24,10 @@ 
 #include <signal.h>
 
 #include "cpu.h"
+
+#if !defined(CONFIG_USER_ONLY)
 #include "hw/sh_intc.h"
+#endif
 
 #if defined(CONFIG_USER_ONLY)