[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic T
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/3] arm_generic_timer: Add the ARM Generic Timer |
Date: |
Mon, 9 Jan 2017 17:41:38 -0800 |
On Fri, Jan 6, 2017 at 3:57 AM, Peter Maydell <address@hidden> wrote:
> On 20 December 2016 at 22:42, Alistair Francis
> <address@hidden> 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 <address@hidden>
>> ---
>> 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 <address@hidden>
>> + *
>> + * 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,
>> + ®_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 <address@hidden>
>> + *
>> + * 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