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 address@hidden:
> 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 so for now it simply returns 0's as
> the old unassigned_mem_read did.
> 
> Signed-off-by: Alex Bennée <address@hidden>
> ---
>  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