[Qemu-devel] [PATCH v3 0/3] Add the generic ARM timer

[Qemu-devel] [PATCH v3 0/3] Add the generic ARM timer

[Alistair Francis]

These three patches and and connect the Generic ARM Timer. This includes
support for dropping insecure writes and includes the ReadBase memory
map.

V3:
 - Add the ReadBase memory map
 - Update the names to match the ARM ARM
V2:
 - Fix couter/counter typo


Alistair Francis (3):
  arm_generic_timer: Add the ARM Generic Timer
  arm_generic_timer: Add support for the ReadBase memory map
  xlnx-zynqmp: Connect the ARM Generic Timer

 hw/arm/xlnx-zynqmp.c                 |  14 +++
 hw/timer/Makefile.objs               |   1 +
 hw/timer/arm_generic_timer.c         | 232 +++++++++++++++++++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h         |   2 +
 include/hw/timer/arm_generic_timer.h |  74 +++++++++++
 5 files changed, 323 insertions(+)
 create mode 100644 hw/timer/arm_generic_timer.c
 create mode 100644 include/hw/timer/arm_generic_timer.h

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer

[Alistair Francis]

Add the ARM generic timer. This allows the guest to poll the timer for
values and also supports secure writes only.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V3:
 - Use ARM ARM names
 - Indicate that we don't support all of the registers
 - Fixup the Makefile CONFIG
V2:
 - Fix couter/counter typo

 hw/timer/Makefile.objs               |   1 +
 hw/timer/arm_generic_timer.c         | 205 +++++++++++++++++++++++++++++++++++
 include/hw/timer/arm_generic_timer.h |  62 +++++++++++
 3 files changed, 268 insertions(+)
 create mode 100644 hw/timer/arm_generic_timer.c
 create mode 100644 include/hw/timer/arm_generic_timer.h

diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 7ba8c23..bb203e2 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
 common-obj-$(CONFIG_IMX) += imx_gpt.o
 common-obj-$(CONFIG_LM32) += lm32_timer.o
 common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
+common-obj-$(CONFIG_ARM_TIMER) += arm_generic_timer.o
 
 obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
 obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
diff --git a/hw/timer/arm_generic_timer.c b/hw/timer/arm_generic_timer.c
new file mode 100644
index 0000000..da434a7
--- /dev/null
+++ b/hw/timer/arm_generic_timer.c
@@ -0,0 +1,205 @@
+/*
+ * QEMU model of the ARM Generic Timer
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Written by Alistair Francis <alistair.francis@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/timer/arm_generic_timer.h"
+#include "qemu/timer.h"
+#include "qemu/log.h"
+
+#ifndef ARM_GEN_TIMER_ERR_DEBUG
+#define ARM_GEN_TIMER_ERR_DEBUG 0
+#endif
+
+static void counter_control_postw(RegisterInfo *reg, uint64_t val64)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
+    bool new_status = extract32(s->regs[R_CNTCR],
+                                R_CNTCR_EN_SHIFT,
+                                R_CNTCR_EN_LENGTH);
+    uint64_t current_ticks;
+
+    current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                             NANOSECONDS_PER_SECOND, 1000000);
+
+    if ((s->enabled && !new_status) ||
+        (!s->enabled && new_status)) {
+        /* The timer is being disabled or enabled */
+        s->tick_offset = current_ticks - s->tick_offset;
+    }
+
+    s->enabled = new_status;
+}
+
+static uint64_t counter_value_postr(RegisterInfo *reg)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
+    uint64_t current_ticks, total_ticks;
+
+    if (s->enabled) {
+        current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                                 NANOSECONDS_PER_SECOND, 1000000);
+        total_ticks = current_ticks - s->tick_offset;
+    } else {
+        /* Timer is disabled, return the time when it was disabled */
+        total_ticks = s->tick_offset;
+    }
+
+    return total_ticks;
+}
+
+static uint64_t counter_low_value_postr(RegisterInfo *reg, uint64_t val64)
+{
+    return (uint32_t) counter_value_postr(reg);
+}
+
+static uint64_t counter_high_value_postr(RegisterInfo *reg, uint64_t val64)
+{
+    return (uint32_t) (counter_value_postr(reg) >> 32);
+}
+
+static RegisterAccessInfo arm_gen_timer_regs_info[] = {
+    {   .name = "CNTCR",
+        .addr = A_CNTCR,
+        .rsvd = 0xfffffffc,
+        .post_write = counter_control_postw,
+    },{ .name = "CNTSR",
+        .addr = A_CNTSR,
+        .rsvd = 0xfffffffd, .ro = 0x2,
+    },{ .name = "CNTCV_LOWER",
+        .addr = A_CNTCV_LOWER,
+        .post_read = counter_low_value_postr,
+    },{ .name = "CNTCV_UPPER",
+        .addr = A_CNTCV_UPPER,
+        .post_read = counter_high_value_postr,
+    },{ .name = "CNTFID0",
+        .addr = A_CNTFID0,
+    }
+    /* We don't model CNTFIDn */
+    /* We don't model the CounterID registers either */
+};
+
+static void arm_gen_timer_reset(DeviceState *dev)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(dev);
+    unsigned int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->regs_info); ++i) {
+        register_reset(&s->regs_info[i]);
+    }
+
+    s->tick_offset = 0;
+    s->enabled = false;
+}
+
+static MemTxResult arm_gen_timer_read(void *opaque,  hwaddr addr,
+                                      uint64_t *data, unsigned size,
+                                      MemTxAttrs attrs)
+{
+    /* Reads are always supported, just blindly pass them through */
+    *data = register_read_memory(opaque, addr, size);
+
+    return MEMTX_OK;
+}
+
+static MemTxResult arm_gen_timer_write(void *opaque, hwaddr addr,
+                                       uint64_t data, unsigned size,
+                                       MemTxAttrs attrs)
+{
+    /* Block insecure writes */
+    if (!attrs.secure) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Non secure writes to the system timestamp generator " \
+                      "are invalid\n");
+        return MEMTX_ERROR;
+    }
+
+    register_write_memory(opaque, addr, data, size);
+
+    return MEMTX_OK;
+}
+
+static const MemoryRegionOps arm_gen_timer_ops = {
+    .read_with_attrs = arm_gen_timer_read,
+    .write_with_attrs = arm_gen_timer_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static const VMStateDescription vmstate_arm_gen_timer = {
+    .name = TYPE_ARM_GEN_TIMER,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, ARMGenTimer, R_ARM_GEN_TIMER_MAX),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
+static void arm_gen_timer_init(Object *obj)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    RegisterInfoArray *reg_array;
+
+    memory_region_init_io(&s->iomem, obj, &arm_gen_timer_ops, s,
+                          TYPE_ARM_GEN_TIMER, R_ARM_GEN_TIMER_MAX * 4);
+    reg_array =
+        register_init_block32(DEVICE(obj), arm_gen_timer_regs_info,
+                              ARRAY_SIZE(arm_gen_timer_regs_info),
+                              s->regs_info, s->regs,
+                              &arm_gen_timer_ops,
+                              ARM_GEN_TIMER_ERR_DEBUG,
+                              R_ARM_GEN_TIMER_MAX * 4);
+    memory_region_add_subregion(&s->iomem,
+                                A_CNTCR,
+                                &reg_array->mem);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void arm_gen_timer_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = arm_gen_timer_reset;
+    dc->vmsd = &vmstate_arm_gen_timer;
+}
+
+static const TypeInfo arm_gen_timer_info = {
+    .name          = TYPE_ARM_GEN_TIMER,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMGenTimer),
+    .class_init    = arm_gen_timer_class_init,
+    .instance_init = arm_gen_timer_init,
+};
+
+static void arm_gen_timer_register_types(void)
+{
+    type_register_static(&arm_gen_timer_info);
+}
+
+type_init(arm_gen_timer_register_types)
diff --git a/include/hw/timer/arm_generic_timer.h b/include/hw/timer/arm_generic_timer.h
new file mode 100644
index 0000000..ae4319c
--- /dev/null
+++ b/include/hw/timer/arm_generic_timer.h
@@ -0,0 +1,62 @@
+/*
+ * QEMU model of the ARM Generic Timer
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Written by Alistair Francis <alistair.francis@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef ARM_GEN_TIMER_H
+#define ARM_GEN_TIMER_H
+
+#include "hw/sysbus.h"
+#include "hw/register.h"
+
+#define TYPE_ARM_GEN_TIMER "arm.generic-timer"
+#define ARM_GEN_TIMER(obj) \
+            OBJECT_CHECK(ARMGenTimer, (obj), TYPE_ARM_GEN_TIMER)
+
+REG32(CNTCR, 0x00)
+    FIELD(CNTCR, EN, 0, 1)
+    FIELD(CNTCR, HDBG, 1, 1)
+REG32(CNTSR, 0x04)
+    FIELD(CNTSR, DBGH, 1, 1)
+REG32(CNTCV_LOWER, 0x08)
+REG32(CNTCV_UPPER, 0x0C)
+REG32(CNTFID0, 0x20)
+/* We don't model CNTFIDn */
+/* We don't model the CounterID registers either */
+
+#define R_ARM_GEN_TIMER_MAX (R_CNTFID0 + 1)
+
+typedef struct ARMGenTimer {
+    /* <private> */
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    /* <public> */
+    bool enabled;
+    uint64_t tick_offset;
+
+    uint32_t regs[R_ARM_GEN_TIMER_MAX];
+    RegisterInfo regs_info[R_ARM_GEN_TIMER_MAX];
+} ARMGenTimer;
+
+#endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer

[Peter Maydell]

On 20 December 2016 at 22:42, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Add the ARM generic timer. This allows the guest to poll the timer for
> values and also supports secure writes only.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V3:
>  - Use ARM ARM names
>  - Indicate that we don't support all of the registers
>  - Fixup the Makefile CONFIG
> V2:
>  - Fix couter/counter typo
>
>  hw/timer/Makefile.objs               |   1 +
>  hw/timer/arm_generic_timer.c         | 205 +++++++++++++++++++++++++++++++++++
>  include/hw/timer/arm_generic_timer.h |  62 +++++++++++
>  3 files changed, 268 insertions(+)
>  create mode 100644 hw/timer/arm_generic_timer.c
>  create mode 100644 include/hw/timer/arm_generic_timer.h
>
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 7ba8c23..bb203e2 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
>  common-obj-$(CONFIG_IMX) += imx_gpt.o
>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> +common-obj-$(CONFIG_ARM_TIMER) += arm_generic_timer.o
>
>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/arm_generic_timer.c b/hw/timer/arm_generic_timer.c
> new file mode 100644
> index 0000000..da434a7
> --- /dev/null
> +++ b/hw/timer/arm_generic_timer.c
> @@ -0,0 +1,205 @@
> +/*
> + * QEMU model of the ARM Generic Timer
> + *
> + * Copyright (c) 2016 Xilinx Inc.
> + * Written by Alistair Francis <alistair.francis@xilinx.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/timer/arm_generic_timer.h"
> +#include "qemu/timer.h"
> +#include "qemu/log.h"
> +
> +#ifndef ARM_GEN_TIMER_ERR_DEBUG
> +#define ARM_GEN_TIMER_ERR_DEBUG 0
> +#endif
> +
> +static void counter_control_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
> +    bool new_status = extract32(s->regs[R_CNTCR],
> +                                R_CNTCR_EN_SHIFT,
> +                                R_CNTCR_EN_LENGTH);
> +    uint64_t current_ticks;
> +
> +    current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> +                             NANOSECONDS_PER_SECOND, 1000000);
> +
> +    if ((s->enabled && !new_status) ||
> +        (!s->enabled && new_status)) {

Since s->enabled and new_status are both bool, you can
write this as "if (s->enabled != new_status)".
(If they were ints you could use xor.)

> +        /* The timer is being disabled or enabled */
> +        s->tick_offset = current_ticks - s->tick_offset;
> +    }
> +
> +    s->enabled = new_status;
> +}
> +
> +static uint64_t counter_value_postr(RegisterInfo *reg)
> +{
> +    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
> +    uint64_t current_ticks, total_ticks;
> +
> +    if (s->enabled) {
> +        current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> +                                 NANOSECONDS_PER_SECOND, 1000000);
> +        total_ticks = current_ticks - s->tick_offset;
> +    } else {
> +        /* Timer is disabled, return the time when it was disabled */
> +        total_ticks = s->tick_offset;
> +    }
> +
> +    return total_ticks;
> +}
> +
> +static uint64_t counter_low_value_postr(RegisterInfo *reg, uint64_t val64)
> +{
> +    return (uint32_t) counter_value_postr(reg);
> +}
> +
> +static uint64_t counter_high_value_postr(RegisterInfo *reg, uint64_t val64)
> +{
> +    return (uint32_t) (counter_value_postr(reg) >> 32);
> +}

The spec says that a write to the CNTCV should cause changes
to each CPU's CNTPCT/CNTPCT_EL0 register -- how do we plan to implement this?

(v8 ARM ARM section D6.1 is the clearest description of the
relationship between the system counter and the per-CPU timers.)

> +
> +static RegisterAccessInfo arm_gen_timer_regs_info[] = {
> +    {   .name = "CNTCR",
> +        .addr = A_CNTCR,
> +        .rsvd = 0xfffffffc,

Spec says bits [17:8] are FCREQ field ?

> +        .post_write = counter_control_postw,
> +    },{ .name = "CNTSR",
> +        .addr = A_CNTSR,
> +        .rsvd = 0xfffffffd, .ro = 0x2,

Spec says bits [31:8] are FCACK ?

> +    },{ .name = "CNTCV_LOWER",
> +        .addr = A_CNTCV_LOWER,
> +        .post_read = counter_low_value_postr,
> +    },{ .name = "CNTCV_UPPER",
> +        .addr = A_CNTCV_UPPER,
> +        .post_read = counter_high_value_postr,

Spec says that you should also be able to access the whole
64-bit counter value with a 64-bit access -- is this reginfo
sufficient to make that work?

> +    },{ .name = "CNTFID0",
> +        .addr = A_CNTFID0,
> +    }
> +    /* We don't model CNTFIDn */
> +    /* We don't model the CounterID registers either */
> +};
> +
> +static void arm_gen_timer_reset(DeviceState *dev)
> +{
> +    ARMGenTimer *s = ARM_GEN_TIMER(dev);
> +    unsigned int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(s->regs_info); ++i) {
> +        register_reset(&s->regs_info[i]);
> +    }
> +
> +    s->tick_offset = 0;
> +    s->enabled = false;
> +}
> +
> +static MemTxResult arm_gen_timer_read(void *opaque,  hwaddr addr,
> +                                      uint64_t *data, unsigned size,
> +                                      MemTxAttrs attrs)
> +{
> +    /* Reads are always supported, just blindly pass them through */
> +    *data = register_read_memory(opaque, addr, size);
> +
> +    return MEMTX_OK;
> +}
> +
> +static MemTxResult arm_gen_timer_write(void *opaque, hwaddr addr,
> +                                       uint64_t data, unsigned size,
> +                                       MemTxAttrs attrs)
> +{
> +    /* Block insecure writes */
> +    if (!attrs.secure) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Non secure writes to the system timestamp generator " \
> +                      "are invalid\n");
> +        return MEMTX_ERROR;
> +    }

This means you can't use this device in a system which
doesn't implement TrustZone. I think this would be better
handled by just having the board map the memory region
in to only the Secure address space if it is a TZ-aware board.

This also gets handling of NS reads correct (your code
allows them).

> +
> +    register_write_memory(opaque, addr, data, size);
> +
> +    return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps arm_gen_timer_ops = {
> +    .read_with_attrs = arm_gen_timer_read,
> +    .write_with_attrs = arm_gen_timer_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,

64-bit access is needed for the CNTCV.

> +    },
> +};
> +
> +static const VMStateDescription vmstate_arm_gen_timer = {
> +    .name = TYPE_ARM_GEN_TIMER,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, ARMGenTimer, R_ARM_GEN_TIMER_MAX),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
> +static void arm_gen_timer_init(Object *obj)
> +{
> +    ARMGenTimer *s = ARM_GEN_TIMER(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    RegisterInfoArray *reg_array;
> +
> +    memory_region_init_io(&s->iomem, obj, &arm_gen_timer_ops, s,
> +                          TYPE_ARM_GEN_TIMER, R_ARM_GEN_TIMER_MAX * 4);
> +    reg_array =
> +        register_init_block32(DEVICE(obj), arm_gen_timer_regs_info,
> +                              ARRAY_SIZE(arm_gen_timer_regs_info),
> +                              s->regs_info, s->regs,
> +                              &arm_gen_timer_ops,
> +                              ARM_GEN_TIMER_ERR_DEBUG,
> +                              R_ARM_GEN_TIMER_MAX * 4);
> +    memory_region_add_subregion(&s->iomem,
> +                                A_CNTCR,
> +                                &reg_array->mem);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static void arm_gen_timer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = arm_gen_timer_reset;
> +    dc->vmsd = &vmstate_arm_gen_timer;
> +}
> +
> +static const TypeInfo arm_gen_timer_info = {
> +    .name          = TYPE_ARM_GEN_TIMER,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(ARMGenTimer),
> +    .class_init    = arm_gen_timer_class_init,
> +    .instance_init = arm_gen_timer_init,
> +};
> +
> +static void arm_gen_timer_register_types(void)
> +{
> +    type_register_static(&arm_gen_timer_info);
> +}
> +
> +type_init(arm_gen_timer_register_types)
> diff --git a/include/hw/timer/arm_generic_timer.h b/include/hw/timer/arm_generic_timer.h
> new file mode 100644
> index 0000000..ae4319c
> --- /dev/null
> +++ b/include/hw/timer/arm_generic_timer.h
> @@ -0,0 +1,62 @@
> +/*
> + * QEMU model of the ARM Generic Timer
> + *
> + * Copyright (c) 2016 Xilinx Inc.
> + * Written by Alistair Francis <alistair.francis@xilinx.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef ARM_GEN_TIMER_H
> +#define ARM_GEN_TIMER_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/register.h"
> +
> +#define TYPE_ARM_GEN_TIMER "arm.generic-timer"
> +#define ARM_GEN_TIMER(obj) \
> +            OBJECT_CHECK(ARMGenTimer, (obj), TYPE_ARM_GEN_TIMER)
> +
> +REG32(CNTCR, 0x00)
> +    FIELD(CNTCR, EN, 0, 1)
> +    FIELD(CNTCR, HDBG, 1, 1)
> +REG32(CNTSR, 0x04)
> +    FIELD(CNTSR, DBGH, 1, 1)
> +REG32(CNTCV_LOWER, 0x08)
> +REG32(CNTCV_UPPER, 0x0C)
> +REG32(CNTFID0, 0x20)
> +/* We don't model CNTFIDn */
> +/* We don't model the CounterID registers either */

Does the Xilinx h/w not implement them at all?
(ie do we need a property for "device has the ID regs" if we add them
later?)

The spec says it's mandatory to have at least the entry for
the counter base frequency plus end-of-table marker.
I would expect EL3 firmware to want to read this table in order
to write the correct values to CNTFRQ_EL0 for each CPU.

> +
> +#define R_ARM_GEN_TIMER_MAX (R_CNTFID0 + 1)
> +
> +typedef struct ARMGenTimer {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +
> +    /* <public> */
> +    bool enabled;
> +    uint64_t tick_offset;
> +
> +    uint32_t regs[R_ARM_GEN_TIMER_MAX];
> +    RegisterInfo regs_info[R_ARM_GEN_TIMER_MAX];
> +} ARMGenTimer;
> +
> +#endif
> --
> 2.7.4
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer

[Alistair Francis]

On Fri, Jan 6, 2017 at 3:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 December 2016 at 22:42, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Add the ARM generic timer. This allows the guest to poll the timer for
>> values and also supports secure writes only.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V3:
>>  - Use ARM ARM names
>>  - Indicate that we don't support all of the registers
>>  - Fixup the Makefile CONFIG
>> V2:
>>  - Fix couter/counter typo
>>
>>  hw/timer/Makefile.objs               |   1 +
>>  hw/timer/arm_generic_timer.c         | 205 +++++++++++++++++++++++++++++++++++
>>  include/hw/timer/arm_generic_timer.h |  62 +++++++++++
>>  3 files changed, 268 insertions(+)
>>  create mode 100644 hw/timer/arm_generic_timer.c
>>  create mode 100644 include/hw/timer/arm_generic_timer.h
>>
>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>> index 7ba8c23..bb203e2 100644
>> --- a/hw/timer/Makefile.objs
>> +++ b/hw/timer/Makefile.objs
>> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
>>  common-obj-$(CONFIG_IMX) += imx_gpt.o
>>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
>> +common-obj-$(CONFIG_ARM_TIMER) += arm_generic_timer.o
>>
>>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
>> diff --git a/hw/timer/arm_generic_timer.c b/hw/timer/arm_generic_timer.c
>> new file mode 100644
>> index 0000000..da434a7
>> --- /dev/null
>> +++ b/hw/timer/arm_generic_timer.c
>> @@ -0,0 +1,205 @@
>> +/*
>> + * QEMU model of the ARM Generic Timer
>> + *
>> + * Copyright (c) 2016 Xilinx Inc.
>> + * Written by Alistair Francis <alistair.francis@xilinx.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/timer/arm_generic_timer.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/log.h"
>> +
>> +#ifndef ARM_GEN_TIMER_ERR_DEBUG
>> +#define ARM_GEN_TIMER_ERR_DEBUG 0
>> +#endif
>> +
>> +static void counter_control_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
>> +    bool new_status = extract32(s->regs[R_CNTCR],
>> +                                R_CNTCR_EN_SHIFT,
>> +                                R_CNTCR_EN_LENGTH);
>> +    uint64_t current_ticks;
>> +
>> +    current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>> +                             NANOSECONDS_PER_SECOND, 1000000);
>> +
>> +    if ((s->enabled && !new_status) ||
>> +        (!s->enabled && new_status)) {
>
> Since s->enabled and new_status are both bool, you can
> write this as "if (s->enabled != new_status)".
> (If they were ints you could use xor.)

I should have realised that, thanks.

>
>> +        /* The timer is being disabled or enabled */
>> +        s->tick_offset = current_ticks - s->tick_offset;
>> +    }
>> +
>> +    s->enabled = new_status;
>> +}
>> +
>> +static uint64_t counter_value_postr(RegisterInfo *reg)
>> +{
>> +    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
>> +    uint64_t current_ticks, total_ticks;
>> +
>> +    if (s->enabled) {
>> +        current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>> +                                 NANOSECONDS_PER_SECOND, 1000000);
>> +        total_ticks = current_ticks - s->tick_offset;
>> +    } else {
>> +        /* Timer is disabled, return the time when it was disabled */
>> +        total_ticks = s->tick_offset;
>> +    }
>> +
>> +    return total_ticks;
>> +}
>> +
>> +static uint64_t counter_low_value_postr(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    return (uint32_t) counter_value_postr(reg);
>> +}
>> +
>> +static uint64_t counter_high_value_postr(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    return (uint32_t) (counter_value_postr(reg) >> 32);
>> +}
>
> The spec says that a write to the CNTCV should cause changes
> to each CPU's CNTPCT/CNTPCT_EL0 register -- how do we plan to implement this?

Hmm... This I don't know. What do you think?

>
> (v8 ARM ARM section D6.1 is the clearest description of the
> relationship between the system counter and the per-CPU timers.)
>
>> +
>> +static RegisterAccessInfo arm_gen_timer_regs_info[] = {
>> +    {   .name = "CNTCR",
>> +        .addr = A_CNTCR,
>> +        .rsvd = 0xfffffffc,
>
> Spec says bits [17:8] are FCREQ field ?

Yeah, you are right. That was left over from the Xilinx spec.

>
>> +        .post_write = counter_control_postw,
>> +    },{ .name = "CNTSR",
>> +        .addr = A_CNTSR,
>> +        .rsvd = 0xfffffffd, .ro = 0x2,
>
> Spec says bits [31:8] are FCACK ?

Same

>
>> +    },{ .name = "CNTCV_LOWER",
>> +        .addr = A_CNTCV_LOWER,
>> +        .post_read = counter_low_value_postr,
>> +    },{ .name = "CNTCV_UPPER",
>> +        .addr = A_CNTCV_UPPER,
>> +        .post_read = counter_high_value_postr,
>
> Spec says that you should also be able to access the whole
> 64-bit counter value with a 64-bit access -- is this reginfo
> sufficient to make that work?

How do you know, all I see if that it is a 64-bit register. All the
documentation about accesses specifies only 32-bit accesses.

I have to make some changes to support 64-bit but it should be pretty
easy to do.

>
>> +    },{ .name = "CNTFID0",
>> +        .addr = A_CNTFID0,
>> +    }
>> +    /* We don't model CNTFIDn */
>> +    /* We don't model the CounterID registers either */
>> +};
>> +
>> +static void arm_gen_timer_reset(DeviceState *dev)
>> +{
>> +    ARMGenTimer *s = ARM_GEN_TIMER(dev);
>> +    unsigned int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(s->regs_info); ++i) {
>> +        register_reset(&s->regs_info[i]);
>> +    }
>> +
>> +    s->tick_offset = 0;
>> +    s->enabled = false;
>> +}
>> +
>> +static MemTxResult arm_gen_timer_read(void *opaque,  hwaddr addr,
>> +                                      uint64_t *data, unsigned size,
>> +                                      MemTxAttrs attrs)
>> +{
>> +    /* Reads are always supported, just blindly pass them through */
>> +    *data = register_read_memory(opaque, addr, size);
>> +
>> +    return MEMTX_OK;
>> +}
>> +
>> +static MemTxResult arm_gen_timer_write(void *opaque, hwaddr addr,
>> +                                       uint64_t data, unsigned size,
>> +                                       MemTxAttrs attrs)
>> +{
>> +    /* Block insecure writes */
>> +    if (!attrs.secure) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "Non secure writes to the system timestamp generator " \
>> +                      "are invalid\n");
>> +        return MEMTX_ERROR;
>> +    }
>
> This means you can't use this device in a system which
> doesn't implement TrustZone. I think this would be better
> handled by just having the board map the memory region
> in to only the Secure address space if it is a TZ-aware board.

How can I map something into the secure address space? Is there an
example of this? The only think I can find is in the arm/virt.c
machine with the secure_sysmem MemoryRegion.

>
> This also gets handling of NS reads correct (your code
> allows them).
>
>> +
>> +    register_write_memory(opaque, addr, data, size);
>> +
>> +    return MEMTX_OK;
>> +}
>> +
>> +static const MemoryRegionOps arm_gen_timer_ops = {
>> +    .read_with_attrs = arm_gen_timer_read,
>> +    .write_with_attrs = arm_gen_timer_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>
> 64-bit access is needed for the CNTCV.

Ok, will fix.

>
>> +    },
>> +};
>> +
>> +static const VMStateDescription vmstate_arm_gen_timer = {
>> +    .name = TYPE_ARM_GEN_TIMER,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(regs, ARMGenTimer, R_ARM_GEN_TIMER_MAX),
>> +        VMSTATE_END_OF_LIST(),
>> +    }
>> +};
>> +
>> +static void arm_gen_timer_init(Object *obj)
>> +{
>> +    ARMGenTimer *s = ARM_GEN_TIMER(obj);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    RegisterInfoArray *reg_array;
>> +
>> +    memory_region_init_io(&s->iomem, obj, &arm_gen_timer_ops, s,
>> +                          TYPE_ARM_GEN_TIMER, R_ARM_GEN_TIMER_MAX * 4);
>> +    reg_array =
>> +        register_init_block32(DEVICE(obj), arm_gen_timer_regs_info,
>> +                              ARRAY_SIZE(arm_gen_timer_regs_info),
>> +                              s->regs_info, s->regs,
>> +                              &arm_gen_timer_ops,
>> +                              ARM_GEN_TIMER_ERR_DEBUG,
>> +                              R_ARM_GEN_TIMER_MAX * 4);
>> +    memory_region_add_subregion(&s->iomem,
>> +                                A_CNTCR,
>> +                                &reg_array->mem);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +}
>> +
>> +static void arm_gen_timer_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->reset = arm_gen_timer_reset;
>> +    dc->vmsd = &vmstate_arm_gen_timer;
>> +}
>> +
>> +static const TypeInfo arm_gen_timer_info = {
>> +    .name          = TYPE_ARM_GEN_TIMER,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(ARMGenTimer),
>> +    .class_init    = arm_gen_timer_class_init,
>> +    .instance_init = arm_gen_timer_init,
>> +};
>> +
>> +static void arm_gen_timer_register_types(void)
>> +{
>> +    type_register_static(&arm_gen_timer_info);
>> +}
>> +
>> +type_init(arm_gen_timer_register_types)
>> diff --git a/include/hw/timer/arm_generic_timer.h b/include/hw/timer/arm_generic_timer.h
>> new file mode 100644
>> index 0000000..ae4319c
>> --- /dev/null
>> +++ b/include/hw/timer/arm_generic_timer.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + * QEMU model of the ARM Generic Timer
>> + *
>> + * Copyright (c) 2016 Xilinx Inc.
>> + * Written by Alistair Francis <alistair.francis@xilinx.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#ifndef ARM_GEN_TIMER_H
>> +#define ARM_GEN_TIMER_H
>> +
>> +#include "hw/sysbus.h"
>> +#include "hw/register.h"
>> +
>> +#define TYPE_ARM_GEN_TIMER "arm.generic-timer"
>> +#define ARM_GEN_TIMER(obj) \
>> +            OBJECT_CHECK(ARMGenTimer, (obj), TYPE_ARM_GEN_TIMER)
>> +
>> +REG32(CNTCR, 0x00)
>> +    FIELD(CNTCR, EN, 0, 1)
>> +    FIELD(CNTCR, HDBG, 1, 1)
>> +REG32(CNTSR, 0x04)
>> +    FIELD(CNTSR, DBGH, 1, 1)
>> +REG32(CNTCV_LOWER, 0x08)
>> +REG32(CNTCV_UPPER, 0x0C)
>> +REG32(CNTFID0, 0x20)
>> +/* We don't model CNTFIDn */
>> +/* We don't model the CounterID registers either */
>
> Does the Xilinx h/w not implement them at all?
> (ie do we need a property for "device has the ID regs" if we add them
> later?)

There is nothing in the register spec describing registers being here.

The last register I see is called: (IOU_SCNTRS)
base_frequency_ID_register at address 0xFF260020.

>
> The spec says it's mandatory to have at least the entry for
> the counter base frequency plus end-of-table marker.
> I would expect EL3 firmware to want to read this table in order
> to write the correct values to CNTFRQ_EL0 for each CPU.

I don't see anything like that in section I3.5.21 in the ARM ARM,
where are you looking?

Thanks,

Alistair

>
>> +
>> +#define R_ARM_GEN_TIMER_MAX (R_CNTFID0 + 1)
>> +
>> +typedef struct ARMGenTimer {
>> +    /* <private> */
>> +    SysBusDevice parent_obj;
>> +    MemoryRegion iomem;
>> +
>> +    /* <public> */
>> +    bool enabled;
>> +    uint64_t tick_offset;
>> +
>> +    uint32_t regs[R_ARM_GEN_TIMER_MAX];
>> +    RegisterInfo regs_info[R_ARM_GEN_TIMER_MAX];
>> +} ARMGenTimer;
>> +
>> +#endif
>> --
>> 2.7.4
>>
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer

[Peter Maydell]

On 10 January 2017 at 01:41, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, Jan 6, 2017 at 3:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 20 December 2016 at 22:42, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:

>>> +    },{ .name = "CNTCV_LOWER",
>>> +        .addr = A_CNTCV_LOWER,
>>> +        .post_read = counter_low_value_postr,
>>> +    },{ .name = "CNTCV_UPPER",
>>> +        .addr = A_CNTCV_UPPER,
>>> +        .post_read = counter_high_value_postr,
>>
>> Spec says that you should also be able to access the whole
>> 64-bit counter value with a 64-bit access -- is this reginfo
>> sufficient to make that work?
>
> How do you know, all I see if that it is a 64-bit register. All the
> documentation about accesses specifies only 32-bit accesses.

v8 ARM ARM DDI0487A.k, I3.5.3 (CNTCV register description):
"In an implementation that supports 64-bit atomic memory accesses,
this register must be accessible using a 64-bit atomic access."


>> This means you can't use this device in a system which
>> doesn't implement TrustZone. I think this would be better
>> handled by just having the board map the memory region
>> in to only the Secure address space if it is a TZ-aware board.
>
> How can I map something into the secure address space? Is there an
> example of this? The only think I can find is in the arm/virt.c
> machine with the secure_sysmem MemoryRegion.

Yeah, virt.c is the place to look -- for instance we put one
of the UARTs in the secure address space only.
Basically to support separate address spaces:
 * create a MemoryRegion to be the secure view of the world
 * add the NS MemoryRegion to it as a subregion with low
   priority (so S sees all the NS devices -- this isn't
   required but that's usually how h/w is set up)
 * use object_property_set_link() to connect the S memory
   region to each CPU's secure-memory property
 * add any S-only devices to the S MemoryRegion

>>> +REG32(CNTCR, 0x00)
>>> +    FIELD(CNTCR, EN, 0, 1)
>>> +    FIELD(CNTCR, HDBG, 1, 1)
>>> +REG32(CNTSR, 0x04)
>>> +    FIELD(CNTSR, DBGH, 1, 1)
>>> +REG32(CNTCV_LOWER, 0x08)
>>> +REG32(CNTCV_UPPER, 0x0C)
>>> +REG32(CNTFID0, 0x20)
>>> +/* We don't model CNTFIDn */
>>> +/* We don't model the CounterID registers either */
>>
>> Does the Xilinx h/w not implement them at all?
>> (ie do we need a property for "device has the ID regs" if we add them
>> later?)
>
> There is nothing in the register spec describing registers being here.
>
> The last register I see is called: (IOU_SCNTRS)
> base_frequency_ID_register at address 0xFF260020.

I suspect there's an implicit reads-as-zero after that.
(The frequency table has to have at least one real entry
plus the zero terminator -- see I3.5.5 (CNTFID0).)

QEMU's implementation will only support one frequency so
we only need one table entry too (for the moment, anyway).

>> The spec says it's mandatory to have at least the entry for
>> the counter base frequency plus end-of-table marker.
>> I would expect EL3 firmware to want to read this table in order
>> to write the correct values to CNTFRQ_EL0 for each CPU.
>
> I don't see anything like that in section I3.5.21 in the ARM ARM,
> where are you looking?

There's no I3.5.21 in my copy of the spec (rev A.k), I suspect
you have an older rev. I think the generic timer docs got
revamped at some point, so you might want to grab the latest rev.

Anyway, it looks like you have got CNTFIDn (that's CNTFID0)
and an implicit undocumented zero-terminator.

The spec says CounterIDn are IMPDEF, which I guess is why
your implementation doesn't bother to implement them.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer

[Alistair Francis]

On Tue, Jan 10, 2017 at 1:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 January 2017 at 01:41, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Fri, Jan 6, 2017 at 3:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 20 December 2016 at 22:42, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>
>>>> +    },{ .name = "CNTCV_LOWER",
>>>> +        .addr = A_CNTCV_LOWER,
>>>> +        .post_read = counter_low_value_postr,
>>>> +    },{ .name = "CNTCV_UPPER",
>>>> +        .addr = A_CNTCV_UPPER,
>>>> +        .post_read = counter_high_value_postr,
>>>
>>> Spec says that you should also be able to access the whole
>>> 64-bit counter value with a 64-bit access -- is this reginfo
>>> sufficient to make that work?
>>
>> How do you know, all I see if that it is a 64-bit register. All the
>> documentation about accesses specifies only 32-bit accesses.
>
> v8 ARM ARM DDI0487A.k, I3.5.3 (CNTCV register description):
> "In an implementation that supports 64-bit atomic memory accesses,
> this register must be accessible using a 64-bit atomic access."

Ok, I don't even have that section. I'm going to try to find an updated ARM ARM.

>
>
>>> This means you can't use this device in a system which
>>> doesn't implement TrustZone. I think this would be better
>>> handled by just having the board map the memory region
>>> in to only the Secure address space if it is a TZ-aware board.
>>
>> How can I map something into the secure address space? Is there an
>> example of this? The only think I can find is in the arm/virt.c
>> machine with the secure_sysmem MemoryRegion.
>
> Yeah, virt.c is the place to look -- for instance we put one
> of the UARTs in the secure address space only.
> Basically to support separate address spaces:
>  * create a MemoryRegion to be the secure view of the world
>  * add the NS MemoryRegion to it as a subregion with low
>    priority (so S sees all the NS devices -- this isn't
>    required but that's usually how h/w is set up)
>  * use object_property_set_link() to connect the S memory
>    region to each CPU's secure-memory property
>  * add any S-only devices to the S MemoryRegion

Ok, sounds easy enough.

>
>>>> +REG32(CNTCR, 0x00)
>>>> +    FIELD(CNTCR, EN, 0, 1)
>>>> +    FIELD(CNTCR, HDBG, 1, 1)
>>>> +REG32(CNTSR, 0x04)
>>>> +    FIELD(CNTSR, DBGH, 1, 1)
>>>> +REG32(CNTCV_LOWER, 0x08)
>>>> +REG32(CNTCV_UPPER, 0x0C)
>>>> +REG32(CNTFID0, 0x20)
>>>> +/* We don't model CNTFIDn */
>>>> +/* We don't model the CounterID registers either */
>>>
>>> Does the Xilinx h/w not implement them at all?
>>> (ie do we need a property for "device has the ID regs" if we add them
>>> later?)
>>
>> There is nothing in the register spec describing registers being here.
>>
>> The last register I see is called: (IOU_SCNTRS)
>> base_frequency_ID_register at address 0xFF260020.
>
> I suspect there's an implicit reads-as-zero after that.
> (The frequency table has to have at least one real entry
> plus the zero terminator -- see I3.5.5 (CNTFID0).)
>
> QEMU's implementation will only support one frequency so
> we only need one table entry too (for the moment, anyway).
>
>>> The spec says it's mandatory to have at least the entry for
>>> the counter base frequency plus end-of-table marker.
>>> I would expect EL3 firmware to want to read this table in order
>>> to write the correct values to CNTFRQ_EL0 for each CPU.
>>
>> I don't see anything like that in section I3.5.21 in the ARM ARM,
>> where are you looking?
>
> There's no I3.5.21 in my copy of the spec (rev A.k), I suspect
> you have an older rev. I think the generic timer docs got
> revamped at some point, so you might want to grab the latest rev.
>
> Anyway, it looks like you have got CNTFIDn (that's CNTFID0)
> and an implicit undocumented zero-terminator.
>
> The spec says CounterIDn are IMPDEF, which I guess is why
> your implementation doesn't bother to implement them.

I do have an out of date version. I'll find a newer one and update the
registers to follow that.

Thanks,

Alistair

>
> thanks
> -- PMM
>

[Qemu-devel] [PATCH v3 2/3] arm_generic_timer: Add support for the ReadBase memory map

[Alistair Francis]

Add support for the read only regions in the ReadBase memory map.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/timer/arm_generic_timer.c         | 27 +++++++++++++++++++++++++++
 include/hw/timer/arm_generic_timer.h | 12 ++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/hw/timer/arm_generic_timer.c b/hw/timer/arm_generic_timer.c
index da434a7..5392657 100644
--- a/hw/timer/arm_generic_timer.c
+++ b/hw/timer/arm_generic_timer.c
@@ -100,6 +100,17 @@ static RegisterAccessInfo arm_gen_timer_regs_info[] = {
     /* We don't model the CounterID registers either */
 };
 
+static RegisterAccessInfo arm_gen_timer_read_regs_info[] = {
+    {   .name = "CNTCV_READ_LOWER",
+        .addr = A_CNTCV_READ_LOWER, .ro = 0xFFFF,
+        .post_read = counter_low_value_postr,
+    },{ .name = "CNTCV_READ_UPPER",
+        .addr = A_CNTCV_READ_UPPER, .ro = 0xFFFF,
+        .post_read = counter_high_value_postr,
+    }
+    /* We don't model the CounterID registers */
+};
+
 static void arm_gen_timer_reset(DeviceState *dev)
 {
     ARMGenTimer *s = ARM_GEN_TIMER(dev);
@@ -166,6 +177,7 @@ static void arm_gen_timer_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     RegisterInfoArray *reg_array;
 
+    /* Create the ControlBase memory region */
     memory_region_init_io(&s->iomem, obj, &arm_gen_timer_ops, s,
                           TYPE_ARM_GEN_TIMER, R_ARM_GEN_TIMER_MAX * 4);
     reg_array =
@@ -179,6 +191,21 @@ static void arm_gen_timer_init(Object *obj)
                                 A_CNTCR,
                                 &reg_array->mem);
     sysbus_init_mmio(sbd, &s->iomem);
+
+    /* Create the ReadBase memory region */
+    memory_region_init_io(&s->iomem_read, obj, &arm_gen_timer_ops, s,
+                          TYPE_ARM_GEN_TIMER "-read", R_ARM_GEN_TIMER_READ_MAX * 4);
+    reg_array =
+        register_init_block32(DEVICE(obj), arm_gen_timer_read_regs_info,
+                              ARRAY_SIZE(arm_gen_timer_read_regs_info),
+                              s->regs_read_info, s->regs_read,
+                              &arm_gen_timer_ops,
+                              ARM_GEN_TIMER_ERR_DEBUG,
+                              R_ARM_GEN_TIMER_READ_MAX * 4);
+    memory_region_add_subregion(&s->iomem_read,
+                                R_CNTCV_READ_LOWER,
+                                &reg_array->mem);
+    sysbus_init_mmio(sbd, &s->iomem_read);
 }
 
 static void arm_gen_timer_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/timer/arm_generic_timer.h b/include/hw/timer/arm_generic_timer.h
index ae4319c..462ee5b 100644
--- a/include/hw/timer/arm_generic_timer.h
+++ b/include/hw/timer/arm_generic_timer.h
@@ -33,6 +33,7 @@
 #define ARM_GEN_TIMER(obj) \
             OBJECT_CHECK(ARMGenTimer, (obj), TYPE_ARM_GEN_TIMER)
 
+/* ControlBase Memory Map */
 REG32(CNTCR, 0x00)
     FIELD(CNTCR, EN, 0, 1)
     FIELD(CNTCR, HDBG, 1, 1)
@@ -46,10 +47,18 @@ REG32(CNTFID0, 0x20)
 
 #define R_ARM_GEN_TIMER_MAX (R_CNTFID0 + 1)
 
+/* Read Base Memory Map */
+REG32(CNTCV_READ_LOWER, 0x00)
+REG32(CNTCV_READ_UPPER, 0x04)
+/* We don't model the CounterID registers */
+
+#define R_ARM_GEN_TIMER_READ_MAX (R_CNTCV_READ_UPPER + 1)
+
 typedef struct ARMGenTimer {
     /* <private> */
     SysBusDevice parent_obj;
     MemoryRegion iomem;
+    MemoryRegion iomem_read;
 
     /* <public> */
     bool enabled;
@@ -57,6 +66,9 @@ typedef struct ARMGenTimer {
 
     uint32_t regs[R_ARM_GEN_TIMER_MAX];
     RegisterInfo regs_info[R_ARM_GEN_TIMER_MAX];
+
+    uint32_t regs_read[R_ARM_GEN_TIMER_READ_MAX];
+    RegisterInfo regs_read_info[R_ARM_GEN_TIMER_READ_MAX];
 } ARMGenTimer;
 
 #endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 2/3] arm_generic_timer: Add support for the ReadBase memory map

[Peter Maydell]

On 20 December 2016 at 22:42, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Add support for the read only regions in the ReadBase memory map.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  hw/timer/arm_generic_timer.c         | 27 +++++++++++++++++++++++++++
>  include/hw/timer/arm_generic_timer.h | 12 ++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/hw/timer/arm_generic_timer.c b/hw/timer/arm_generic_timer.c
> index da434a7..5392657 100644
> --- a/hw/timer/arm_generic_timer.c
> +++ b/hw/timer/arm_generic_timer.c
> @@ -100,6 +100,17 @@ static RegisterAccessInfo arm_gen_timer_regs_info[] = {
>      /* We don't model the CounterID registers either */
>  };
>
> +static RegisterAccessInfo arm_gen_timer_read_regs_info[] = {
> +    {   .name = "CNTCV_READ_LOWER",
> +        .addr = A_CNTCV_READ_LOWER, .ro = 0xFFFF,
> +        .post_read = counter_low_value_postr,
> +    },{ .name = "CNTCV_READ_UPPER",
> +        .addr = A_CNTCV_READ_UPPER, .ro = 0xFFFF,
> +        .post_read = counter_high_value_postr,
> +    }

Needs to support 64-bit reads too.

> +    /* We don't model the CounterID registers */

(same query about ID regs as in patch 1)

> +};
> +
>  static void arm_gen_timer_reset(DeviceState *dev)
>  {
>      ARMGenTimer *s = ARM_GEN_TIMER(dev);
> @@ -166,6 +177,7 @@ static void arm_gen_timer_init(Object *obj)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      RegisterInfoArray *reg_array;
>
> +    /* Create the ControlBase memory region */
>      memory_region_init_io(&s->iomem, obj, &arm_gen_timer_ops, s,
>                            TYPE_ARM_GEN_TIMER, R_ARM_GEN_TIMER_MAX * 4);
>      reg_array =
> @@ -179,6 +191,21 @@ static void arm_gen_timer_init(Object *obj)
>                                  A_CNTCR,
>                                  &reg_array->mem);
>      sysbus_init_mmio(sbd, &s->iomem);
> +
> +    /* Create the ReadBase memory region */
> +    memory_region_init_io(&s->iomem_read, obj, &arm_gen_timer_ops, s,
> +                          TYPE_ARM_GEN_TIMER "-read", R_ARM_GEN_TIMER_READ_MAX * 4);

checkpatch.pl complains that this line needs wrapping.

> +    reg_array =
> +        register_init_block32(DEVICE(obj), arm_gen_timer_read_regs_info,
> +                              ARRAY_SIZE(arm_gen_timer_read_regs_info),
> +                              s->regs_read_info, s->regs_read,
> +                              &arm_gen_timer_ops,
> +                              ARM_GEN_TIMER_ERR_DEBUG,
> +                              R_ARM_GEN_TIMER_READ_MAX * 4);
> +    memory_region_add_subregion(&s->iomem_read,
> +                                R_CNTCV_READ_LOWER,
> +                                &reg_array->mem);
> +    sysbus_init_mmio(sbd, &s->iomem_read);
>  }

Otherwise this looks good.

thanks
-- PMM

[Qemu-devel] [PATCH v3 3/3] xlnx-zynqmp: Connect the ARM Generic Timer

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/arm/xlnx-zynqmp.c         | 14 ++++++++++++++
 include/hw/arm/xlnx-zynqmp.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 0d86ba3..f3891ae 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -38,6 +38,8 @@
 #define SATA_ADDR           0xFD0C0000
 #define SATA_NUM_PORTS      2
 
+#define ARM_GEN_TIMER_ADDR  0xFF260000
+
 #define DP_ADDR             0xfd4a0000
 #define DP_IRQ              113
 
@@ -172,6 +174,10 @@ static void xlnx_zynqmp_init(Object *obj)
         qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
     }
 
+    object_initialize(&s->arm_gen_timer, sizeof(s->arm_gen_timer),
+                      TYPE_ARM_GEN_TIMER);
+    qdev_set_parent_bus(DEVICE(&s->arm_gen_timer), sysbus_get_default());
+
     object_initialize(&s->dp, sizeof(s->dp), TYPE_XLNX_DP);
     qdev_set_parent_bus(DEVICE(&s->dp), sysbus_get_default());
 
@@ -405,6 +411,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         g_free(bus_name);
     }
 
+    object_property_set_bool(OBJECT(&s->arm_gen_timer), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->arm_gen_timer), 0, ARM_GEN_TIMER_ADDR);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->arm_gen_timer), 1, ARM_GEN_TIMER_ADDR - 0x10000);
+
     object_property_set_bool(OBJECT(&s->dp), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index c2931bf..8deabb4 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -26,6 +26,7 @@
 #include "hw/ide/ahci.h"
 #include "hw/sd/sdhci.h"
 #include "hw/ssi/xilinx_spips.h"
+#include "hw/timer/arm_generic_timer.h"
 #include "hw/dma/xlnx_dpdma.h"
 #include "hw/display/xlnx_dp.h"
 
@@ -83,6 +84,7 @@ typedef struct XlnxZynqMPState {
     SysbusAHCIState sata;
     SDHCIState sdhci[XLNX_ZYNQMP_NUM_SDHCI];
     XilinxSPIPS spi[XLNX_ZYNQMP_NUM_SPIS];
+    ARMGenTimer arm_gen_timer;
     XlnxDPState dp;
     XlnxDPDMAState dpdma;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 3/3] xlnx-zynqmp: Connect the ARM Generic Timer

[Peter Maydell]

On 20 December 2016 at 22:42, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  hw/arm/xlnx-zynqmp.c         | 14 ++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h |  2 ++
>  2 files changed, 16 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 0d86ba3..f3891ae 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -38,6 +38,8 @@
>  #define SATA_ADDR           0xFD0C0000
>  #define SATA_NUM_PORTS      2
>
> +#define ARM_GEN_TIMER_ADDR  0xFF260000
> +
>  #define DP_ADDR             0xfd4a0000
>  #define DP_IRQ              113
>
> @@ -172,6 +174,10 @@ static void xlnx_zynqmp_init(Object *obj)
>          qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
>      }
>
> +    object_initialize(&s->arm_gen_timer, sizeof(s->arm_gen_timer),
> +                      TYPE_ARM_GEN_TIMER);
> +    qdev_set_parent_bus(DEVICE(&s->arm_gen_timer), sysbus_get_default());
> +
>      object_initialize(&s->dp, sizeof(s->dp), TYPE_XLNX_DP);
>      qdev_set_parent_bus(DEVICE(&s->dp), sysbus_get_default());
>
> @@ -405,6 +411,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          g_free(bus_name);
>      }
>
> +    object_property_set_bool(OBJECT(&s->arm_gen_timer), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->arm_gen_timer), 0, ARM_GEN_TIMER_ADDR);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->arm_gen_timer), 1, ARM_GEN_TIMER_ADDR - 0x10000);

Having a #define for a base address and then also using
thing - 0x10000 is a bit odd. I'd use two #defines, I think.
(Also avoids the overlong line.)

If we take the "board responsible for only mapping the secure-only
regs into the secure address space" approach this code will need to
change accordingly.

> +
>      object_property_set_bool(OBJECT(&s->dp), true, "realized", &err);
>      if (err) {
>          error_propagate(errp, err);
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index c2931bf..8deabb4 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -26,6 +26,7 @@
>  #include "hw/ide/ahci.h"
>  #include "hw/sd/sdhci.h"
>  #include "hw/ssi/xilinx_spips.h"
> +#include "hw/timer/arm_generic_timer.h"
>  #include "hw/dma/xlnx_dpdma.h"
>  #include "hw/display/xlnx_dp.h"
>
> @@ -83,6 +84,7 @@ typedef struct XlnxZynqMPState {
>      SysbusAHCIState sata;
>      SDHCIState sdhci[XLNX_ZYNQMP_NUM_SDHCI];
>      XilinxSPIPS spi[XLNX_ZYNQMP_NUM_SPIS];
> +    ARMGenTimer arm_gen_timer;
>      XlnxDPState dp;
>      XlnxDPDMAState dpdma;

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/3] Add the generic ARM timer

[Peter Maydell]

On 20 December 2016 at 22:41, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> These three patches and and connect the Generic ARM Timer. This includes
> support for dropping insecure writes and includes the ReadBase memory
> map.

Now reviewed. The really sticky bit I suspect is going to be
how we model the required connections between this and the
CPUs so that adjustments to the counter here update the
views the CPUs have of the counter value.

The other mandatory part of the system-level generic timer is
the CNTCTLBase frame (timer control), but I guess we can
leave that for later.

thanks
-- PMM