diff mbox

[v1.1,09/24] omap_gpmc/nseries/tusb6010: convert to memory API

Message ID 1312880570-12530-1-git-send-email-avi@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Avi Kivity Aug. 9, 2011, 9:02 a.m. UTC
Somewhat clumsy since it needs a variable sized region.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

v1.1: set access size validity for omap gpmc region to allow only 32-bit
      accesses

 hw/omap.h      |    3 ++-
 hw/omap_gpmc.c |   56 ++++++++++++++++++++++++++++++++------------------------
 hw/tusb6010.c  |   30 +++++++++++++-----------------
 hw/tusb6010.h  |    7 +++++--
 4 files changed, 52 insertions(+), 44 deletions(-)

Comments

Peter Maydell Aug. 9, 2011, 9:23 a.m. UTC | #1
On 9 August 2011 10:02, Avi Kivity <avi@redhat.com> wrote:
> +static const MemoryRegionOps omap_gpmc_ops = {
> +    .read = omap_gpmc_read,
> +    .write = omap_gpmc_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
>  };

Does this give debug trace output for missized accesses?
That's the main point of the badread/badwrite functions...

-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity Aug. 9, 2011, 9:26 a.m. UTC | #2
On 08/09/2011 12:23 PM, Peter Maydell wrote:
> On 9 August 2011 10:02, Avi Kivity<avi@redhat.com>  wrote:
> >  +static const MemoryRegionOps omap_gpmc_ops = {
> >  +    .read = omap_gpmc_read,
> >  +    .write = omap_gpmc_write,
> >  +    .endianness = DEVICE_NATIVE_ENDIAN,
> >  +    .valid = {
> >  +        .min_access_size = 4,
> >  +        .max_access_size = 4,
> >  +    },
> >    };
>
> Does this give debug trace output for missized accesses?

Only after we implement and configure a debug policy in memory.c

> That's the main point of the badread/badwrite functions...

These are broken now, aren't they?
Peter Maydell Aug. 9, 2011, 9:41 a.m. UTC | #3
On 9 August 2011 10:26, Avi Kivity <avi@redhat.com> wrote:
> On 08/09/2011 12:23 PM, Peter Maydell wrote:
>>
>> On 9 August 2011 10:02, Avi Kivity<avi@redhat.com>  wrote:
>> >  +static const MemoryRegionOps omap_gpmc_ops = {
>> >  +    .read = omap_gpmc_read,
>> >  +    .write = omap_gpmc_write,
>> >  +    .endianness = DEVICE_NATIVE_ENDIAN,
>> >  +    .valid = {
>> >  +        .min_access_size = 4,
>> >  +        .max_access_size = 4,
>> >  +    },
>> >    };
>>
>> Does this give debug trace output for missized accesses?
>
> Only after we implement and configure a debug policy in memory.c
>
>> That's the main point of the badread/badwrite functions...
>
> These are broken now, aren't they?

They successfully print a message, which as I say is the
main point. The OMAP TRM typically says the behaviour isn't
defined for wrong-size accesses, so guest code doing it is
broken anyhow and so the error in the current implementation
is IMHO not so serious.

I think if (as here) it turns out that we need more functionality
in memory.c then we should be implementing it, not silently
dropping things in the conversion process.

-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity Aug. 9, 2011, 12:56 p.m. UTC | #4
On 08/09/2011 12:41 PM, Peter Maydell wrote:
> On 9 August 2011 10:26, Avi Kivity<avi@redhat.com>  wrote:
> >  On 08/09/2011 12:23 PM, Peter Maydell wrote:
> >>
> >>  On 9 August 2011 10:02, Avi Kivity<avi@redhat.com>    wrote:
> >>  >    +static const MemoryRegionOps omap_gpmc_ops = {
> >>  >    +    .read = omap_gpmc_read,
> >>  >    +    .write = omap_gpmc_write,
> >>  >    +    .endianness = DEVICE_NATIVE_ENDIAN,
> >>  >    +    .valid = {
> >>  >    +        .min_access_size = 4,
> >>  >    +        .max_access_size = 4,
> >>  >    +    },
> >>  >      };
> >>
> >>  Does this give debug trace output for missized accesses?
> >
> >  Only after we implement and configure a debug policy in memory.c
> >
> >>  That's the main point of the badread/badwrite functions...
> >
> >  These are broken now, aren't they?
>
> They successfully print a message, which as I say is the
> main point. The OMAP TRM typically says the behaviour isn't
> defined for wrong-size accesses, so guest code doing it is
> broken anyhow and so the error in the current implementation
> is IMHO not so serious.
>
> I think if (as here) it turns out that we need more functionality
> in memory.c then we should be implementing it, not silently
> dropping things in the conversion process.
>

Okay.  I'll update the patch to forward to the badread/badwrite 
functions on bad size.
diff mbox

Patch

diff --git a/hw/omap.h b/hw/omap.h
index a064353..c2fe54c 100644
--- a/hw/omap.h
+++ b/hw/omap.h
@@ -17,6 +17,7 @@ 
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #ifndef hw_omap_h
+#include "memory.h"
 # define hw_omap_h		"omap.h"
 
 # define OMAP_EMIFS_BASE	0x00000000
@@ -119,7 +120,7 @@  void omap_sdrc_reset(struct omap_sdrc_s *s);
 struct omap_gpmc_s;
 struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq);
 void omap_gpmc_reset(struct omap_gpmc_s *s);
-void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype,
+void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem,
                 void (*base_upd)(void *opaque, target_phys_addr_t new),
                 void (*unmap)(void *opaque), void *opaque);
 
diff --git a/hw/omap_gpmc.c b/hw/omap_gpmc.c
index 8bf3343..19e0865 100644
--- a/hw/omap_gpmc.c
+++ b/hw/omap_gpmc.c
@@ -21,10 +21,13 @@ 
 #include "hw.h"
 #include "flash.h"
 #include "omap.h"
+#include "memory.h"
+#include "exec-memory.h"
 
 /* General-Purpose Memory Controller */
 struct omap_gpmc_s {
     qemu_irq irq;
+    MemoryRegion iomem;
 
     uint8_t sysconfig;
     uint16_t irqst;
@@ -39,7 +42,8 @@  struct omap_gpmc_s {
         uint32_t config[7];
         target_phys_addr_t base;
         size_t size;
-        int iomemtype;
+        MemoryRegion *iomem;
+        MemoryRegion container;
         void (*base_update)(void *opaque, target_phys_addr_t new);
         void (*unmap)(void *opaque);
         void *opaque;
@@ -75,8 +79,12 @@  static void omap_gpmc_cs_map(struct omap_gpmc_cs_file_s *f, int base, int mask)
      * constant), the mask should cause wrapping of the address space, so
      * that the same memory becomes accessible at every <i>size</i> bytes
      * starting from <i>base</i>.  */
-    if (f->iomemtype)
-        cpu_register_physical_memory(f->base, f->size, f->iomemtype);
+    if (f->iomem) {
+        memory_region_init(&f->container, "omap-gpmc-file", f->size);
+        memory_region_add_subregion(&f->container, 0, f->iomem);
+        memory_region_add_subregion(get_system_memory(), f->base,
+                                    &f->container);
+    }
 
     if (f->base_update)
         f->base_update(f->opaque, f->base);
@@ -87,8 +95,11 @@  static void omap_gpmc_cs_unmap(struct omap_gpmc_cs_file_s *f)
     if (f->size) {
         if (f->unmap)
             f->unmap(f->opaque);
-        if (f->iomemtype)
-            cpu_register_physical_memory(f->base, f->size, IO_MEM_UNASSIGNED);
+        if (f->iomem) {
+            memory_region_del_subregion(get_system_memory(), &f->container);
+            memory_region_del_subregion(&f->container, f->iomem);
+            memory_region_destroy(&f->container);
+        }
         f->base = 0;
         f->size = 0;
     }
@@ -132,7 +143,8 @@  void omap_gpmc_reset(struct omap_gpmc_s *s)
         ecc_reset(&s->ecc[i]);
 }
 
-static uint32_t omap_gpmc_read(void *opaque, target_phys_addr_t addr)
+static uint64_t omap_gpmc_read(void *opaque, target_phys_addr_t addr,
+                               unsigned size)
 {
     struct omap_gpmc_s *s = (struct omap_gpmc_s *) opaque;
     int cs;
@@ -230,7 +242,7 @@  static uint32_t omap_gpmc_read(void *opaque, target_phys_addr_t addr)
 }
 
 static void omap_gpmc_write(void *opaque, target_phys_addr_t addr,
-                uint32_t value)
+                            uint64_t value, unsigned size)
 {
     struct omap_gpmc_s *s = (struct omap_gpmc_s *) opaque;
     int cs;
@@ -249,7 +261,7 @@  static void omap_gpmc_write(void *opaque, target_phys_addr_t addr,
 
     case 0x010:	/* GPMC_SYSCONFIG */
         if ((value >> 3) == 0x3)
-            fprintf(stderr, "%s: bad SDRAM idle mode %i\n",
+            fprintf(stderr, "%s: bad SDRAM idle mode %"PRIi64"\n",
                             __FUNCTION__, value >> 3);
         if (value & 2)
             omap_gpmc_reset(s);
@@ -369,34 +381,30 @@  static void omap_gpmc_write(void *opaque, target_phys_addr_t addr,
     }
 }
 
-static CPUReadMemoryFunc * const omap_gpmc_readfn[] = {
-    omap_badwidth_read32,	/* TODO */
-    omap_badwidth_read32,	/* TODO */
-    omap_gpmc_read,
-};
-
-static CPUWriteMemoryFunc * const omap_gpmc_writefn[] = {
-    omap_badwidth_write32,	/* TODO */
-    omap_badwidth_write32,	/* TODO */
-    omap_gpmc_write,
+static const MemoryRegionOps omap_gpmc_ops = {
+    .read = omap_gpmc_read,
+    .write = omap_gpmc_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
 };
 
 struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq)
 {
-    int iomemtype;
     struct omap_gpmc_s *s = (struct omap_gpmc_s *)
             qemu_mallocz(sizeof(struct omap_gpmc_s));
 
     omap_gpmc_reset(s);
 
-    iomemtype = cpu_register_io_memory(omap_gpmc_readfn,
-                    omap_gpmc_writefn, s, DEVICE_NATIVE_ENDIAN);
-    cpu_register_physical_memory(base, 0x1000, iomemtype);
+    memory_region_init_io(&s->iomem, &omap_gpmc_ops, s, "omap-gpmc", 0x1000);
+    memory_region_add_subregion(get_system_memory(), base, &s->iomem);
 
     return s;
 }
 
-void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype,
+void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem,
                 void (*base_upd)(void *opaque, target_phys_addr_t new),
                 void (*unmap)(void *opaque), void *opaque)
 {
@@ -408,7 +416,7 @@  void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype,
     }
     f = &s->cs_file[cs];
 
-    f->iomemtype = iomemtype;
+    f->iomem = iomem;
     f->base_update = base_upd;
     f->unmap = unmap;
     f->opaque = opaque;
diff --git a/hw/tusb6010.c b/hw/tusb6010.c
index add748c..28ff52c 100644
--- a/hw/tusb6010.c
+++ b/hw/tusb6010.c
@@ -26,7 +26,7 @@ 
 #include "tusb6010.h"
 
 struct TUSBState {
-    int iomemtype[2];
+    MemoryRegion iomem[2];
     qemu_irq irq;
     MUSBState *musb;
     QEMUTimer *otg_timer;
@@ -234,14 +234,14 @@  struct TUSBState {
 #define TUSB_EP_CONFIG_XFR_SIZE(v)	((v) & 0x7fffffff)
 #define TUSB_PROD_TEST_RESET_VAL	0xa596
 
-int tusb6010_sync_io(TUSBState *s)
+MemoryRegion *tusb6010_sync_io(TUSBState *s)
 {
-    return s->iomemtype[0];
+    return &s->iomem[0];
 }
 
-int tusb6010_async_io(TUSBState *s)
+MemoryRegion *tusb6010_async_io(TUSBState *s)
 {
-    return s->iomemtype[1];
+    return &s->iomem[1];
 }
 
 static void tusb_intr_update(TUSBState *s)
@@ -647,16 +647,12 @@  static void tusb_async_writew(void *opaque, target_phys_addr_t addr,
     }
 }
 
-static CPUReadMemoryFunc * const tusb_async_readfn[] = {
-    tusb_async_readb,
-    tusb_async_readh,
-    tusb_async_readw,
-};
-
-static CPUWriteMemoryFunc * const tusb_async_writefn[] = {
-    tusb_async_writeb,
-    tusb_async_writeh,
-    tusb_async_writew,
+static const MemoryRegionOps tusb_async_ops = {
+    .old_mmio = {
+        .read = { tusb_async_readb, tusb_async_readh, tusb_async_readw, },
+        .write =  { tusb_async_writeb, tusb_async_writeh, tusb_async_writew, },
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 static void tusb_otg_tick(void *opaque)
@@ -739,8 +735,8 @@  TUSBState *tusb6010_init(qemu_irq intr)
     s->mask = 0xffffffff;
     s->intr = 0x00000000;
     s->otg_timer_val = 0;
-    s->iomemtype[1] = cpu_register_io_memory(tusb_async_readfn,
-                    tusb_async_writefn, s, DEVICE_NATIVE_ENDIAN);
+    memory_region_init_io(&s->iomem[1], &tusb_async_ops, s, "tusb-async",
+                          UINT32_MAX);
     s->irq = intr;
     s->otg_timer = qemu_new_timer_ns(vm_clock, tusb_otg_tick, s);
     s->pwr_timer = qemu_new_timer_ns(vm_clock, tusb_power_tick, s);
diff --git a/hw/tusb6010.h b/hw/tusb6010.h
index ebb3584..b85ee86 100644
--- a/hw/tusb6010.h
+++ b/hw/tusb6010.h
@@ -16,10 +16,13 @@ 
 #ifndef TUSB6010_H
 #define TUSB6010_H
 
+#include "targphys.h"
+#include "memory.h"
+
 typedef struct TUSBState TUSBState;
 TUSBState *tusb6010_init(qemu_irq intr);
-int tusb6010_sync_io(TUSBState *s);
-int tusb6010_async_io(TUSBState *s);
+MemoryRegion *tusb6010_sync_io(TUSBState *s);
+MemoryRegion *tusb6010_async_io(TUSBState *s);
 void tusb6010_power(TUSBState *s, int on);
 
 #endif