[v6,2/4] cadence_ttc: initial version of device model

Implemented cadence Triple Timer Counter (TCC)

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
Signed-off-by: John Linn <john.linn@xilinx.com>
---
changed from v4:
fixed FSF addess
changed device_init -> type_init
changed from v3:
Fixed race condition where timer could miss match events on wrap around
changed from v2:
changed ptimer to QEMUTimer (Fixed skew/drift issue in timer delays)
changes from v1:
refactored event driven code
marked vmsd as unmigratable

 Makefile.target  |    1 +
 hw/cadence_ttc.c |  439 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 440 insertions(+), 0 deletions(-)
 create mode 100644 hw/cadence_ttc.c

On 20 February 2012 01:45, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Implemented cadence Triple Timer Counter (TCC)
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> Signed-off-by: John Linn <john.linn@xilinx.com>
> ---
> changed from v4:
> fixed FSF addess
> changed device_init -> type_init
> changed from v3:
> Fixed race condition where timer could miss match events on wrap around
> changed from v2:
> changed ptimer to QEMUTimer (Fixed skew/drift issue in timer delays)
> changes from v1:
> refactored event driven code
> marked vmsd as unmigratable
>
>  Makefile.target  |    1 +
>  hw/cadence_ttc.c |  439 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 440 insertions(+), 0 deletions(-)
>  create mode 100644 hw/cadence_ttc.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 868be15..e96b37e 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -344,6 +344,7 @@ obj-arm-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
>  obj-arm-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o pl190.o
>  obj-arm-y += versatile_pci.o
>  obj-arm-y += cadence_uart.o
> +obj-arm-y += cadence_ttc.o
>  obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
>  obj-arm-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
>  obj-arm-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
> diff --git a/hw/cadence_ttc.c b/hw/cadence_ttc.c
> new file mode 100644
> index 0000000..14c3ba7
> --- /dev/null
> +++ b/hw/cadence_ttc.c
> @@ -0,0 +1,439 @@
> +/*
> + * Xilinx Zynq cadence TTC model
> + *
> + * Copyright (c) 2011 Xilinx Inc.
> + * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
> + * Copyright (c) 2012 PetaLogix Pty Ltd.
> + * Written By Haibing Ma
> + *            M. Habib
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "sysbus.h"
> +#include "qemu-timer.h"
> +
> +#ifdef CADENCE_TTC_ERR_DEBUG
> +#define qemu_debug(...) do { \
> +    fprintf(stderr,  ": %s: ", __func__); \
> +    fprintf(stderr, ## __VA_ARGS__); \
> +    fflush(stderr); \
> +    } while (0);
> +#else
> +    #define qemu_debug(...)
> +#endif
> +
> +#define COUNTER_INTR_IV     0x00000001
> +#define COUNTER_INTR_M1     0x00000002
> +#define COUNTER_INTR_M2     0x00000004
> +#define COUNTER_INTR_M3     0x00000008
> +#define COUNTER_INTR_OV     0x00000010
> +#define COUNTER_INTR_EV     0x00000020
> +
> +#define COUNTER_CTRL_DIS    0x00000001
> +#define COUNTER_CTRL_INT    0x00000002
> +#define COUNTER_CTRL_DEC    0x00000004
> +#define COUNTER_CTRL_MATCH  0x00000008
> +#define COUNTER_CTRL_RST    0x00000010
> +
> +#define CLOCK_CTRL_PS_EN    0x00000001
> +#define CLOCK_CTRL_PS_V     0x0000001e
> +
> +typedef struct {
> +    QEMUTimer *timer;
> +    int freq;
> +
> +    uint32_t reg_clock;
> +    uint32_t reg_count;
> +    uint32_t reg_value;
> +    uint16_t reg_interval;
> +    uint16_t reg_match[3];
> +    uint32_t reg_intr;
> +    uint32_t reg_intr_en;
> +    uint32_t reg_event_ctrl;
> +    uint32_t reg_event;
> +
> +    uint64_t cpu_time;
> +    unsigned int cpu_time_valid;
> +
> +    qemu_irq irq;
> +} CadenceTimerState;
> +
> +typedef struct {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    CadenceTimerState * timer[3];
> +} cadence_ttc_state;

This type name is in breach of the coding style, which
demands camelcase.

> +
> +static void cadence_timer_update(CadenceTimerState *s)
> +{
> +    qemu_set_irq(s->irq, !!(s->reg_intr & s->reg_intr_en));
> +}
> +
> +static CadenceTimerState *cadence_timer_from_addr(void *opaque,
> +                                        target_phys_addr_t offset)
> +{
> +    unsigned int index;
> +    cadence_ttc_state *s = (cadence_ttc_state *)opaque;
> +
> +    index = (offset >> 2) % 3;

Really % 3 ? This must be a pain to implement in hardware...

> +    return s->timer[index];
> +}
> +
> +static uint64_t cadence_timer_get_ns(CadenceTimerState *s, uint64_t timer_steps)
> +{
> +    /* timer_steps has max value of 0x100000000. double check it
> +     * (or overflow can happen below) */
> +    assert(timer_steps <= 1ULL << 32);
> +
> +    uint64_t r = timer_steps * 1000000000ULL;
> +    if (s->reg_clock & CLOCK_CTRL_PS_EN) {
> +        r >>= 16 - (((s->reg_clock & CLOCK_CTRL_PS_V) >> 1) + 1);
> +    } else {
> +        r >>= 16;
> +    }
> +    r /= (uint64_t)s->freq;
> +    return r;
> +}
> +
> +static uint64_t cadence_timer_get_steps(CadenceTimerState *s, uint64_t ns)
> +{
> +    uint64_t to_divide = 1000000000ULL;
> +
> +    uint64_t r = ns;
> +     /* for very large intervals (> 8s) do some division first to stop
> +      * overflow (costs some prescision) */
> +    while (r >= 8ULL << 30 && to_divide >  1) {

Stray double spaces here and below.

> +        r /= 1000;
> +        to_divide /= 1000;
> +    }
> +    r <<= 16;
> +    /* keep early-dividing as needed */
> +    while (r >= 8ULL << 30 && to_divide >  1) {
> +        r /= 1000;
> +        to_divide /= 1000;
> +    }
> +    r *= (uint64_t)s->freq;
> +    if (s->reg_clock & CLOCK_CTRL_PS_EN) {
> +        r /= 1 << (((s->reg_clock & CLOCK_CTRL_PS_V) >> 1) + 1);
> +    }
> +
> +    r /= to_divide;
> +    return r;
> +}
> +
> +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
> +{
> +    if (a < b) {
> +        return x > a && x <= b;
> +    }
> +    return x < a && x >= b;
> +}

This looks slightly odd -- should the boundary condition for whether
a value equal to the max/min really change depending on whether a
or b is greater?

> +
> +static void cadence_timer_run(CadenceTimerState *s)
> +{
> +    int i;
> +    int64_t event_interval, next_value;
> +
> +    assert(s->cpu_time_valid); /* cadence_timer_sync must be called first */
> +
> +    if (s->reg_count & COUNTER_CTRL_DIS) {
> +        s->cpu_time_valid = 0;
> +        return;
> +    }
> +
> +    { /* figure out whats going to happen next (rollover or match) */
> +        int64_t interval = (uint64_t)((s->reg_count & COUNTER_CTRL_INT) ?
> +                (int64_t)s->reg_interval + 1 : 0x10000ULL) << 16;
> +        next_value = (s->reg_count & COUNTER_CTRL_DEC) ? -1ULL : interval;
> +        for (i = 0; i < 3; ++i) {
> +            int64_t cand = (uint64_t)s->reg_match[i] << 16;
> +            if (is_between(cand, (uint64_t)s->reg_value, next_value)) {
> +                next_value = cand;
> +            }
> +        }
> +    }
> +    qemu_debug("next timer event value: %09llx\n",
> +            (unsigned long long)next_value);
> +
> +    event_interval = next_value - (int64_t)s->reg_value;
> +    event_interval = (event_interval < 0) ? -event_interval : event_interval;
> +
> +    qemu_mod_timer(s->timer, s->cpu_time +
> +                cadence_timer_get_ns(s, event_interval));
> +}
> +
> +static void cadence_timer_sync(CadenceTimerState *s)
> +{
> +    int i;
> +    int64_t r, x;
> +    int64_t interval = ((s->reg_count & COUNTER_CTRL_INT) ?
> +            (int64_t)s->reg_interval + 1 : 0x10000ULL) << 16;
> +    uint64_t old_time = s->cpu_time;
> +
> +    s->cpu_time = qemu_get_clock_ns(vm_clock);
> +    qemu_debug("cpu time: %lld ns\n", (long long)old_time);
> +
> +    if (!s->cpu_time_valid || old_time == s->cpu_time) {
> +        s->cpu_time_valid = 1;
> +        return;
> +    }
> +
> +    r = (int64_t)cadence_timer_get_steps(s, s->cpu_time - old_time);
> +    x = (int64_t)s->reg_value + ((s->reg_count & COUNTER_CTRL_DEC) ? -r : r);
> +
> +    for (i = 0; i < 3; ++i) {
> +        int64_t m = (int64_t)s->reg_match[i] << 16;
> +        if (m > interval) {
> +            continue;
> +        }
> +        if (is_between(m, s->reg_value, x) ||
> +            is_between(m + interval, s->reg_value, x) ||
> +            is_between(m - interval, s->reg_value, x)) {
> +            s->reg_intr |= (2 << i);
> +        }
> +    }
> +
> +    s->reg_value = (uint32_t)((x + interval) % interval);
> +
> +    if (s->reg_value != x) {
> +        s->reg_intr |= (s->reg_count & COUNTER_CTRL_INT) ?
> +            COUNTER_INTR_IV : COUNTER_INTR_OV;
> +    }
> +    cadence_timer_update(s);
> +}
> +
> +static void cadence_timer_tick(void *opaque)
> +{
> +    CadenceTimerState *s = opaque;
> +
> +    qemu_debug("\n");
> +    cadence_timer_sync(s);
> +    cadence_timer_run(s);
> +}
> +
> +static uint32_t cadence_ttc_read_imp(void *opaque, target_phys_addr_t offset)
> +{
> +    CadenceTimerState *s = cadence_timer_from_addr(opaque, offset);
> +    uint32_t value;
> +
> +    cadence_timer_sync(s);
> +    cadence_timer_run(s);
> +
> +    switch (offset) {
> +    case 0x00: /* clock control */
> +    case 0x04:
> +    case 0x08:
> +        return s->reg_clock;
> +
> +    case 0x0c: /* counter control */
> +    case 0x10:
> +    case 0x14:
> +        return s->reg_count;
> +
> +    case 0x18: /* counter value */
> +    case 0x1c:
> +    case 0x20:
> +        return (uint16_t)(s->reg_value >> 16);
> +
> +    case 0x24: /* reg_interval counter */
> +    case 0x28:
> +    case 0x2c:
> +        return s->reg_interval;
> +
> +    case 0x30: /* match 1 counter */
> +    case 0x34:
> +    case 0x38:
> +        return s->reg_match[0];
> +
> +    case 0x3c: /* match 2 counter */
> +    case 0x40:
> +    case 0x44:
> +        return s->reg_match[1];
> +
> +    case 0x48: /* match 3 counter */
> +    case 0x4c:
> +    case 0x50:
> +        return s->reg_match[2];
> +
> +    case 0x54: /* interrupt register */
> +    case 0x58:
> +    case 0x5c:
> +        /* cleared after read */
> +        value = s->reg_intr;
> +        s->reg_intr = 0;
> +        return value;
> +
> +    case 0x60: /* interrupt enable */
> +    case 0x64:
> +    case 0x68:
> +        return s->reg_intr_en;
> +
> +    case 0x6c:
> +    case 0x70:
> +    case 0x74:
> +        return s->reg_event_ctrl;
> +
> +    case 0x78:
> +    case 0x7c:
> +    case 0x80:
> +        return s->reg_event;
> +
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static uint64_t cadence_ttc_read(void *opaque, target_phys_addr_t offset,
> +    unsigned size)
> +{
> +    uint32_t ret = cadence_ttc_read_imp(opaque, offset);
> +
> +    qemu_debug("addr: %08x data: %08x\n", offset, ret);
> +    return ret;
> +}
> +
> +static void cadence_ttc_write(void *opaque, target_phys_addr_t offset,
> +        uint64_t value, unsigned size)
> +{
> +    CadenceTimerState *s = cadence_timer_from_addr(opaque, offset);
> +
> +    qemu_debug("addr: %08x data %08x\n", offset, (unsigned)value);
> +
> +    cadence_timer_sync(s);
> +
> +    switch (offset) {
> +    case 0x00: /* clock control */
> +    case 0x04:
> +    case 0x08:
> +        s->reg_clock = value & 0x3F;
> +        break;
> +
> +    case 0x0c: /* conter control */

"counter"

> +    case 0x10:
> +    case 0x14:
> +        if (value & COUNTER_CTRL_RST) {
> +            s->reg_value = 0;
> +        }
> +        s->reg_count = value & 0x3f & ~COUNTER_CTRL_RST;
> +        break;
> +
> +    case 0x24: /* interval register */
> +    case 0x28:
> +    case 0x2c:
> +        s->reg_interval = value & 0xffff;
> +        break;
> +
> +    case 0x30: /* match register */
> +    case 0x34:
> +    case 0x38:
> +        s->reg_match[0] = value & 0xffff;
> +
> +    case 0x3c: /* match register */
> +    case 0x40:
> +    case 0x44:
> +        s->reg_match[1] = value & 0xffff;
> +
> +    case 0x48: /* match register */
> +    case 0x4c:
> +    case 0x50:
> +        s->reg_match[2] = value & 0xffff;
> +        break;
> +
> +    case 0x54: /* interrupt register */
> +    case 0x58:
> +    case 0x5c:
> +        s->reg_intr &= (~value & 0xfff);
> +        break;
> +
> +    case 0x60: /* interrupt enable */
> +    case 0x64:
> +    case 0x68:
> +        s->reg_intr_en = value & 0x3f;
> +        break;
> +
> +    case 0x6c: /* event control */
> +    case 0x70:
> +    case 0x74:
> +        s->reg_event_ctrl = value & 0x07;
> +        break;
> +
> +    default:
> +        return;
> +    }
> +
> +    cadence_timer_run(s);
> +    cadence_timer_update(s);
> +}
> +
> +static const MemoryRegionOps cadence_ttc_ops = {
> +    .read = cadence_ttc_read,
> +    .write = cadence_ttc_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static CadenceTimerState *cadence_timer_init(uint32_t freq)
> +{
> +    CadenceTimerState *s;
> +
> +    s = (CadenceTimerState *)g_malloc0(sizeof(CadenceTimerState));
> +    s->freq = freq;
> +    s->reg_count = 0x21;

Reset function.

> +
> +    s->timer = qemu_new_timer_ns(vm_clock, cadence_timer_tick, s);
> +
> +    return s;
> +}
> +
> +static int cadence_ttc_init(SysBusDevice *dev)
> +{
> +    cadence_ttc_state *s = FROM_SYSBUS(cadence_ttc_state, dev);
> +    int i;
> +
> +    for (i = 0; i < 3; ++i) {
> +        s->timer[i] = cadence_timer_init(2500000);
> +        sysbus_init_irq(dev, &s->timer[i]->irq);
> +    }
> +
> +    memory_region_init_io(&s->iomem, &cadence_ttc_ops, s, "timer", 0x1000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +
> +    return 0;
> +}
> +
> +/* FIMXE: add vmsd support */

Leaving FIXME comments in patches is an invitation to being asked
to fix them :-) vmsd shouldn't be very hard.

> +
> +static const VMStateDescription vmstate_cadence_ttc = {
> +    .name = "cadence_TTC",
> +    .unmigratable = 1,
> +};
> +
> +static void cadence_ttc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    sdc->init = cadence_ttc_init;
> +    dc->vmsd = &vmstate_cadence_ttc;
> +}
> +
> +static TypeInfo cadence_ttc_info = {
> +    .name  = "cadence_ttc",
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(cadence_ttc_state),
> +    .class_init = cadence_ttc_class_init,
> +};
> +
> +static void cadence_ttc_register_types(void)
> +{
> +    type_register_static(&cadence_ttc_info);
> +}
> +
> +type_init(cadence_ttc_register_types)

-- PMM

> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
> > +{
> > +    if (a < b) {
> > +        return x > a && x <= b;
> > +    }
> > +    return x < a && x >= b;
> > +}
> 
> This looks slightly odd -- should the boundary condition for whether
> a value equal to the max/min really change depending on :whether a
> or b is greater?

This is a ugly hack.  Instead of figuring out whether we have a count-up or 
count-down timer the code checks for both, and have the "in_between" function 
magically DTRT.  I haven't followed the paths through in enough detail to 
figure out whether it gets all the corner cases right.

Paul

On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <paul@codesourcery.com> wrote:
>> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
>> > +{
>> > +    if (a < b) {
>> > +        return x > a && x <= b;
>> > +    }
>> > +    return x < a && x >= b;
>> > +}
>>
>> This looks slightly odd -- should the boundary condition for whether
>> a value equal to the max/min really change depending on :whether a
>> or b is greater?

The function determines whether x is in-between a and b exclusive of
a, inclusive of b, so it is consistent with itself in that regard.

>
> This is a ugly hack.  Instead of figuring out whether we have a count-up or
> count-down timer the code checks for both, and have the "in_between" function
> magically DTRT.  I haven't followed the paths through in enough detail to
> figure out whether it gets all the corner cases right.
>

Is it really a "hack"?? For count up b will always be greater than a,
and for count down the reverse. I suppose I could assert these
conditions at the call site for peace of mind? The invocation from
cadence_timer_run doesn't care whether it is count up of count down,
it really does just only care if the match value is in-between the
current timer value and the next timer value, which is exactly what
this function determines.

> Paul

On Tue, Feb 21, 2012 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 February 2012 01:45, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> Implemented cadence Triple Timer Counter (TCC)
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> Signed-off-by: John Linn <john.linn@xilinx.com>
>> ---
>> changed from v4:
>> fixed FSF addess
>> changed device_init -> type_init
>> changed from v3:
>> Fixed race condition where timer could miss match events on wrap around
>> changed from v2:
>> changed ptimer to QEMUTimer (Fixed skew/drift issue in timer delays)
>> changes from v1:
>> refactored event driven code
>> marked vmsd as unmigratable
>>
>>  Makefile.target  |    1 +
>>  hw/cadence_ttc.c |  439 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 440 insertions(+), 0 deletions(-)
>>  create mode 100644 hw/cadence_ttc.c
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index 868be15..e96b37e 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -344,6 +344,7 @@ obj-arm-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
>>  obj-arm-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o pl190.o
>>  obj-arm-y += versatile_pci.o
>>  obj-arm-y += cadence_uart.o
>> +obj-arm-y += cadence_ttc.o
>>  obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
>>  obj-arm-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
>>  obj-arm-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
>> diff --git a/hw/cadence_ttc.c b/hw/cadence_ttc.c
>> new file mode 100644
>> index 0000000..14c3ba7
>> --- /dev/null
>> +++ b/hw/cadence_ttc.c
>> @@ -0,0 +1,439 @@
>> +/*
>> + * Xilinx Zynq cadence TTC model
>> + *
>> + * Copyright (c) 2011 Xilinx Inc.
>> + * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
>> + * Copyright (c) 2012 PetaLogix Pty Ltd.
>> + * Written By Haibing Ma
>> + *            M. Habib
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "sysbus.h"
>> +#include "qemu-timer.h"
>> +
>> +#ifdef CADENCE_TTC_ERR_DEBUG
>> +#define qemu_debug(...) do { \
>> +    fprintf(stderr,  ": %s: ", __func__); \
>> +    fprintf(stderr, ## __VA_ARGS__); \
>> +    fflush(stderr); \
>> +    } while (0);
>> +#else
>> +    #define qemu_debug(...)
>> +#endif
>> +
>> +#define COUNTER_INTR_IV     0x00000001
>> +#define COUNTER_INTR_M1     0x00000002
>> +#define COUNTER_INTR_M2     0x00000004
>> +#define COUNTER_INTR_M3     0x00000008
>> +#define COUNTER_INTR_OV     0x00000010
>> +#define COUNTER_INTR_EV     0x00000020
>> +
>> +#define COUNTER_CTRL_DIS    0x00000001
>> +#define COUNTER_CTRL_INT    0x00000002
>> +#define COUNTER_CTRL_DEC    0x00000004
>> +#define COUNTER_CTRL_MATCH  0x00000008
>> +#define COUNTER_CTRL_RST    0x00000010
>> +
>> +#define CLOCK_CTRL_PS_EN    0x00000001
>> +#define CLOCK_CTRL_PS_V     0x0000001e
>> +
>> +typedef struct {
>> +    QEMUTimer *timer;
>> +    int freq;
>> +
>> +    uint32_t reg_clock;
>> +    uint32_t reg_count;
>> +    uint32_t reg_value;
>> +    uint16_t reg_interval;
>> +    uint16_t reg_match[3];
>> +    uint32_t reg_intr;
>> +    uint32_t reg_intr_en;
>> +    uint32_t reg_event_ctrl;
>> +    uint32_t reg_event;
>> +
>> +    uint64_t cpu_time;
>> +    unsigned int cpu_time_valid;
>> +
>> +    qemu_irq irq;
>> +} CadenceTimerState;
>> +
>> +typedef struct {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    CadenceTimerState * timer[3];
>> +} cadence_ttc_state;
>
> This type name is in breach of the coding style, which
> demands camelcase.
>
>> +
>> +static void cadence_timer_update(CadenceTimerState *s)
>> +{
>> +    qemu_set_irq(s->irq, !!(s->reg_intr & s->reg_intr_en));
>> +}
>> +
>> +static CadenceTimerState *cadence_timer_from_addr(void *opaque,
>> +                                        target_phys_addr_t offset)
>> +{
>> +    unsigned int index;
>> +    cadence_ttc_state *s = (cadence_ttc_state *)opaque;
>> +
>> +    index = (offset >> 2) % 3;
>
> Really % 3 ? This must be a pain to implement in hardware...
>
>> +    return s->timer[index];
>> +}
>> +
>> +static uint64_t cadence_timer_get_ns(CadenceTimerState *s, uint64_t timer_steps)
>> +{
>> +    /* timer_steps has max value of 0x100000000. double check it
>> +     * (or overflow can happen below) */
>> +    assert(timer_steps <= 1ULL << 32);
>> +
>> +    uint64_t r = timer_steps * 1000000000ULL;
>> +    if (s->reg_clock & CLOCK_CTRL_PS_EN) {
>> +        r >>= 16 - (((s->reg_clock & CLOCK_CTRL_PS_V) >> 1) + 1);
>> +    } else {
>> +        r >>= 16;
>> +    }
>> +    r /= (uint64_t)s->freq;
>> +    return r;
>> +}
>> +
>> +static uint64_t cadence_timer_get_steps(CadenceTimerState *s, uint64_t ns)
>> +{
>> +    uint64_t to_divide = 1000000000ULL;
>> +
>> +    uint64_t r = ns;
>> +     /* for very large intervals (> 8s) do some division first to stop
>> +      * overflow (costs some prescision) */
>> +    while (r >= 8ULL << 30 && to_divide >  1) {
>
> Stray double spaces here and below.
>

Fixed

>> +        r /= 1000;
>> +        to_divide /= 1000;
>> +    }
>> +    r <<= 16;
>> +    /* keep early-dividing as needed */
>> +    while (r >= 8ULL << 30 && to_divide >  1) {
>> +        r /= 1000;
>> +        to_divide /= 1000;
>> +    }
>> +    r *= (uint64_t)s->freq;
>> +    if (s->reg_clock & CLOCK_CTRL_PS_EN) {
>> +        r /= 1 << (((s->reg_clock & CLOCK_CTRL_PS_V) >> 1) + 1);
>> +    }
>> +
>> +    r /= to_divide;
>> +    return r;
>> +}
>> +
>> +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
>> +{
>> +    if (a < b) {
>> +        return x > a && x <= b;
>> +    }
>> +    return x < a && x >= b;
>> +}
>
> This looks slightly odd -- should the boundary condition for whether
> a value equal to the max/min really change depending on whether a
> or b is greater?
>
>> +
>> +static void cadence_timer_run(CadenceTimerState *s)
>> +{
>> +    int i;
>> +    int64_t event_interval, next_value;
>> +
>> +    assert(s->cpu_time_valid); /* cadence_timer_sync must be called first */
>> +
>> +    if (s->reg_count & COUNTER_CTRL_DIS) {
>> +        s->cpu_time_valid = 0;
>> +        return;
>> +    }
>> +
>> +    { /* figure out whats going to happen next (rollover or match) */
>> +        int64_t interval = (uint64_t)((s->reg_count & COUNTER_CTRL_INT) ?
>> +                (int64_t)s->reg_interval + 1 : 0x10000ULL) << 16;
>> +        next_value = (s->reg_count & COUNTER_CTRL_DEC) ? -1ULL : interval;
>> +        for (i = 0; i < 3; ++i) {
>> +            int64_t cand = (uint64_t)s->reg_match[i] << 16;
>> +            if (is_between(cand, (uint64_t)s->reg_value, next_value)) {
>> +                next_value = cand;
>> +            }
>> +        }
>> +    }
>> +    qemu_debug("next timer event value: %09llx\n",
>> +            (unsigned long long)next_value);
>> +
>> +    event_interval = next_value - (int64_t)s->reg_value;
>> +    event_interval = (event_interval < 0) ? -event_interval : event_interval;
>> +
>> +    qemu_mod_timer(s->timer, s->cpu_time +
>> +                cadence_timer_get_ns(s, event_interval));
>> +}
>> +
>> +static void cadence_timer_sync(CadenceTimerState *s)
>> +{
>> +    int i;
>> +    int64_t r, x;
>> +    int64_t interval = ((s->reg_count & COUNTER_CTRL_INT) ?
>> +            (int64_t)s->reg_interval + 1 : 0x10000ULL) << 16;
>> +    uint64_t old_time = s->cpu_time;
>> +
>> +    s->cpu_time = qemu_get_clock_ns(vm_clock);
>> +    qemu_debug("cpu time: %lld ns\n", (long long)old_time);
>> +
>> +    if (!s->cpu_time_valid || old_time == s->cpu_time) {
>> +        s->cpu_time_valid = 1;
>> +        return;
>> +    }
>> +
>> +    r = (int64_t)cadence_timer_get_steps(s, s->cpu_time - old_time);
>> +    x = (int64_t)s->reg_value + ((s->reg_count & COUNTER_CTRL_DEC) ? -r : r);
>> +
>> +    for (i = 0; i < 3; ++i) {
>> +        int64_t m = (int64_t)s->reg_match[i] << 16;
>> +        if (m > interval) {
>> +            continue;
>> +        }
>> +        if (is_between(m, s->reg_value, x) ||
>> +            is_between(m + interval, s->reg_value, x) ||
>> +            is_between(m - interval, s->reg_value, x)) {
>> +            s->reg_intr |= (2 << i);
>> +        }
>> +    }
>> +
>> +    s->reg_value = (uint32_t)((x + interval) % interval);
>> +
>> +    if (s->reg_value != x) {
>> +        s->reg_intr |= (s->reg_count & COUNTER_CTRL_INT) ?
>> +            COUNTER_INTR_IV : COUNTER_INTR_OV;
>> +    }
>> +    cadence_timer_update(s);
>> +}
>> +
>> +static void cadence_timer_tick(void *opaque)
>> +{
>> +    CadenceTimerState *s = opaque;
>> +
>> +    qemu_debug("\n");
>> +    cadence_timer_sync(s);
>> +    cadence_timer_run(s);
>> +}
>> +
>> +static uint32_t cadence_ttc_read_imp(void *opaque, target_phys_addr_t offset)
>> +{
>> +    CadenceTimerState *s = cadence_timer_from_addr(opaque, offset);
>> +    uint32_t value;
>> +
>> +    cadence_timer_sync(s);
>> +    cadence_timer_run(s);
>> +
>> +    switch (offset) {
>> +    case 0x00: /* clock control */
>> +    case 0x04:
>> +    case 0x08:
>> +        return s->reg_clock;
>> +
>> +    case 0x0c: /* counter control */
>> +    case 0x10:
>> +    case 0x14:
>> +        return s->reg_count;
>> +
>> +    case 0x18: /* counter value */
>> +    case 0x1c:
>> +    case 0x20:
>> +        return (uint16_t)(s->reg_value >> 16);
>> +
>> +    case 0x24: /* reg_interval counter */
>> +    case 0x28:
>> +    case 0x2c:
>> +        return s->reg_interval;
>> +
>> +    case 0x30: /* match 1 counter */
>> +    case 0x34:
>> +    case 0x38:
>> +        return s->reg_match[0];
>> +
>> +    case 0x3c: /* match 2 counter */
>> +    case 0x40:
>> +    case 0x44:
>> +        return s->reg_match[1];
>> +
>> +    case 0x48: /* match 3 counter */
>> +    case 0x4c:
>> +    case 0x50:
>> +        return s->reg_match[2];
>> +
>> +    case 0x54: /* interrupt register */
>> +    case 0x58:
>> +    case 0x5c:
>> +        /* cleared after read */
>> +        value = s->reg_intr;
>> +        s->reg_intr = 0;
>> +        return value;
>> +
>> +    case 0x60: /* interrupt enable */
>> +    case 0x64:
>> +    case 0x68:
>> +        return s->reg_intr_en;
>> +
>> +    case 0x6c:
>> +    case 0x70:
>> +    case 0x74:
>> +        return s->reg_event_ctrl;
>> +
>> +    case 0x78:
>> +    case 0x7c:
>> +    case 0x80:
>> +        return s->reg_event;
>> +
>> +    default:
>> +        return 0;
>> +    }
>> +}
>> +
>> +static uint64_t cadence_ttc_read(void *opaque, target_phys_addr_t offset,
>> +    unsigned size)
>> +{
>> +    uint32_t ret = cadence_ttc_read_imp(opaque, offset);
>> +
>> +    qemu_debug("addr: %08x data: %08x\n", offset, ret);
>> +    return ret;
>> +}
>> +
>> +static void cadence_ttc_write(void *opaque, target_phys_addr_t offset,
>> +        uint64_t value, unsigned size)
>> +{
>> +    CadenceTimerState *s = cadence_timer_from_addr(opaque, offset);
>> +
>> +    qemu_debug("addr: %08x data %08x\n", offset, (unsigned)value);
>> +
>> +    cadence_timer_sync(s);
>> +
>> +    switch (offset) {
>> +    case 0x00: /* clock control */
>> +    case 0x04:
>> +    case 0x08:
>> +        s->reg_clock = value & 0x3F;
>> +        break;
>> +
>> +    case 0x0c: /* conter control */
>
> "counter"
>

Fixed

>> +    case 0x10:
>> +    case 0x14:
>> +        if (value & COUNTER_CTRL_RST) {
>> +            s->reg_value = 0;
>> +        }
>> +        s->reg_count = value & 0x3f & ~COUNTER_CTRL_RST;
>> +        break;
>> +
>> +    case 0x24: /* interval register */
>> +    case 0x28:
>> +    case 0x2c:
>> +        s->reg_interval = value & 0xffff;
>> +        break;
>> +
>> +    case 0x30: /* match register */
>> +    case 0x34:
>> +    case 0x38:
>> +        s->reg_match[0] = value & 0xffff;
>> +
>> +    case 0x3c: /* match register */
>> +    case 0x40:
>> +    case 0x44:
>> +        s->reg_match[1] = value & 0xffff;
>> +
>> +    case 0x48: /* match register */
>> +    case 0x4c:
>> +    case 0x50:
>> +        s->reg_match[2] = value & 0xffff;
>> +        break;
>> +
>> +    case 0x54: /* interrupt register */
>> +    case 0x58:
>> +    case 0x5c:
>> +        s->reg_intr &= (~value & 0xfff);
>> +        break;
>> +
>> +    case 0x60: /* interrupt enable */
>> +    case 0x64:
>> +    case 0x68:
>> +        s->reg_intr_en = value & 0x3f;
>> +        break;
>> +
>> +    case 0x6c: /* event control */
>> +    case 0x70:
>> +    case 0x74:
>> +        s->reg_event_ctrl = value & 0x07;
>> +        break;
>> +
>> +    default:
>> +        return;
>> +    }
>> +
>> +    cadence_timer_run(s);
>> +    cadence_timer_update(s);
>> +}
>> +
>> +static const MemoryRegionOps cadence_ttc_ops = {
>> +    .read = cadence_ttc_read,
>> +    .write = cadence_ttc_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static CadenceTimerState *cadence_timer_init(uint32_t freq)
>> +{
>> +    CadenceTimerState *s;
>> +
>> +    s = (CadenceTimerState *)g_malloc0(sizeof(CadenceTimerState));
>> +    s->freq = freq;
>> +    s->reg_count = 0x21;
>
> Reset function.
>

Fixed

>> +
>> +    s->timer = qemu_new_timer_ns(vm_clock, cadence_timer_tick, s);
>> +
>> +    return s;
>> +}
>> +
>> +static int cadence_ttc_init(SysBusDevice *dev)
>> +{
>> +    cadence_ttc_state *s = FROM_SYSBUS(cadence_ttc_state, dev);
>> +    int i;
>> +
>> +    for (i = 0; i < 3; ++i) {
>> +        s->timer[i] = cadence_timer_init(2500000);
>> +        sysbus_init_irq(dev, &s->timer[i]->irq);
>> +    }
>> +
>> +    memory_region_init_io(&s->iomem, &cadence_ttc_ops, s, "timer", 0x1000);
>> +    sysbus_init_mmio(dev, &s->iomem);
>> +
>> +    return 0;
>> +}
>> +
>> +/* FIMXE: add vmsd support */
>
> Leaving FIXME comments in patches is an invitation to being asked
> to fix them :-) vmsd shouldn't be very hard.
>
>> +
>> +static const VMStateDescription vmstate_cadence_ttc = {
>> +    .name = "cadence_TTC",
>> +    .unmigratable = 1,
>> +};
>> +
>> +static void cadence_ttc_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> +    sdc->init = cadence_ttc_init;
>> +    dc->vmsd = &vmstate_cadence_ttc;
>> +}
>> +
>> +static TypeInfo cadence_ttc_info = {
>> +    .name  = "cadence_ttc",
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size  = sizeof(cadence_ttc_state),
>> +    .class_init = cadence_ttc_class_init,
>> +};
>> +
>> +static void cadence_ttc_register_types(void)
>> +{
>> +    type_register_static(&cadence_ttc_info);
>> +}
>> +
>> +type_init(cadence_ttc_register_types)
>
> -- PMM

> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <paul@codesourcery.com> wrote:
> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
> >> > +{
> >> > +    if (a < b) {
> >> > +        return x > a && x <= b;
> >> > +    }
> >> > +    return x < a && x >= b;
> >> > +}
> >> 
> >> This looks slightly odd -- should the boundary condition for whether
> >> a value equal to the max/min really change depending on :whether a
> >> or b is greater?
> 
> The function determines whether x is in-between a and b exclusive of
> a, inclusive of b, so it is consistent with itself in that regard.
> 
> > This is a ugly hack.  Instead of figuring out whether we have a count-up
> > or count-down timer the code checks for both, and have the "in_between"
> > function magically DTRT.  I haven't followed the paths through in enough
> > detail to figure out whether it gets all the corner cases right.
> 
> Is it really a "hack"?? For count up b will always be greater than a,
> and for count down the reverse. I suppose I could assert these
> conditions at the call site for peace of mind? The invocation from
> cadence_timer_run doesn't care whether it is count up of count down,
> it really does just only care if the match value is in-between the
> current timer value and the next timer value, which is exactly what
> this function determines.

When you explain it like this, it makes a more sense.  But this isn't 
immediately obvious from the code.  It took me at least a couple of readings 
to figure out what was going on. This is exactly the sort of thing that should 
be described in comments.  A function with a very generic name is used in a 
way that has fairly subtle implications.  There's a good chance someone[1] 
will come along in a few months/years, reuse this function and "fix" the 
wierdness at the same time.

Annother non-obvious detail is the way you handle overflow.  Specifically you 
check a range both plus and minus the wrap value before wrapping the final 
count.  This is certainly confusing/surprising when you first encounter it.  
Very large steps result in overlapping ranges, which triggers [in this case 
harmless] warning bells.

Thinking about that, I realised why I don't like the following line:

> +    s->reg_value = (uint32_t)((x + interval) % interval);

This assumes x > -interval, which is not always true.

Paul

[1] "someone" includes me.  After I've forgotten this obscure detail.

On Tue, Feb 28, 2012 at 1:45 AM, Paul Brook <paul@codesourcery.com> wrote:
>> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <paul@codesourcery.com> wrote:
>> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
>> >> > +{
>> >> > +    if (a < b) {
>> >> > +        return x > a && x <= b;
>> >> > +    }
>> >> > +    return x < a && x >= b;
>> >> > +}
>> >>
>> >> This looks slightly odd -- should the boundary condition for whether
>> >> a value equal to the max/min really change depending on :whether a
>> >> or b is greater?
>>
>> The function determines whether x is in-between a and b exclusive of
>> a, inclusive of b, so it is consistent with itself in that regard.
>>
>> > This is a ugly hack.  Instead of figuring out whether we have a count-up
>> > or count-down timer the code checks for both, and have the "in_between"
>> > function magically DTRT.  I haven't followed the paths through in enough
>> > detail to figure out whether it gets all the corner cases right.
>>
>> Is it really a "hack"?? For count up b will always be greater than a,
>> and for count down the reverse. I suppose I could assert these
>> conditions at the call site for peace of mind? The invocation from
>> cadence_timer_run doesn't care whether it is count up of count down,
>> it really does just only care if the match value is in-between the
>> current timer value and the next timer value, which is exactly what
>> this function determines.
>
> When you explain it like this, it makes a more sense.  But this isn't
> immediately obvious from the code.  It took me at least a couple of readings
> to figure out what was going on. This is exactly the sort of thing that should
> be described in comments.

Ok, ill be a little more descriptive :)

A function with a very generic name

Perhaps clarify the whole inclusive a exclusive b in comment?

is used in a
> way that has fairly subtle implications.  There's a good chance someone[1]
> will come along in a few months/years, reuse this function and "fix" the
> wierdness at the same time.
>
> Annother non-obvious detail is the way you handle overflow.  Specifically you
> check a range both plus and minus the wrap value before wrapping the final
> count.  This is certainly confusing/surprising when you first encounter it.
> Very large steps result in overlapping ranges, which triggers [in this case
> harmless] warning bells.
>
> Thinking about that, I realised why I don't like the following line:
>
>> +    s->reg_value = (uint32_t)((x + interval) % interval);
>
> This assumes x > -interval, which is not always true.

This would mean you have wrapped twice or more in one time step, which
I am assuming is a fatal error condition, as It means your software
has missed interrupts and all sort of race conditions would occur. I
would personally prefer to assert !(x < -interval) and have qemu
hw_error or something, as in these cases QEMU can just not handle your
super quick timer wrap around.

>
> Paul
>
> [1] "someone" includes me.  After I've forgotten this obscure detail.

Heres the diff for proposed comments:

@@ -129,6 +129,8 @@ static uint64_t
cadence_timer_get_steps(CadenceTimerState *s, uint64_t ns)
     return r;
 }

+/* determine if x is inbetween a and b, exclusive of a, inclusive of b */
+
 static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
 {
     if (a < b) {
@@ -188,12 +190,18 @@ static void cadence_timer_sync(CadenceTimerState *s)

     r = (int64_t)cadence_timer_get_steps(s, s->cpu_time - old_time);
     x = (int64_t)s->reg_value + ((s->reg_count & COUNTER_CTRL_DEC) ? -r : r);
+    if (x > 2 * interval || x < -interval) {
+        hw_error("Timer wrapped around twice in one I/O event "
+                "interval (wrap interval set too low / frequency too high)\n");
+    }

     for (i = 0; i < 3; ++i) {
         int64_t m = (int64_t)s->reg_match[i] << 16;
         if (m > interval) {
             continue;
         }
+        /* check to see if match event has occured. check m +/- interval
+         * to account for match events in wrap around cases */
         if (is_between(m, s->reg_value, x) ||
             is_between(m + interval, s->reg_value, x) ||
             is_between(m - interval, s->reg_value, x)) {


On Tue, Feb 28, 2012 at 11:04 AM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Tue, Feb 28, 2012 at 1:45 AM, Paul Brook <paul@codesourcery.com> wrote:
>>> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <paul@codesourcery.com> wrote:
>>> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
>>> >> > +{
>>> >> > +    if (a < b) {
>>> >> > +        return x > a && x <= b;
>>> >> > +    }
>>> >> > +    return x < a && x >= b;
>>> >> > +}
>>> >>
>>> >> This looks slightly odd -- should the boundary condition for whether
>>> >> a value equal to the max/min really change depending on :whether a
>>> >> or b is greater?
>>>
>>> The function determines whether x is in-between a and b exclusive of
>>> a, inclusive of b, so it is consistent with itself in that regard.
>>>
>>> > This is a ugly hack.  Instead of figuring out whether we have a count-up
>>> > or count-down timer the code checks for both, and have the "in_between"
>>> > function magically DTRT.  I haven't followed the paths through in enough
>>> > detail to figure out whether it gets all the corner cases right.
>>>
>>> Is it really a "hack"?? For count up b will always be greater than a,
>>> and for count down the reverse. I suppose I could assert these
>>> conditions at the call site for peace of mind? The invocation from
>>> cadence_timer_run doesn't care whether it is count up of count down,
>>> it really does just only care if the match value is in-between the
>>> current timer value and the next timer value, which is exactly what
>>> this function determines.
>>
>> When you explain it like this, it makes a more sense.  But this isn't
>> immediately obvious from the code.  It took me at least a couple of readings
>> to figure out what was going on. This is exactly the sort of thing that should
>> be described in comments.
>
> Ok, ill be a little more descriptive :)
>
> A function with a very generic name
>
> Perhaps clarify the whole inclusive a exclusive b in comment?
>
> is used in a
>> way that has fairly subtle implications.  There's a good chance someone[1]
>> will come along in a few months/years, reuse this function and "fix" the
>> wierdness at the same time.
>>
>> Annother non-obvious detail is the way you handle overflow.  Specifically you
>> check a range both plus and minus the wrap value before wrapping the final
>> count.  This is certainly confusing/surprising when you first encounter it.
>> Very large steps result in overlapping ranges, which triggers [in this case
>> harmless] warning bells.
>>
>> Thinking about that, I realised why I don't like the following line:
>>
>>> +    s->reg_value = (uint32_t)((x + interval) % interval);
>>
>> This assumes x > -interval, which is not always true.
>
> This would mean you have wrapped twice or more in one time step, which
> I am assuming is a fatal error condition, as It means your software
> has missed interrupts and all sort of race conditions would occur. I
> would personally prefer to assert !(x < -interval) and have qemu
> hw_error or something, as in these cases QEMU can just not handle your
> super quick timer wrap around.
>
>>
>> Paul
>>
>> [1] "someone" includes me.  After I've forgotten this obscure detail.

> > Thinking about that, I realised why I don't like the following line:
> >> +    s->reg_value = (uint32_t)((x + interval) % interval);
> > 
> > This assumes x > -interval, which is not always true.
> 
> This would mean you have wrapped twice or more in one time step, which
> I am assuming is a fatal error condition, as It means your software
> has missed interrupts and all sort of race conditions would occur. I
> would personally prefer to assert !(x < -interval) and have qemu
> hw_error or something, as in these cases QEMU can just not handle your
> super quick timer wrap around.

No, not fatal at all.  On a heavily loaded host it's normal for qemu[1] to 
stall for tens of miliseconds at a time.  In extreme cases several seconds at 
a time, though we've fixed most of those.  This is greater than the interval 
of many guest periodic events.  A linux guest with HZ=1000 (even HZ=100) will 
routinely loose timer ticks.  By my reading your timers have a configurable 
limit, so the obvious configuration is a limit of 25000 (2.5MHz / 100), and 
trigger the jiffy tick off the timer wrap/reload.

If your guest OS assumes their ISR will complete before timer triggers again 
then it will break.  That's their problem.  Any guest that relies on 
consistent realtime performance/behavior is going to malfunction when run 
inside qemu.  This is only one of several ways that qemu behavior can differ 
from that of real hardware.

You can workaround some of these issues with -icount, though that has its own 
set of problems.  One of which is that is doesn't support KVM or SMP[2]. 
guests.

Paul

[1] The same applies for KVM.
[2] SMP may be fixable.  KVM probably not.

On Tue, Feb 28, 2012 at 10:07 PM, Paul Brook <paul@codesourcery.com> wrote:
>> > Thinking about that, I realised why I don't like the following line:
>> >> +    s->reg_value = (uint32_t)((x + interval) % interval);
>> >
>> > This assumes x > -interval, which is not always true.
>>
>> This would mean you have wrapped twice or more in one time step, which
>> I am assuming is a fatal error condition, as It means your software
>> has missed interrupts and all sort of race conditions would occur. I
>> would personally prefer to assert !(x < -interval) and have qemu
>> hw_error or something, as in these cases QEMU can just not handle your
>> super quick timer wrap around.
>
> No, not fatal at all.  On a heavily loaded host it's normal for qemu[1] to
> stall for tens of miliseconds at a time.  In extreme cases several seconds at
> a time, though we've fixed most of those.  This is greater than the interval
> of many guest periodic events.  A linux guest with HZ=1000 (even HZ=100) will
> routinely loose timer ticks.  By my reading your timers have a configurable
> limit, so the obvious configuration is a limit of 25000 (2.5MHz / 100), and
> trigger the jiffy tick off the timer wrap/reload.
>

Yeh i discovered that in testing :) my hw_error did trigger under
linux. Fixed that % issue you raised (in v8) such this it will not % a
-ve number when the timer multi-wraps.

> If your guest OS assumes their ISR will complete before timer triggers again
> then it will break.  That's their problem.  Any guest that relies on
> consistent realtime performance/behavior is going to malfunction when run
> inside qemu.  This is only one of several ways that qemu behavior can differ
> from that of real hardware.
>
> You can workaround some of these issues with -icount, though that has its own
> set of problems.  One of which is that is doesn't support KVM or SMP[2].
> guests.
>
> Paul
>
> [1] The same applies for KVM.
> [2] SMP may be fixable.  KVM probably not.

Peter