[Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Peter Chubb]

Reads to unassigned memory now return non-zero (since patch
9b8c69243585).  This breaks guests runnong on i.MX31 that use the
cache controller --- they poll forever waiting for the L2CC cache
invalidate regsiter to be zero.

This patch adds in an L2 cache controller.  It's not quite right ---
it reuses the PL2x0 implementation that  is already in QEMU.  The
differences however are minor --- a different ID, a different initial
value for the aux control register (because Freescale have used
some of the reserved bits), and the pl2x0 implements registers that
are not present in the Freescale cache controller.

Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>

diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index bd6c05c..018fc81 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -33,6 +33,7 @@
      * 0x1fffc000-0x1fffffff RAM                  EMULATED
      * 0x20000000-0x2fffffff Reserved             IGNORED
      * 0x30000000-0x7fffffff I.MX31 Internal Register Space
+     *   0x30000000-0x30000fff L2 Cache Controller PARTIALLY EMULATED
      *   0x43f00000 IO_AREA0
      *   0x43f90000 UART1                         EMULATED
      *   0x43f94000 UART2                         EMULATED
@@ -134,6 +135,15 @@ static void kzm_init(QEMUMachineInitArgs *args)
                        DEVICE_NATIVE_ENDIAN);
     }
 
+    /*
+     * The i.MX L2CC  is almost the same as the PL210
+     * except for a different ID (the implementor bits are different)
+     * and the `reserved' bits in the auxilliary control register
+     * are implemented.  The l2x0 qemu implementation is for a superset
+     * of the PL210.
+     */
+    sysbus_create_varargs("l2x0", 0x30000000, NULL);
+
     kzm_binfo.ram_size = ram_size;
     kzm_binfo.kernel_filename = kernel_filename;
     kzm_binfo.kernel_cmdline = kernel_cmdline;


--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Peter Maydell]

On 5 August 2013 02:21, Peter Chubb <peter.chubb@nicta.com.au> wrote:
> Reads to unassigned memory now return non-zero (since patch
> 9b8c69243585). This breaks guests runnong on i.MX31 that use the
> cache controller --- they poll forever waiting for the L2CC cache
> invalidate regsiter to be zero.

That commit claims it was just restoring the previous
behaviour, so it shouldn't break guests, surely?

-- PMM

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Andreas Färber]

Am 05.08.2013 11:18, schrieb Peter Maydell:
> On 5 August 2013 02:21, Peter Chubb <peter.chubb@nicta.com.au> wrote:
>> Reads to unassigned memory now return non-zero (since patch
>> 9b8c69243585). This breaks guests runnong on i.MX31 that use the
>> cache controller --- they poll forever waiting for the L2CC cache
>> invalidate regsiter to be zero.
> 
> That commit claims it was just restoring the previous
> behaviour, so it shouldn't break guests, surely?

See Jan's patches on the list. PReP was reported affected, too.

Peter Ch., if you know the exact differences, why don't you just derive
an imx-l2cc type (or so) derived from ARM's type, overriding the values
mentioned above? Sounds trivial to me.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[peter]

>>>>> "Andreas" == Andreas Färber <afaerber@suse.de> writes:

Andreas> Am 05.08.2013 11:18, schrieb Peter Maydell:
>> On 5 August 2013 02:21, Peter Chubb <peter.chubb@nicta.com.au>
>> wrote:
>>> Reads to unassigned memory now return non-zero (since patch
>>> 9b8c69243585). This breaks guests runnong on i.MX31 that use the
>>> cache controller --- they poll forever waiting for the L2CC cache
>>> invalidate regsiter to be zero.
>> 
Andreas> Peter Ch., if you know the exact differences, why don't you
Andreas> just derive an imx-l2cc type (or so) derived from ARM's type,
Andreas> overriding the values mentioned above? Sounds trivial to me.

Because I don't know how -- can you point me at some documentation?

Peter C

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Andreas Färber]

Am 06.08.2013 02:00, schrieb peter@chubb.wattle.id.au:
>>>>>> "Andreas" == Andreas Färber <afaerber@suse.de> writes:
> 
> Andreas> Am 05.08.2013 11:18, schrieb Peter Maydell:
>>> On 5 August 2013 02:21, Peter Chubb <peter.chubb@nicta.com.au>
>>> wrote:
>>>> Reads to unassigned memory now return non-zero (since patch
>>>> 9b8c69243585). This breaks guests runnong on i.MX31 that use the
>>>> cache controller --- they poll forever waiting for the L2CC cache
>>>> invalidate regsiter to be zero.
>>>
> Andreas> Peter Ch., if you know the exact differences, why don't you
> Andreas> just derive an imx-l2cc type (or so) derived from ARM's type,
> Andreas> overriding the values mentioned above? Sounds trivial to me.
> 
> Because I don't know how -- can you point me at some documentation?

There's no official how-to, but QOM is documented in include/qom/object.h.

May I simply point you to an example:
http://git.qemu.org/?p=qemu.git;a=commit;h=692a76d1c4a32573bf3cc19110c7fa6cc8c93f60

pl061 has Luminary and ARM IDs, with ARM in the base type and Luminary
overriding values. Another idea is to use an abstract base type and
several derived types if the differences are bigger.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Peter Chubb]

OK, this is what I've come up with.  Dunno whether it's right or not
-- the object model is decoupled from the memory model, so  there's no
straightforward way to override just a few of the registers.

At this stage this is just for comment, as I don't really have that
much of a clue about how the object/class hierarchy is meant to work.

---
 hw/arm/kzm.c       |    3 +
 hw/misc/arm_l2x0.c |  129 ++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 96 insertions(+), 36 deletions(-)

Index: qemu/hw/arm/kzm.c
===================================================================
--- qemu.orig/hw/arm/kzm.c	2013-08-07 11:21:48.864692846 +1000
+++ qemu/hw/arm/kzm.c	2013-08-07 11:22:40.292983604 +1000
@@ -33,6 +33,7 @@
      * 0x1fffc000-0x1fffffff RAM                  EMULATED
      * 0x20000000-0x2fffffff Reserved             IGNORED
      * 0x30000000-0x7fffffff I.MX31 Internal Register Space
+     *   0x30000000-0x30000fff L2 Cache Controller PARTIALLY EMULATED
      *   0x43f00000 IO_AREA0
      *   0x43f90000 UART1                         EMULATED
      *   0x43f94000 UART2                         EMULATED
@@ -134,6 +135,8 @@ static void kzm_init(QEMUMachineInitArgs
                        DEVICE_NATIVE_ENDIAN);
     }
 
+    sysbus_create_varargs("imx_l2cc", 0x30000000, NULL);
+
     kzm_binfo.ram_size = ram_size;
     kzm_binfo.kernel_filename = kernel_filename;
     kzm_binfo.kernel_cmdline = kernel_cmdline;
Index: qemu/hw/misc/arm_l2x0.c
===================================================================
--- qemu.orig/hw/misc/arm_l2x0.c	2013-08-07 11:21:48.864692846 +1000
+++ qemu/hw/misc/arm_l2x0.c	2013-08-07 11:21:48.860692824 +1000
@@ -21,7 +21,9 @@
 #include "hw/sysbus.h"
 
 /* L2C-310 r3p2 */
-#define CACHE_ID 0x410000c8
+#define PL310_CACHE_ID 0x410000c8
+/* L2CC from Freescale */
+#define IMX_PL2CC_CACHE_ID 0xD5000041
 
 #define TYPE_ARM_L2X0 "l2x0"
 #define ARM_L2X0(obj) OBJECT_CHECK(L2x0State, (obj), TYPE_ARM_L2X0)
@@ -30,6 +32,7 @@ typedef struct L2x0State {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
+    uint32_t cache_id;
     uint32_t cache_type;
     uint32_t ctrl;
     uint32_t aux_ctrl;
@@ -66,7 +69,7 @@ static uint64_t l2x0_priv_read(void *opa
     }
     switch (offset) {
     case 0:
-        return CACHE_ID;
+        return s->cache_id;
     case 0x4:
         /* aux_ctrl values affect cache_type values */
         cache_data = (s->aux_ctrl & (7 << 17)) >> 15;
@@ -78,23 +81,25 @@ static uint64_t l2x0_priv_read(void *opa
         return s->aux_ctrl;
     case 0x108:
         return s->tag_ctrl;
-    case 0x10C:
-        return s->data_ctrl;
-    case 0xC00:
-        return s->filter_start;
-    case 0xC04:
-        return s->filter_end;
     case 0xF40:
         return 0;
-    case 0xF60:
-        return 0;
-    case 0xF80:
-        return 0;
-    default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "l2x0_priv_read: Bad offset %x\n", (int)offset);
-        break;
     }
+    if (s->cache_id == PL310_CACHE_ID) {
+        switch (offset) {
+        case 0x10C:
+            return s->data_ctrl;
+        case 0xC00:
+            return s->filter_start;
+        case 0xC04:
+            return s->filter_end;
+        case 0xF60:
+            return 0;
+        case 0xF80:
+            return 0;
+        }
+    }
+    qemu_log_mask(LOG_GUEST_ERROR,
+                      "l2x0_priv_read: Bad offset %x\n", (int)offset);
     return 0;
 }
 
@@ -107,6 +112,7 @@ static void l2x0_priv_write(void *opaque
         /* ignore */
         return;
     }
+
     switch (offset) {
     case 0x100:
         s->ctrl = value & 1;
@@ -114,29 +120,32 @@ static void l2x0_priv_write(void *opaque
     case 0x104:
         s->aux_ctrl = value;
         break;
-    case 0x108:
-        s->tag_ctrl = value;
-        break;
-    case 0x10C:
-        s->data_ctrl = value;
-        break;
-    case 0xC00:
-        s->filter_start = value;
-        break;
-    case 0xC04:
-        s->filter_end = value;
-        break;
     case 0xF40:
         return;
-    case 0xF60:
-        return;
-    case 0xF80:
-        return;
-    default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "l2x0_priv_write: Bad offset %x\n", (int)offset);
-        break;
     }
+
+    if (s->cache_id == PL310_CACHE_ID) {
+        switch (offset) {
+        case 0x108:
+            s->tag_ctrl = value;
+            break;
+        case 0x10C:
+            s->data_ctrl = value;
+            break;
+        case 0xC00:
+            s->filter_start = value;
+            break;
+        case 0xC04:
+            s->filter_end = value;
+            break;
+        case 0xF60:
+            return;
+        case 0xF80:
+            return;
+        }
+    }
+    qemu_log_mask(LOG_GUEST_ERROR,
+                      "l2x0_priv_write: Bad offset %x\n", (int)offset);
 }
 
 static void l2x0_priv_reset(DeviceState *dev)
@@ -184,16 +193,64 @@ static void l2x0_class_init(ObjectClass
     dc->reset = l2x0_priv_reset;
 }
 
+static void l2x0_init(Object *obj)
+{
+    L2x0State *s = ARM_L2X0(obj);
+
+    s->cache_id = PL310_CACHE_ID;
+}
+
 static const TypeInfo l2x0_info = {
     .name = TYPE_ARM_L2X0,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(L2x0State),
+    .instance_init = l2x0_init,
     .class_init = l2x0_class_init,
 };
 
+/********************************
+ * The i.MX31 L2CC is a subset of the PL310 implemented above.
+ */
+
+static void imx_l2cc_init(Object *obj) {
+    L2x0State *s = ARM_L2X0(obj);
+
+    s->cache_id = IMX_PL2CC_CACHE_ID;
+}
+
+
+static void imx_l2cc_priv_reset(DeviceState *dev)
+{
+    L2x0State *s = ARM_L2X0(dev);
+
+    s->ctrl = 0;
+    s->aux_ctrl = 0xE4020FFF;
+}
+
+
+/*
+ * I assume the parent class's init function has already been called.
+ */
+static void imx_l2cc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = imx_l2cc_priv_reset;
+}
+
+
+static const TypeInfo imx_l2cc_info = {
+    .name = "imx_l2cc",
+    .parent = TYPE_ARM_L2X0,
+    .instance_init = imx_l2cc_init,
+    .class_init = imx_l2cc_class_init,
+};
+
+
 static void l2x0_register_types(void)
 {
     type_register_static(&l2x0_info);
+    type_register_static(&imx_l2cc_info);
 }
 
 type_init(l2x0_register_types)

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Peter Chubb]

>>>>> "Andreas" == Andreas Färber <afaerber@suse.de> writes:


Andreas> Peter Ch., if you know the exact differences, why don't you
Andreas> just derive an imx-l2cc type (or so) derived from ARM's type,
Andreas> overriding the values mentioned above? Sounds trivial to me.

For what it's worth, here's a diff between the arm_l2x0.c
implementation and a working imx_l2cc.c implementation.

Most of the diffs are name change; but there are substantive
differences to the initial values, and to which registers are
supported.  This makes the State variable smaller.

So it's a bit more than just overriding a few constants.

--- arm_l2x0.c	2013-08-06 09:59:30.008468028 +1000
+++ imx-l2cc-indep.c	2013-08-06 10:49:38.021514073 +1000
@@ -1,7 +1,10 @@
 /*
- * ARM dummy L210, L220, PL310 cache controller.
+ * IMX dummy level 2 cache controller
  *
- * Copyright (c) 2010-2012 Calxeda
+ * Copyright (c) 2013 NICTA Peter Chubb
+ *
+ * Based on the PL210 implementation in arm_l2x0.c
+ * Differences: different Cache ID and aux control register values.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -20,46 +23,40 @@
 
 #include "hw/sysbus.h"
 
-/* L2C-310 r3p2 */
-#define CACHE_ID 0x410000c8
 
-#define TYPE_ARM_L2X0 "l2x0"
-#define ARM_L2X0(obj) OBJECT_CHECK(L2x0State, (obj), TYPE_ARM_L2X0)
+#define CACHE_ID 0xd5000041
+#define CACHE_TYPE_DEFAULT 0x1C100100 
+
+#define TYPE_IMX_L2CC "imx_l2cc"
+#define IMX_L2CC(obj) OBJECT_CHECK(L2ccState, (obj), TYPE_IMX_L2CC)
 
-typedef struct L2x0State {
+typedef struct L2ccState {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
     uint32_t cache_type;
     uint32_t ctrl;
     uint32_t aux_ctrl;
-    uint32_t data_ctrl;
-    uint32_t tag_ctrl;
-    uint32_t filter_start;
-    uint32_t filter_end;
-} L2x0State;
+} L2ccState;
 
-static const VMStateDescription vmstate_l2x0 = {
-    .name = "l2x0",
+static const VMStateDescription vmstate_l2cc = {
+    .name = "imx_l2cc",
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(ctrl, L2x0State),
-        VMSTATE_UINT32(aux_ctrl, L2x0State),
-        VMSTATE_UINT32(data_ctrl, L2x0State),
-        VMSTATE_UINT32(tag_ctrl, L2x0State),
-        VMSTATE_UINT32(filter_start, L2x0State),
-        VMSTATE_UINT32(filter_end, L2x0State),
+        VMSTATE_UINT32(ctrl, L2ccState),
+        VMSTATE_UINT32(cache_type, L2ccState),
+        VMSTATE_UINT32(aux_ctrl, L2ccState),
         VMSTATE_END_OF_LIST()
     }
 };
 
 
-static uint64_t l2x0_priv_read(void *opaque, hwaddr offset,
+static uint64_t l2cc_priv_read(void *opaque, hwaddr offset,
                                unsigned size)
 {
     uint32_t cache_data;
-    L2x0State *s = (L2x0State *)opaque;
+    L2ccState *s = (L2ccState *)opaque;
     offset &= 0xfff;
     if (offset >= 0x730 && offset < 0x800) {
         return 0; /* cache ops complete */
@@ -76,32 +73,20 @@
         return s->ctrl;
     case 0x104:
         return s->aux_ctrl;
-    case 0x108:
-        return s->tag_ctrl;
-    case 0x10C:
-        return s->data_ctrl;
-    case 0xC00:
-        return s->filter_start;
-    case 0xC04:
-        return s->filter_end;
     case 0xF40:
         return 0;
-    case 0xF60:
-        return 0;
-    case 0xF80:
-        return 0;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "l2x0_priv_read: Bad offset %x\n", (int)offset);
+                      "l2cc_priv_read: Bad offset %x\n", (int)offset);
         break;
     }
     return 0;
 }
 
-static void l2x0_priv_write(void *opaque, hwaddr offset,
+static void l2cc_priv_write(void *opaque, hwaddr offset,
                             uint64_t value, unsigned size)
 {
-    L2x0State *s = (L2x0State *)opaque;
+    L2ccState *s = (L2ccState *)opaque;
     offset &= 0xfff;
     if (offset >= 0x730 && offset < 0x800) {
         /* ignore */
@@ -114,86 +99,61 @@
     case 0x104:
         s->aux_ctrl = value;
         break;
-    case 0x108:
-        s->tag_ctrl = value;
-        break;
-    case 0x10C:
-        s->data_ctrl = value;
-        break;
-    case 0xC00:
-        s->filter_start = value;
-        break;
-    case 0xC04:
-        s->filter_end = value;
-        break;
     case 0xF40:
         return;
-    case 0xF60:
-        return;
-    case 0xF80:
-        return;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "l2x0_priv_write: Bad offset %x\n", (int)offset);
+                      "l2cc_priv_write: Bad offset %x\n", (int)offset);
         break;
     }
 }
 
-static void l2x0_priv_reset(DeviceState *dev)
+static void l2cc_priv_reset(DeviceState *dev)
 {
-    L2x0State *s = ARM_L2X0(dev);
+    L2ccState *s = IMX_L2CC(dev);

     s->ctrl = 0;
-    s->aux_ctrl = 0x02020000;
-    s->tag_ctrl = 0;
-    s->data_ctrl = 0;
-    s->filter_start = 0;
-    s->filter_end = 0;
+    s->cache_type = CACHE_TYPE_DEFAULT;
+    s->aux_ctrl = 0xE4020FFF;
 }
 
-static const MemoryRegionOps l2x0_mem_ops = {
-    .read = l2x0_priv_read,
-    .write = l2x0_priv_write,
+static const MemoryRegionOps l2cc_mem_ops = {
+    .read = l2cc_priv_read,
+    .write = l2cc_priv_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
  };
 
-static int l2x0_priv_init(SysBusDevice *dev)
+static int l2cc_priv_init(SysBusDevice *dev)
 {
-    L2x0State *s = ARM_L2X0(dev);
+    L2ccState *s = IMX_L2CC(dev);
 
-    memory_region_init_io(&s->iomem, OBJECT(dev), &l2x0_mem_ops, s,
-                          "l2x0_cc", 0x1000);
+    memory_region_init_io(&s->iomem, OBJECT(dev), &l2cc_mem_ops, s,
+                          "l2cc_cc", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
     return 0;
 }
 
-static Property l2x0_properties[] = {
-    DEFINE_PROP_UINT32("cache-type", L2x0State, cache_type, 0x1c100100),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void l2x0_class_init(ObjectClass *klass, void *data)
+static void l2cc_class_init(ObjectClass *klass, void *data)
 {
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init = l2x0_priv_init;
-    dc->vmsd = &vmstate_l2x0;
+    k->init = l2cc_priv_init;
+    dc->vmsd = &vmstate_l2cc;
     dc->no_user = 1;
-    dc->props = l2x0_properties;
-    dc->reset = l2x0_priv_reset;
+    dc->reset = l2cc_priv_reset;
 }
 
-static const TypeInfo l2x0_info = {
-    .name = TYPE_ARM_L2X0,
+static const TypeInfo l2cc_info = {
+    .name = TYPE_IMX_L2CC,
     .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(L2x0State),
-    .class_init = l2x0_class_init,
+    .instance_size = sizeof(L2ccState),
+    .class_init = l2cc_class_init,
 };
 
-static void l2x0_register_types(void)
+static void l2cc_register_types(void)
 {
-    type_register_static(&l2x0_info);
+    type_register_static(&l2cc_info);
 }
 
-type_init(l2x0_register_types)
+type_init(l2cc_register_types)


--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

Re: [Qemu-devel] [Patch] ARM: Add an L2 cache controller to KZM

[Peter Chubb]

>>>>> "Peter" == Peter Maydell <peter.maydell@linaro.org> writes:

Peter> On 5 August 2013 02:21, Peter Chubb <peter.chubb@nicta.com.au>
Peter> wrote:
>> Reads to unassigned memory now return non-zero (since patch
>> 9b8c69243585). This breaks guests runnong on i.MX31 that use the
>> cache controller --- they poll forever waiting for the L2CC cache
>> invalidate regsiter to be zero.

Peter> That commit claims it was just restoring the previous
Peter> behaviour, so it shouldn't break guests, surely?

The behaviour was introduced in 2008 in commit e18231a3 --- The KZM
port only went in last year and assumed the then-current behaviour.

Peter C
--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA