Re: [Qemu-devel] [PATCH] Basic support for ARM A15 "architectured" (cp15) timers

[Blue Swirl]

On Sat, Sep 15, 2012 at 8:57 AM, Daniel Forsgren
<address@hidden> wrote:
> Thanks for the feedback!
>
> I should probably point out (as I wrote in my initial mail) that this is just 
> a prototype - a quick n dirty hack to get Linux up and running with the arch 
> timers. It is very true that I'm not following the QEMU coding standard (I 
> must admit that haven't even read it).
>
> The background is that I wanted to run QEMU and the A15 CoreTile side by side 
> with as similar configuration as possible. And the missing A15 timers was 
> kind of stopping me, so I had to work around that. (For that reason, I tried 
> to keep most of my additions in a single file and not to clutter the entire 
> source tree). At the same time I saw that someone asked for these timers on 
> the mailing list some month ago. So I thought that I could as well share my 
> results.
>
> That said, I'm very grateful that you still took the time to actually review 
> the code, and I will try to improve it. I have fixed some minor issues that 
> prevented me to run multicore so far. (My eventual goal is to run as close as 
> possible to the real 2xA15+3xA7 CoreTile that I try to mimic).
>
> However, being a QEMU newbie I have a couple of questions related to the 
> right way of implementing this:
>
> 1) What is considered to be part of the "core" and what is considered to be a 
> device external to the core? To me, it looks like co-processor functionality 
> in general is considered to be part of the core (implemented in 
> target-arm/helper.c or similar), whereas timer devices in general are kept in 
> hw/arm_* (c.f. arm_timer.c and arm_mptimer.c). But in this case I have a 
> timer that is implemented as a coprocessor - where should that go? Or should 
> it be split in two places?

SoC devices attached to the CPU is a bit grey area. In this case, I
think coprocessor should be part of the CPU. Peter?

>
> 2) Where should a device like this save its own internal state? Some other 
> devices seems to save its state as an extension of the SysBusDevice 
> structure, but coprocessor state in general rather seems to be part of 
> CPUARMState or similar. What is the right way in this particular case?

Currently the divisive line seems to be that devices which are only
accessible via MMIO or generic IO instructions should be external to
CPU. But it could be possible to introduce generic methods to register
other classes, for example for the ARM coprocessors, x86 model
specific registers, PPC SPRs and Sparc ASIs. The memory API should
support adding more address spaces. Maybe this could be a nice
approach.

But I'd vote for CPUARMState for now.

>
> br,
>
> /D
>
>> -----Original Message-----
>> From: Blue Swirl [mailto:address@hidden
>> Sent: den 14 september 2012 19:26
>> To: Daniel Forsgren
>> Cc: address@hidden
>> Subject: Re: [Qemu-devel] [PATCH] Basic support for ARM A15 "architectured"
>> (cp15) timers
>>
>> On Wed, Sep 12, 2012 at 11:49 AM, Daniel Forsgren
>> <address@hidden> wrote:
>> > This patch adds basic support for the "architected" timers (i.e. cp15)
>> > found in A15. It's enough to allow Linux to boot, using arch_timer for
>> > the tick. However - it is not a complete model of the timer block at
>> > large, it is not that well structured, and it is currently tested with
>> > qemu-linaro-1.1.50-2012.07 (not latest and greatest). It's simply a
>> > prototype.
>> >
>> > However, if anyone wants to play with the architectured (cp15) timers
>> > instead of sp804, then please feel free to try it out. It has been
>> > tested with linux-linaro-3.6-rc2-2012.08, and you can easily verify
>> > the existence of these timers under /proc/interrupts:
>> >
>> >     address@hidden:~# cat /proc/interrupts
>> >     cat /proc/interrupts
>> >                CPU0
>> >      29:       7424       GIC  arch_timer
>> >      30:          0       GIC  arch_timer
>> >
>> > Please note that this also requires some minor fixes that are not part
>> > of qemu-linaro-1.1.50-2012.07:
>> >
>> >     http://patches.linaro.org/9833/
>> >
>> > Signed-off-by: Daniel Forsgren <address@hidden>
>> >
>> > ---
>> >
>> > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c qemu-linaro-1.1.50-
>> 2012.07-modified/hw/a15mpcore.c
>> > --- qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c   2012-07-05
>> 16:48:28.000000000 +0200
>> > +++ qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c  2012-09-12
>> > +++ 11:24:25.844237405 +0200
>> > @@ -28,6 +28,7 @@ typedef struct A15MPPrivState {
>> >      uint32_t num_cpu;
>> >      uint32_t num_irq;
>> >      MemoryRegion container;
>> > +    DeviceState *archtimer;
>> >      DeviceState *gic;
>> >  } A15MPPrivState;
>> >
>> > @@ -40,7 +41,8 @@ static void a15mp_priv_set_irq(void *opa  static int
>> > a15mp_priv_init(SysBusDevice *dev)  {
>> >      A15MPPrivState *s = FROM_SYSBUS(A15MPPrivState, dev);
>> > -    SysBusDevice *busdev;
>> > +    SysBusDevice *busdev, *timerbusdev;
>> > +    int i;
>> >
>> >      if (kvm_irqchip_in_kernel()) {
>> >          s->gic = qdev_create(NULL, "kvm-arm_gic"); @@ -60,6 +62,11 @@
>> > static int a15mp_priv_init(SysBusDevice
>> >      /* Pass through inbound GPIO lines to the GIC */
>> >      qdev_init_gpio_in(&s->busdev.qdev, a15mp_priv_set_irq, s->num_irq
>> > - 32);
>> >
>> > +    s->archtimer = qdev_create(NULL, "arm_archtimer");
>> > +    //    qdev_prop_set_uint32(s->archtimer, "num-cpu", s->num_cpu);
>>
>> Please don't introduce dead code.
>>
>> > +    qdev_init_nofail(s->archtimer);
>> > +    timerbusdev = sysbus_from_qdev(s->archtimer);
>> > +
>> >      /* Memory map (addresses are offsets from PERIPHBASE):
>> >       *  0x0000-0x0fff -- reserved
>> >       *  0x1000-0x1fff -- GIC Distributor @@ -75,6 +82,16 @@ static
>> > int a15mp_priv_init(SysBusDevice
>> >                                  sysbus_mmio_get_region(busdev, 1));
>> >
>> >      sysbus_init_mmio(dev, &s->container);
>> > +
>> > +
>> > +    for (i = 0; i < s->num_cpu; i++) {
>> > +        int ppibase = (s->num_irq - 32) + i * 32;
>> > +        sysbus_connect_irq(timerbusdev, i * 2,
>> > +                           qdev_get_gpio_in(s->gic, ppibase + 29));
>> > +        sysbus_connect_irq(timerbusdev, i * 2 + 1,
>> > +                           qdev_get_gpio_in(s->gic, ppibase + 30));
>> > +    }
>> > +
>> >      return 0;
>> >  }
>> >
>> > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs qemu-linaro-
>> 1.1.50-2012.07-modified/hw/arm/Makefile.objs
>> > --- qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs     2012-07-05
>> 16:48:28.000000000 +0200
>> > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs    2012-09-
>> 12 11:28:39.121053287 +0200
>> > @@ -1,4 +1,4 @@
>> > -obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
>> > +obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
>> > +arm_archtimer.o
>> >  obj-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o
>> > pl190.o  obj-y += versatile_pci.o  obj-y += versatile_i2c.o diff -Nupr
>> > qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c qemu-linaro-1.1.50-
>> 2012.07-modified/hw/arm_archtimer.c
>> > --- qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c       1970-01-01
>> 01:00:00.000000000 +0100
>> > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c      2012-09-
>> 12 13:21:44.676267111 +0200
>> > @@ -0,0 +1,232 @@
>> > +/*
>> > + * "Architectured" timer for A15
>> > + *
>> > + * Copyright (c) 2012 Enea Software AB
>> > + * Written by Daniel Forsgren
>> > + *
>> > + * 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.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * 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"
>> > +
>> > +/* Primitive (and imcomplete) support for A15 "architectured" timers
>>
>> incomplete
>>
>> > +
>> > +   - Only PL1 timer supported.
>> > +
>> > +   - Only minimal subset of fuctionality requred by Linux.
>>
>> functionality required
>>
>> > +
>> > +   - Only tested with single-core.
>> > +
>> > +*/
>> > +
>> > +/* control register bit assignment */ #define CTL_ENABLE  0x01
>> > +#define CTL_MASK    0x02
>> > +#define CTL_INT     0x04
>> > +
>> > +/* state of per-core timers */
>> > +typedef struct {
>> > +    ARMCPU *cpu; /* who are we serving */
>> > +    QEMUTimer *pl1_timer;
>> > +    QEMUTimer *pl2_timer;
>> > +    qemu_irq pl1_irq;
>> > +    qemu_irq pl2_irq;
>> > +    uint32_t cntkctl; /* timer pl1 control register */
>> > +    uint32_t cntp_ctl; /* pl1 physical timer control register */
>> > +    uint64_t cntvoff; /* virtual offset register */ } archtimers;
>>
>> ArchTimers, please see CODING_STYLE.
>>
>> > +
>> > +#define MAX_CPUS 4
>> > +
>> > +typedef struct {
>> > +    SysBusDevice busdev;
>> > +    archtimers archtimers[MAX_CPUS];
>> > +} arm_archtimer_state;
>>
>> ARMArchTimerState
>>
>> > +
>> > +
>> > +static int a15_cntfrq_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> > +uint64_t *value) {
>> > +    /* Let's assume that we're running at 1GHz for now. It's not
>> > +       correct, but it simplifies translation between cycles <-> ns */
>> > +    *value = 1000000000UL;
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntkctl_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> > +uint64_t *value) {
>> > +    archtimers *at = (archtimers *)(ri->opaque);
>>
>> If ri->opaque is void pointer, the cast is useless in C. Also other similar 
>> cases.
>>
>> > +    *value = at->cntkctl;
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntpct_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> > +uint64_t *value) {
>> > +    /* Let's assume that the physical count register is driven by
>> > +       vm_clock for now. As we have specified that that we're running
>> > +       at 1GHz, no translation from ns should be needed. */
>> > +    *value = qemu_get_clock_ns(vm_clock);
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntvct_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> > +uint64_t *value) {
>> > +    archtimers *at = (archtimers *)(ri->opaque);
>> > +
>> > +    /* Virtual count should subtract the virtual offfset from physical
>> > +       count? */
>> > +    *value = qemu_get_clock_ns(vm_clock) - at->cntvoff;
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntp_tval_read(CPUARMState *env, const ARMCPRegInfo
>> > +*ri, uint64_t *value) {
>> > +    archtimers *at = (archtimers *)(ri->opaque);
>> > +    *value = qemu_timer_expire_time_ns(at->pl1_timer);
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntp_tval_write(CPUARMState *env, const ARMCPRegInfo
>> > +*ri, uint64_t value) {
>> > +    archtimers *at = (archtimers *)(ri->opaque);
>> > +
>> > +    /* I assume that setting a new value means that we should clear
>> > +       any previous? */
>> > +    qemu_set_irq(at->pl1_irq, 0);
>> > +    at->cntp_ctl &= ~CTL_INT;
>> > +
>> > +    qemu_mod_timer_ns(at->pl1_timer, qemu_get_clock_ns(vm_clock) +
>> > + value);
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntp_ctl_read(CPUARMState *env, const ARMCPRegInfo
>> > +*ri, uint64_t *value) {
>> > +    archtimers *at = (archtimers *)(ri->opaque);
>> > +
>> > +    *value = at->cntp_ctl;
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static int a15_cntp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> > +                              uint64_t value) {
>> > +    archtimers *at = (archtimers *)(ri->opaque);
>> > +
>> > +    /* Copy "enable" and "mask" bits from the new value. Preserve the 
>> > rest.
>> */
>> > +    at->cntp_ctl = (at->cntp_ctl & ~(CTL_ENABLE | CTL_MASK)) | (value
>> > + & (CTL_ENABLE | CTL_MASK));
>> > +
>> > +    /* If no interrupt is asserted, or interrupt is masked, then lower 
>> > the irq. */
>> > +    if (!(at->cntp_ctl & CTL_INT) || (at->cntp_ctl & CTL_MASK))
>> > +        qemu_set_irq(at->pl1_irq, 0);
>>
>> Missing braces, please read CODING_STYLE. Please fix also other cases below.
>>
>> > +
>> > +    /* If interrupt is asserted and not masked, then raise the irq. */
>> > +    if ((at->cntp_ctl & CTL_INT) && !(at->cntp_ctl & CTL_MASK))
>> > +        qemu_set_irq(at->pl1_irq, 1);
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static const ARMCPRegInfo archtimer_cp_reginfo[] = {
>> > +
>> > +    { .name = "CNTFRQ", .cp = 15, .crn = 14, .crm = 0, .opc1 = 0, .opc2 = 
>> > 0,
>> > +      .access = PL1_R, .resetvalue = 0, .readfn = a15_cntfrq_read, },
>> > +
>> > +    { .name = "CNTKCTL", .cp = 15, .crn = 14, .crm = 1, .opc1 = 0, .opc2 
>> > = 0,
>> > +      .access = PL1_R, .resetvalue = 0, .readfn = a15_cntkctl_read,
>> > + },
>> > +
>> > +    { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, 
>> > .opc2 = 0,
>> > +      .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_tval_read,
>> > +      .writefn = a15_cntp_tval_write, },
>> > +
>> > +    { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 
>> > = 1,
>> > +      .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_ctl_read,
>> > +      .writefn = a15_cntp_ctl_write, },
>> > +
>> > +    { .name = "CNTPCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 
>> > 14,
>> .opc1 = 0, .opc2 = 0,
>> > +      .access = PL1_R, .resetvalue = 0, .readfn = a15_cntpct_read, },
>> > +
>> > +    { .name = "CNTVCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 
>> > 14,
>> .opc1 = 1, .opc2 = 0,
>> > +      .access = PL1_R, .resetvalue = 0, .readfn = a15_cntvct_read, },
>> > +
>> > +    REGINFO_SENTINEL
>> > +};
>> > +
>> > +static void pl1_timer_cb(void *opaque) {
>> > +    archtimers *at = (archtimers *)opaque;
>>
>> Here the cast is definitely useless, please remove.
>>
>> > +
>> > +    /* Set the interrupt bit in control register */
>> > +    at->cntp_ctl |= CTL_INT;
>> > +
>> > +    /* If not masked, then raise the irq */
>> > +    if (!(at->cntp_ctl & CTL_MASK))
>> > +        qemu_set_irq(at->pl1_irq, 1); }
>> > +
>> > +static int arm_archtimer_init(SysBusDevice *dev) {
>> > +    arm_archtimer_state *s = FROM_SYSBUS(arm_archtimer_state, dev);
>> > +    CPUArchState *cpu;
>> > +    int i;
>> > +
>> > +    for (cpu = first_cpu, i = 0; cpu; cpu = cpu->next_cpu, i++) {
>> > +        archtimers *at = &s->archtimers[i];
>> > +        at->pl1_timer = qemu_new_timer_ns(vm_clock, &pl1_timer_cb, at);
>> > +        sysbus_init_irq(dev, &(at->pl1_irq));
>> > +        sysbus_init_irq(dev, &(at->pl2_irq));
>> > +        s->archtimers[i].cpu = arm_env_get_cpu(cpu);
>> > +        define_arm_cp_regs_with_opaque(s->archtimers[i].cpu,
>> archtimer_cp_reginfo, (void *)at);
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static void arm_archtimer_reset(DeviceState *dev) {
>> > +    arm_archtimer_state *s =
>> > +        FROM_SYSBUS(arm_archtimer_state, sysbus_from_qdev(dev));
>> > +    int i;
>> > +
>> > +    for (i = 0; i < MAX_CPUS; i++) {
>> > +        if (s->archtimers[i].pl1_timer)
>> > +            qemu_del_timer(s->archtimers[i].pl1_timer);
>> > +        if (s->archtimers[i].pl2_timer)
>> > +            qemu_del_timer(s->archtimers[i].pl2_timer);
>> > +    }
>> > +}
>> > +
>> > +static void arm_archtimer_class_init(ObjectClass *class, void *data)
>> > +{
>> > +    DeviceClass *dc = DEVICE_CLASS(class);
>> > +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
>> > +
>> > +    sbc->init = arm_archtimer_init;
>> > +    dc->reset = arm_archtimer_reset;
>> > +}
>> > +
>> > +static TypeInfo arm_archtimer_info = {
>> > +    .name          = "arm_archtimer",
>> > +    .parent        = TYPE_SYS_BUS_DEVICE,
>> > +    .instance_size = sizeof(arm_archtimer_state),
>> > +    .class_init    = arm_archtimer_class_init,
>> > +};
>> > +
>> > +static void arm_archtimer_register_types(void)
>> > +{
>> > +    type_register_static(&arm_archtimer_info);
>> > +}
>> > +
>> > +type_init(arm_archtimer_register_types)
>> > diff -Nupr qemu-linaro-1.1.50-2012.07/target-arm/helper.c qemu-linaro-
>> 1.1.50-2012.07-modified/target-arm/helper.c
>> > --- qemu-linaro-1.1.50-2012.07/target-arm/helper.c      2012-07-05
>> 16:48:28.000000000 +0200
>> > +++ qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c     2012-09-12
>> 13:15:45.544424842 +0200
>> > @@ -1012,9 +1012,11 @@ void register_cp_regs_for_features(ARMCP
>> >      if (arm_feature(env, ARM_FEATURE_THUMB2EE)) {
>> >          define_arm_cp_regs(cpu, t2ee_cp_reginfo);
>> >      }
>> > +    /*
>>
>> Why would this be needed?
>>
>> >      if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
>> >          define_arm_cp_regs(cpu, generic_timer_cp_reginfo);
>> >      }
>> > +    */
>> >      if (arm_feature(env, ARM_FEATURE_VAPA)) {
>> >          define_arm_cp_regs(cpu, vapa_cp_reginfo);
>> >      }
>> >
>> >