[Qemu-devel] [PATCH] integrator: fix Linux boot failure by emulating dbg

[Qemu-devel] [PATCH] integrator: fix Linux boot failure by emulating dbg

[alex.bennee]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=true, Size: 4417 bytes --]

From: Alex Bennée <alex@bennee.com>

Commit 9b8c69243 broke the ability to boot the kernel as the value
returned by unassigned_mem_read returned non-zero and left the kernel
looping forever waiting for it to change (see integrator_led_set in
the kernel code).

Relying on a varying implementation detail is incorrect anyway so this
introduces a memory region to emulate the debug/led region on the
integrator board. It is currently a basic stub as I have no idea what the
behaviour of this region should be so for now it simply returns 0's as
the old unassigned_mem_read did.
---
 hw/arm/integratorcp.c |  2 ++
 hw/misc/Makefile.objs |  1 +
 hw/misc/arm_intdbg.c  | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 hw/misc/arm_intdbg.c

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 2ef93ed..e5d07c4 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -446,6 +446,7 @@ static void icp_control_init(hwaddr base)
 }
 
 
+
 /* Board init.  */
 
 static struct arm_boot_info integrator_binfo = {
@@ -508,6 +509,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
     icp_control_init(0xcb000000);
     sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
     sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
+    sysbus_create_simple("integrator_dbg", 0x1a000000, 0);
     sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
     if (nd_table[0].used)
         smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2578e29..e2c84a3 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -22,6 +22,7 @@ obj-$(CONFIG_LINUX) += vfio.o
 endif
 
 obj-$(CONFIG_REALVIEW) += arm_sysctl.o
+obj-y += arm_intdbg.o
 obj-$(CONFIG_A9SCU) += a9scu.o
 obj-$(CONFIG_NSERIES) += cbus.o
 obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c
new file mode 100644
index 0000000..1f21447
--- /dev/null
+++ b/hw/misc/arm_intdbg.c
@@ -0,0 +1,90 @@
+/*
+ * LED, Switch and Debug control registers for ARM Integrator Boards
+ *
+ * This currently is a stub for this functionality written with
+ * reference to what the Linux kernel looks at. Previously we relied
+ * on the behaviour of unassigned_mem_read() in the core.
+ *
+ * Written by Alex Bennée
+ *
+ * This code is licensed under the GPL.
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+
+#define TYPE_ARM_INTDBG "integrator_dbg"
+#define ARM_INTDBG(obj) \
+    OBJECT_CHECK(arm_intdbg_state, (obj), TYPE_ARM_INTDBG)
+
+typedef struct {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    uint32_t alpha;
+    uint32_t leds;
+    uint32_t switches;
+} arm_intdbg_state;
+
+static uint64_t dbg_control_read(void *opaque, hwaddr offset,
+                                 unsigned size)
+{
+    switch (offset >> 2) {
+    case 0: /* ALPHA */
+        return 0;
+    case 1: /* LEDS */
+        return 0;
+    case 2: /* SWITCHES */
+        return 0;
+    default:
+        hw_error("dbg_control_read: Bad offset %x\n", (int)offset);
+        return 0;
+    }
+}
+
+static void dbg_control_write(void *opaque, hwaddr offset,
+                              uint64_t value, unsigned size)
+{
+    switch (offset >> 2) {
+    case 1: /* ALPHA */
+    case 2: /* LEDS */
+    case 3: /* SWITCHES */
+        /* Nothing interesting implemented yet.  */
+        break;
+    default:
+        hw_error("dbg_control_write: Bad offset %x\n", (int)offset);
+    }
+}
+
+static const MemoryRegionOps dbg_control_ops = {
+    .read = dbg_control_read,
+    .write = dbg_control_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void dbg_control_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    arm_intdbg_state *s = ARM_INTDBG(obj);
+    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+/*
+  Hardware boilerplate
+*/
+
+static const TypeInfo arm_intdbg_info = {
+    .name          = TYPE_ARM_INTDBG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(arm_intdbg_state),
+    .instance_init = dbg_control_init,
+};
+
+static void arm_intdbg_register_types(void)
+{
+    type_register_static(&arm_intdbg_info);
+}
+
+type_init(arm_intdbg_register_types)
-- 
1.8.4

Re: [Qemu-devel] [PATCH] integrator: fix Linux boot failure by emulating dbg

[Peter Maydell]

On 13 September 2013 15:58,  <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> Commit 9b8c69243 broke the ability to boot the kernel as the value
> returned by unassigned_mem_read returned non-zero and left the kernel
> looping forever waiting for it to change (see integrator_led_set in
> the kernel code).
>
> Relying on a varying implementation detail is incorrect anyway so this
> introduces a memory region to emulate the debug/led region on the
> integrator board. It is currently a basic stub as I have no idea what the
> behaviour of this region should be

See the integrator board documentation:

http://infocenter.arm.com/help/topic/com.arm.doc.dui0159b/Babbfijf.html

> so for now it simply returns 0's as
> the old unassigned_mem_read did.
> ---
>  hw/arm/integratorcp.c |  2 ++
>  hw/misc/Makefile.objs |  1 +
>  hw/misc/arm_intdbg.c  | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 hw/misc/arm_intdbg.c
>
> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> index 2ef93ed..e5d07c4 100644
> --- a/hw/arm/integratorcp.c
> +++ b/hw/arm/integratorcp.c
> @@ -446,6 +446,7 @@ static void icp_control_init(hwaddr base)
>  }
>
>
> +

Stray whitespace change.

>  /* Board init.  */
>
>  static struct arm_boot_info integrator_binfo = {
> @@ -508,6 +509,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
>      icp_control_init(0xcb000000);
>      sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
>      sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
> +    sysbus_create_simple("integrator_dbg", 0x1a000000, 0);
>      sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
>      if (nd_table[0].used)
>          smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 2578e29..e2c84a3 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -22,6 +22,7 @@ obj-$(CONFIG_LINUX) += vfio.o
>  endif
>
>  obj-$(CONFIG_REALVIEW) += arm_sysctl.o
> +obj-y += arm_intdbg.o

The device isn't depending on anything target specific
so it can go in common-obj-. It needs its own
CONFIG_INTEGRATOR_DBG in default-configs/arm-softmmu.mak
so you can make it common-obj-$(CONFIG_INTEGRATOR_DBG) +=.

>  obj-$(CONFIG_A9SCU) += a9scu.o
>  obj-$(CONFIG_NSERIES) += cbus.o
>  obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
> diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c
> new file mode 100644
> index 0000000..1f21447
> --- /dev/null
> +++ b/hw/misc/arm_intdbg.c
> @@ -0,0 +1,90 @@
> +/*
> + * LED, Switch and Debug control registers for ARM Integrator Boards
> + *
> + * This currently is a stub for this functionality written with
> + * reference to what the Linux kernel looks at. Previously we relied
> + * on the behaviour of unassigned_mem_read() in the core.
> + *
> + * Written by Alex Bennée
> + *
> + * This code is licensed under the GPL.

Would be nice to give a specific version of the GPL...
("2" or "2 or later" are your two basic options.)

> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "exec/address-spaces.h"
> +
> +#define TYPE_ARM_INTDBG "integrator_dbg"
> +#define ARM_INTDBG(obj) \
> +    OBJECT_CHECK(arm_intdbg_state, (obj), TYPE_ARM_INTDBG)
> +
> +typedef struct {
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +
> +    uint32_t alpha;
> +    uint32_t leds;
> +    uint32_t switches;
> +} arm_intdbg_state;

CODING_STYLE says type names should be camelcase, so
ARMIntDbgState.

> +
> +static uint64_t dbg_control_read(void *opaque, hwaddr offset,
> +                                 unsigned size)
> +{
> +    switch (offset >> 2) {
> +    case 0: /* ALPHA */
> +        return 0;
> +    case 1: /* LEDS */
> +        return 0;
> +    case 2: /* SWITCHES */
> +        return 0;

You could log these with qemu_log_mask(LOG_UNIMP, ...)

> +    default:
> +        hw_error("dbg_control_read: Bad offset %x\n", (int)offset);

Don't hw_error() for something a guest can provoke.
You can just log the info to the debug log (if the
user has enabled it) and press on:
        qemu_log_mask(LOG_GUEST_ERROR,
                      "dbg_control_read: Bad offset %x\n", (int)offset);

> +        return 0;
> +    }
> +}
> +
> +static void dbg_control_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    switch (offset >> 2) {
> +    case 1: /* ALPHA */
> +    case 2: /* LEDS */
> +    case 3: /* SWITCHES */
> +        /* Nothing interesting implemented yet.  */
> +        break;
> +    default:
> +        hw_error("dbg_control_write: Bad offset %x\n", (int)offset);

Same remarks as above about logging unimplemented
and bad guest behaviour.

> +    }
> +}
> +
> +static const MemoryRegionOps dbg_control_ops = {
> +    .read = dbg_control_read,
> +    .write = dbg_control_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void dbg_control_init(Object *obj)
> +{
> +    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
> +    arm_intdbg_state *s = ARM_INTDBG(obj);
> +    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000);

(Does this line overflow 80 characters? You can
catch this and other style errors by running your
patch through scripts/checkpatch.pl...)

The documentation says the size of this region is 16MB,
ie 0x1000000.  (Actually it says we effectively don't
decode most of the address lines so it probably repeats
every 16 bytes, but let's not worry about that right now.)

> +    sysbus_init_mmio(sd, &s->iomem);
> +}
> +
> +/*
> +  Hardware boilerplate
> +*/

You don't need to add a comment telling us QOM involves
lots of boilerplate, we already know that :-)

> +
> +static const TypeInfo arm_intdbg_info = {
> +    .name          = TYPE_ARM_INTDBG,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(arm_intdbg_state),
> +    .instance_init = dbg_control_init,
> +};
> +
> +static void arm_intdbg_register_types(void)
> +{
> +    type_register_static(&arm_intdbg_info);
> +}
> +
> +type_init(arm_intdbg_register_types)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] integrator: fix Linux boot failure by emulating dbg

[Peter Maydell]

On 13 September 2013 16:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 September 2013 15:58,  <alex.bennee@linaro.org> wrote:
>> + * This code is licensed under the GPL.
>
> Would be nice to give a specific version of the GPL...
> ("2" or "2 or later" are your two basic options.)

...but I forgot to mention that "2-or-later" is strongly
preferred.

-- PMM

[Qemu-devel] [PATCH v2 0/0] integrator: fix Linux boot failure by emulating dbg

[alex.bennee]

Hi,

I've applied the review comments from Peter.

Alex.

[Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[alex.bennee]

From: Alex Bennée <alex@bennee.com>

Commit 9b8c69243 broke the ability to boot the kernel as the value
returned by unassigned_mem_read returned non-zero and left the kernel
looping forever waiting for it to change (see integrator_led_set in
the kernel code).

Relying on a varying implementation detail is incorrect anyway so this
introduces a memory region to emulate the debug/led region on the
integrator board. It is currently a basic stub as I have no idea what the
behaviour of this region should be so for now it simply returns 0's as
the old unassigned_mem_read did.

Signed-off-by: Alex Bennée <alex@bennee.com>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/integratorcp.c           |  1 +
 hw/misc/Makefile.objs           |  1 +
 hw/misc/arm_intdbg.c            | 90 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+)
 create mode 100644 hw/misc/arm_intdbg.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ac0815d..a5718d1 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -80,3 +80,4 @@ CONFIG_VERSATILE_PCI=y
 CONFIG_VERSATILE_I2C=y
 
 CONFIG_SDHCI=y
+CONFIG_INTEGRATOR_DBG=y
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 2ef93ed..46dc615 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -508,6 +508,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
     icp_control_init(0xcb000000);
     sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
     sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
+    sysbus_create_simple("integrator_dbg", 0x1a000000, 0);
     sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
     if (nd_table[0].used)
         smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2578e29..be284f3 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) += vmport.o
 
 # ARM devices
 common-obj-$(CONFIG_PL310) += arm_l2x0.o
+common-obj-$(CONFIG_INTEGRATOR_DBG) += arm_intdbg.o
 
 # PKUnity SoC devices
 common-obj-$(CONFIG_PUV3) += puv3_pm.o
diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c
new file mode 100644
index 0000000..b505d09
--- /dev/null
+++ b/hw/misc/arm_intdbg.c
@@ -0,0 +1,90 @@
+/*
+ * LED, Switch and Debug control registers for ARM Integrator Boards
+ *
+ * This currently is a stub for this functionality written with
+ * reference to what the Linux kernel looks at. Previously we relied
+ * on the behaviour of unassigned_mem_read() in the core.
+ *
+ * The real h/w is described at:
+ *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
+ *
+ * Written by Alex Bennée
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+
+#define TYPE_ARM_INTDBG "integrator_dbg"
+#define ARM_INTDBG(obj) \
+    OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG)
+
+typedef struct {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    uint32_t alpha;
+    uint32_t leds;
+    uint32_t switches;
+} ARMIntDbgState;
+
+static uint64_t dbg_control_read(void *opaque, hwaddr offset,
+                                 unsigned size)
+{
+    switch (offset >> 2) {
+    case 0: /* ALPHA */
+    case 1: /* LEDS */
+    case 2: /* SWITCHES */
+        qemu_log_mask(LOG_UNIMP, "dbg_control_read: returning zero from %x:%d\n", (int)offset, size);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %x\n", (int)offset);
+        return 0;
+    }
+}
+
+static void dbg_control_write(void *opaque, hwaddr offset,
+                              uint64_t value, unsigned size)
+{
+    switch (offset >> 2) {
+    case 1: /* ALPHA */
+    case 2: /* LEDS */
+    case 3: /* SWITCHES */
+        /* Nothing interesting implemented yet.  */
+        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset);
+    }
+}
+
+static const MemoryRegionOps dbg_control_ops = {
+    .read = dbg_control_read,
+    .write = dbg_control_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void dbg_control_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    ARMIntDbgState *s = ARM_INTDBG(obj);
+    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+static const TypeInfo arm_intdbg_info = {
+    .name          = TYPE_ARM_INTDBG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMIntDbgState),
+    .instance_init = dbg_control_init,
+};
+
+static void arm_intdbg_register_types(void)
+{
+    type_register_static(&arm_intdbg_info);
+}
+
+type_init(arm_intdbg_register_types)
-- 
1.8.4

Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Peter Maydell]

On 16 September 2013 13:50,  <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> Commit 9b8c69243 broke the ability to boot the kernel as the value
> returned by unassigned_mem_read returned non-zero and left the kernel
> looping forever waiting for it to change (see integrator_led_set in
> the kernel code).

This generally looks OK, but I have a few nits.

> Relying on a varying implementation detail is incorrect anyway so this
> introduces a memory region to emulate the debug/led region on the
> integrator board. It is currently a basic stub as I have no idea what the
> behaviour of this region should be so for now it simply returns 0's as
> the old unassigned_mem_read did.

It looks like you need to update the commit message -- you
looked at the board docs, so we do know the correct behaviour.

> +++ b/hw/misc/arm_intdbg.c
> @@ -0,0 +1,90 @@
> +/*
> + * LED, Switch and Debug control registers for ARM Integrator Boards
> + *
> + * This currently is a stub for this functionality written with
> + * reference to what the Linux kernel looks at. Previously we relied
> + * on the behaviour of unassigned_mem_read() in the core.

This sounds like the code was written based on the kernel, not
on the board docs, which is wrong, so I think it could use
rephrasing. There's no need to mention previous behaviour either
IMHO -- people who care can look at the commit history.

> + *
> + * The real h/w is described at:
> + *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
> + *
> + * Written by Alex Bennée

Assuming you wrote this with your Linaro hat on, the comment
should have a
 * Copyright (c) 2013 Linaro Limited

in it above your 'Written by' line.

> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */

> +static void dbg_control_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    switch (offset >> 2) {
> +    case 1: /* ALPHA */
> +    case 2: /* LEDS */
> +    case 3: /* SWITCHES */
> +        /* Nothing interesting implemented yet.  */
> +        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset);

This looks like an overlength line : checkpatch.pl should
warn if so. (I think there are others too.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Alex Bennée]

peter.maydell@linaro.org writes:

> On 16 September 2013 13:50,  <alex.bennee@linaro.org> wrote:
>> From: Alex Bennée <alex@bennee.com>
>>
>> Commit 9b8c69243 broke the ability to boot the kernel as the value
>> returned by unassigned_mem_read returned non-zero and left the kernel
>> looping forever waiting for it to change (see integrator_led_set in
>> the kernel code).
>
> This generally looks OK, but I have a few nits.

OK

>> Relying on a varying implementation detail is incorrect anyway so this
>> introduces a memory region to emulate the debug/led region on the
>> integrator board. It is currently a basic stub as I have no idea what the
>> behaviour of this region should be so for now it simply returns 0's as
>> the old unassigned_mem_read did.
>
> It looks like you need to update the commit message -- you
> looked at the board docs, so we do know the correct behaviour.

True, I shall update.

>> +++ b/hw/misc/arm_intdbg.c
>> @@ -0,0 +1,90 @@
>> +/*
>> + * LED, Switch and Debug control registers for ARM Integrator Boards
>> + *
>> + * This currently is a stub for this functionality written with
>> + * reference to what the Linux kernel looks at. Previously we relied
>> + * on the behaviour of unassigned_mem_read() in the core.
>
> This sounds like the code was written based on the kernel, not
> on the board docs, which is wrong, so I think it could use
> rephrasing. There's no need to mention previous behaviour either
> IMHO -- people who care can look at the commit history.

Sure. I've got a bunch of other review comments to address so I can
clean this up. Unfortunately I've been so busy with other stuff I
haven't had a chance to go through them yet.

As it happens it looks like the integrator image is now running again on
the current HEAD which has made this a little less urgent. However it
does raise an interesting question as to why!

>> + *
>> + * The real h/w is described at:
>> + *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
>> + *
>> + * Written by Alex Bennée
>
> Assuming you wrote this with your Linaro hat on, the comment
> should have a
>  * Copyright (c) 2013 Linaro Limited
>
> in it above your 'Written by' line.

Yeah, I was holding off that until I was officially on the clock
although as noticed I have been taking advantage of the email facilities
as a handy place to have the mailing list go through.

<snip>
>> +        /* Nothing interesting implemented yet.  */
>> +        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset);
>
> This looks like an overlength line : checkpatch.pl should
> warn if so. (I think there are others too.)

OK thanks.

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Peter Maydell]

On 15 October 2013 15:25, Alex Bennée <alex.bennee@linaro.org> wrote:
> peter.maydell@linaro.org writes:
>> This sounds like the code was written based on the kernel, not
>> on the board docs, which is wrong, so I think it could use
>> rephrasing. There's no need to mention previous behaviour either
>> IMHO -- people who care can look at the commit history.
>
> Sure. I've got a bunch of other review comments to address so I can
> clean this up. Unfortunately I've been so busy with other stuff I
> haven't had a chance to go through them yet.
>
> As it happens it looks like the integrator image is now running again on
> the current HEAD which has made this a little less urgent. However it
> does raise an interesting question as to why!

Commits 3bb28b72 and 68a7439a1 reverted the changes to the
behaviour of reads from unassigned regions (since they were
causing widespread brokenness).

>>> + * Written by Alex Bennée
>>
>> Assuming you wrote this with your Linaro hat on, the comment
>> should have a
>>  * Copyright (c) 2013 Linaro Limited
>>
>> in it above your 'Written by' line.
>
> Yeah, I was holding off that until I was officially on the clock
> although as noticed I have been taking advantage of the email facilities
> as a handy place to have the mailing list go through.

Well, it needs *some* copyright line, at least, for whichever
entity has the rights to the code and ability to (re)license it...

thanks
-- PMM

[Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[alex.bennee]

From: Alex Bennée <alex@bennee.com>

Commit 9b8c69243 broke the ability to boot the kernel as the value
returned by unassigned_mem_read returned non-zero and left the kernel
looping forever waiting for it to change (see integrator_led_set in
the kernel code).

Relying on a varying implementation detail is incorrect anyway so this
introduces a memory region to emulate the debug/led region on the
integrator board. It is currently a basic stub as I have no idea what the
behaviour of this region should be so for now it simply returns 0's as
the old unassigned_mem_read did.

Signed-off-by: Alex Bennée <alex@bennee.com>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/integratorcp.c           |  1 +
 hw/misc/Makefile.objs           |  1 +
 hw/misc/arm_intdbg.c            | 90 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+)
 create mode 100644 hw/misc/arm_intdbg.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ac0815d..a5718d1 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -80,3 +80,4 @@ CONFIG_VERSATILE_PCI=y
 CONFIG_VERSATILE_I2C=y
 
 CONFIG_SDHCI=y
+CONFIG_INTEGRATOR_DBG=y
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 2ef93ed..46dc615 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -508,6 +508,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
     icp_control_init(0xcb000000);
     sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
     sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
+    sysbus_create_simple("integrator_dbg", 0x1a000000, 0);
     sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
     if (nd_table[0].used)
         smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2578e29..be284f3 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) += vmport.o
 
 # ARM devices
 common-obj-$(CONFIG_PL310) += arm_l2x0.o
+common-obj-$(CONFIG_INTEGRATOR_DBG) += arm_intdbg.o
 
 # PKUnity SoC devices
 common-obj-$(CONFIG_PUV3) += puv3_pm.o
diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c
new file mode 100644
index 0000000..b505d09
--- /dev/null
+++ b/hw/misc/arm_intdbg.c
@@ -0,0 +1,90 @@
+/*
+ * LED, Switch and Debug control registers for ARM Integrator Boards
+ *
+ * This currently is a stub for this functionality written with
+ * reference to what the Linux kernel looks at. Previously we relied
+ * on the behaviour of unassigned_mem_read() in the core.
+ *
+ * The real h/w is described at:
+ *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
+ *
+ * Written by Alex Bennée
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+
+#define TYPE_ARM_INTDBG "integrator_dbg"
+#define ARM_INTDBG(obj) \
+    OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG)
+
+typedef struct {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    uint32_t alpha;
+    uint32_t leds;
+    uint32_t switches;
+} ARMIntDbgState;
+
+static uint64_t dbg_control_read(void *opaque, hwaddr offset,
+                                 unsigned size)
+{
+    switch (offset >> 2) {
+    case 0: /* ALPHA */
+    case 1: /* LEDS */
+    case 2: /* SWITCHES */
+        qemu_log_mask(LOG_UNIMP, "dbg_control_read: returning zero from %x:%d\n", (int)offset, size);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %x\n", (int)offset);
+        return 0;
+    }
+}
+
+static void dbg_control_write(void *opaque, hwaddr offset,
+                              uint64_t value, unsigned size)
+{
+    switch (offset >> 2) {
+    case 1: /* ALPHA */
+    case 2: /* LEDS */
+    case 3: /* SWITCHES */
+        /* Nothing interesting implemented yet.  */
+        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset);
+    }
+}
+
+static const MemoryRegionOps dbg_control_ops = {
+    .read = dbg_control_read,
+    .write = dbg_control_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void dbg_control_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    ARMIntDbgState *s = ARM_INTDBG(obj);
+    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+static const TypeInfo arm_intdbg_info = {
+    .name          = TYPE_ARM_INTDBG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMIntDbgState),
+    .instance_init = dbg_control_init,
+};
+
+static void arm_intdbg_register_types(void)
+{
+    type_register_static(&arm_intdbg_info);
+}
+
+type_init(arm_intdbg_register_types)
-- 
1.8.4

[Qemu-devel] [PATCH v2 0/0] integrator: fix Linux boot failure by emulating dbg

[alex.bennee]

Hi,

I mistakenly appended this to the previous patch revision. I'm now
sending for further review.

Since v1:
      - Updated with Peter Maydell's review comments.

--
Alex Bennée
      

[Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[alex.bennee]

From: Alex Bennée <alex@bennee.com>

Commit 9b8c69243 broke the ability to boot the kernel as the value
returned by unassigned_mem_read returned non-zero and left the kernel
looping forever waiting for it to change (see integrator_led_set in
the kernel code).

Relying on a varying implementation detail is incorrect anyway so this
introduces a memory region to emulate the debug/led region on the
integrator board. It is currently a basic stub as I have no idea what the
behaviour of this region should be so for now it simply returns 0's as
the old unassigned_mem_read did.

Signed-off-by: Alex Bennée <alex@bennee.com>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/integratorcp.c           |  1 +
 hw/misc/Makefile.objs           |  1 +
 hw/misc/arm_intdbg.c            | 90 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+)
 create mode 100644 hw/misc/arm_intdbg.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ac0815d..a5718d1 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -80,3 +80,4 @@ CONFIG_VERSATILE_PCI=y
 CONFIG_VERSATILE_I2C=y
 
 CONFIG_SDHCI=y
+CONFIG_INTEGRATOR_DBG=y
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 2ef93ed..46dc615 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -508,6 +508,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
     icp_control_init(0xcb000000);
     sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
     sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
+    sysbus_create_simple("integrator_dbg", 0x1a000000, 0);
     sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
     if (nd_table[0].used)
         smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2578e29..be284f3 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) += vmport.o
 
 # ARM devices
 common-obj-$(CONFIG_PL310) += arm_l2x0.o
+common-obj-$(CONFIG_INTEGRATOR_DBG) += arm_intdbg.o
 
 # PKUnity SoC devices
 common-obj-$(CONFIG_PUV3) += puv3_pm.o
diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c
new file mode 100644
index 0000000..b505d09
--- /dev/null
+++ b/hw/misc/arm_intdbg.c
@@ -0,0 +1,90 @@
+/*
+ * LED, Switch and Debug control registers for ARM Integrator Boards
+ *
+ * This currently is a stub for this functionality written with
+ * reference to what the Linux kernel looks at. Previously we relied
+ * on the behaviour of unassigned_mem_read() in the core.
+ *
+ * The real h/w is described at:
+ *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
+ *
+ * Written by Alex Bennée
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+
+#define TYPE_ARM_INTDBG "integrator_dbg"
+#define ARM_INTDBG(obj) \
+    OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG)
+
+typedef struct {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    uint32_t alpha;
+    uint32_t leds;
+    uint32_t switches;
+} ARMIntDbgState;
+
+static uint64_t dbg_control_read(void *opaque, hwaddr offset,
+                                 unsigned size)
+{
+    switch (offset >> 2) {
+    case 0: /* ALPHA */
+    case 1: /* LEDS */
+    case 2: /* SWITCHES */
+        qemu_log_mask(LOG_UNIMP, "dbg_control_read: returning zero from %x:%d\n", (int)offset, size);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %x\n", (int)offset);
+        return 0;
+    }
+}
+
+static void dbg_control_write(void *opaque, hwaddr offset,
+                              uint64_t value, unsigned size)
+{
+    switch (offset >> 2) {
+    case 1: /* ALPHA */
+    case 2: /* LEDS */
+    case 3: /* SWITCHES */
+        /* Nothing interesting implemented yet.  */
+        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset);
+    }
+}
+
+static const MemoryRegionOps dbg_control_ops = {
+    .read = dbg_control_read,
+    .write = dbg_control_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void dbg_control_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    ARMIntDbgState *s = ARM_INTDBG(obj);
+    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+static const TypeInfo arm_intdbg_info = {
+    .name          = TYPE_ARM_INTDBG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMIntDbgState),
+    .instance_init = dbg_control_init,
+};
+
+static void arm_intdbg_register_types(void)
+{
+    type_register_static(&arm_intdbg_info);
+}
+
+type_init(arm_intdbg_register_types)
-- 
1.8.4

Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Andreas Färber]

Am 18.09.2013 16:31, schrieb alex.bennee@linaro.org:
> From: Alex Bennée <alex@bennee.com>
> 
> Commit 9b8c69243 broke the ability to boot the kernel as the value
> returned by unassigned_mem_read returned non-zero and left the kernel
> looping forever waiting for it to change (see integrator_led_set in
> the kernel code).
> 
> Relying on a varying implementation detail is incorrect anyway so this
> introduces a memory region to emulate the debug/led region on the
> integrator board. It is currently a basic stub as I have no idea what the
> behaviour of this region should be so for now it simply returns 0's as
> the old unassigned_mem_read did.
> 
> Signed-off-by: Alex Bennée <alex@bennee.com>
> ---
>  default-configs/arm-softmmu.mak |  1 +
>  hw/arm/integratorcp.c           |  1 +
>  hw/misc/Makefile.objs           |  1 +
>  hw/misc/arm_intdbg.c            | 90 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 93 insertions(+)
>  create mode 100644 hw/misc/arm_intdbg.c

Looks okay in general, some minor nits below:

> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index ac0815d..a5718d1 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -80,3 +80,4 @@ CONFIG_VERSATILE_PCI=y
>  CONFIG_VERSATILE_I2C=y
>  
>  CONFIG_SDHCI=y
> +CONFIG_INTEGRATOR_DBG=y
> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> index 2ef93ed..46dc615 100644
> --- a/hw/arm/integratorcp.c
> +++ b/hw/arm/integratorcp.c
> @@ -508,6 +508,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
>      icp_control_init(0xcb000000);
>      sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
>      sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
> +    sysbus_create_simple("integrator_dbg", 0x1a000000, 0);
>      sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
>      if (nd_table[0].used)
>          smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 2578e29..be284f3 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) += vmport.o
>  
>  # ARM devices
>  common-obj-$(CONFIG_PL310) += arm_l2x0.o
> +common-obj-$(CONFIG_INTEGRATOR_DBG) += arm_intdbg.o
>  
>  # PKUnity SoC devices
>  common-obj-$(CONFIG_PUV3) += puv3_pm.o
> diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c
> new file mode 100644
> index 0000000..b505d09
> --- /dev/null
> +++ b/hw/misc/arm_intdbg.c
> @@ -0,0 +1,90 @@
> +/*
> + * LED, Switch and Debug control registers for ARM Integrator Boards
> + *
> + * This currently is a stub for this functionality written with
> + * reference to what the Linux kernel looks at. Previously we relied
> + * on the behaviour of unassigned_mem_read() in the core.
> + *
> + * The real h/w is described at:
> + *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
> + *
> + * Written by Alex Bennée
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "exec/address-spaces.h"
> +
> +#define TYPE_ARM_INTDBG "integrator_dbg"

If you move this constant into an include/hw/misc/arm_integratorcp_dbg.h
then you can reuse it in hw/arm/integratorcp.c above. (Optional.)

> +#define ARM_INTDBG(obj) \
> +    OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG)
> +
> +typedef struct {
> +    SysBusDevice parent_obj;

Please leave an empty line here to visually separate the parent field.

> +    MemoryRegion iomem;
> +
> +    uint32_t alpha;
> +    uint32_t leds;
> +    uint32_t switches;
> +} ARMIntDbgState;
> +
> +static uint64_t dbg_control_read(void *opaque, hwaddr offset,
> +                                 unsigned size)
> +{
> +    switch (offset >> 2) {
> +    case 0: /* ALPHA */
> +    case 1: /* LEDS */
> +    case 2: /* SWITCHES */
> +        qemu_log_mask(LOG_UNIMP, "dbg_control_read: returning zero from %x:%d\n", (int)offset, size);

HWADDR_PRIx, %u

Also suggest %s and __func__, cf. further below.

> +        return 0;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %x\n", (int)offset);

HWADDR_PRIx

> +        return 0;
> +    }
> +}
> +
> +static void dbg_control_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    switch (offset >> 2) {
> +    case 1: /* ALPHA */
> +    case 2: /* LEDS */
> +    case 3: /* SWITCHES */
> +        /* Nothing interesting implemented yet.  */
> +        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset);
> +    }
> +}
> +
> +static const MemoryRegionOps dbg_control_ops = {
> +    .read = dbg_control_read,
> +    .write = dbg_control_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void dbg_control_init(Object *obj)
> +{
> +    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
> +    ARMIntDbgState *s = ARM_INTDBG(obj);

Please leave an empty line after the variable declaration block.

> +    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000000);

"dbg-leds"?

> +    sysbus_init_mmio(sd, &s->iomem);
> +}
> +
> +static const TypeInfo arm_intdbg_info = {
> +    .name          = TYPE_ARM_INTDBG,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(ARMIntDbgState),
> +    .instance_init = dbg_control_init,
> +};
> +
> +static void arm_intdbg_register_types(void)
> +{
> +    type_register_static(&arm_intdbg_info);
> +}
> +
> +type_init(arm_intdbg_register_types)

The naming is a bit unfortunate and inconsistent. For one, QOM types
should use dashes and at least the in-code constants/macros are
requested to have a verbose, readable name. Could you expand INTDBG to
INTEGRATOR_DBG or INTEGRATOR_DEBUG like you have done for the Makefile?
Also please use arm_intdbg_ prefix (or whatever you choose) consistently
throughout the file (the idea is to have "unique" names that allow to
associate __func__ debug output with its origin). Having "integrator" or
"integratorcp" in the file name would also clarify that it's not for
"interrupt" or "internal". ;) Similar for ARMIntDbgState -
"ARMIntegratorDbgState"?

Cheers,
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 v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Alex Bennée]

afaerber@suse.de writes:

> Am 18.09.2013 16:31, schrieb alex.bennee@linaro.org:
>
> Looks okay in general, some minor nits below:

Thanks, I shall hopefully get those sorted out in the next week.

-- 
Alex Bennée