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

[Peter Maydell]

On 13 September 2013 15:58,  <address@hidden> wrote:
> From: Alex Bennée <address@hidden>
>
> 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