qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-2.5 v2 3/4] mips: add Global Interrupt Contr


From: James Hogan
Subject: Re: [Qemu-devel] [PATCH for-2.5 v2 3/4] mips: add Global Interrupt Controller
Date: Fri, 30 Oct 2015 16:43:49 +0000
User-agent: Mutt/1.5.23 (2014-03-12)

Hi Yongbok,

On Tue, Oct 27, 2015 at 05:12:36PM +0000, Yongbok Kim wrote:
> The Global Interrupt Controller (GIC) is responsible for mapping each
> internal and external interrupt to the correct location for servicing.
> 
> The internal representation of registers is different from the specification
> in order to consolidate information for each GIC Interrupt Sources and Virtual
> Processors with same functionalities. For example SH_MAP00_VP00 registers are
> defined like each bit represents a VP but in this implementation the 
> equivalent
> map_vp contains VP number in integer form for ease accesses. When it is being
> accessed via read write functions an internal data is converted back into the
> original format as the specification.
> 
> Limitations:
> Level triggering only
> GIC CounterHi not implemented (Countbits = 32bits)
> DINT not implemented
> Local WatchDog, Fast Debug Channel, Perf Counter not implemented
> 
> Signed-off-by: Yongbok Kim <address@hidden>
> ---
>  hw/intc/Makefile.objs      |    1 +
>  hw/intc/mips_gic.c         |  530 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/intc/mips_gic.h |  269 ++++++++++++++++++++++
>  3 files changed, 800 insertions(+), 0 deletions(-)
>  create mode 100644 hw/intc/mips_gic.c
>  create mode 100644 include/hw/intc/mips_gic.h
> 
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 004b0c2..aedb4b3 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -30,3 +30,4 @@ obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>  obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
>  obj-$(CONFIG_S390_FLIC) += s390_flic.o
>  obj-$(CONFIG_S390_FLIC_KVM) += s390_flic_kvm.o
> +obj-$(CONFIG_MIPS_GIC) += mips_gic.o
> diff --git a/hw/intc/mips_gic.c b/hw/intc/mips_gic.c
> new file mode 100644
> index 0000000..eb19da0
> --- /dev/null
> +++ b/hw/intc/mips_gic.c
> @@ -0,0 +1,530 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2012  MIPS Technologies, Inc.  All rights reserved.
> + * Authors: Sanjay Lal <address@hidden>
> + *
> + * Copyright (C) 2015 Imagination Technologies
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "qemu/bitmap.h"
> +#include "exec/memory.h"
> +#include "sysemu/sysemu.h"
> +#include "qom/cpu.h"
> +#include "exec/address-spaces.h"
> +
> +#ifdef CONFIG_KVM

I think you can scrap the ifdef here.

> +#include "sysemu/kvm.h"
> +#include "kvm_mips.h"
> +#endif
> +
> +#include "hw/intc/mips_gic.h"
> +
> +#define TIMER_PERIOD 10 /* 10 ns period for 100 Mhz frequency */
> +
> +static inline int gic_get_current_vp(MIPSGICState *g)
> +{
> +    if (g->num_vps > 1) {
> +        return current_cpu->cpu_index;
> +    }
> +    return 0;

If this is special casing num_vps==0 to avoid thread local storage of
current_cpu when not necessary, perhaps it could make that explicit in a
comment. Otherwise it may as well just return current_cpu->cpu_index.

> +}
> +
> +static void gic_set_vp_irq(MIPSGICState *gic, int vp, int pin, int level)
> +{
> +    int ored_level = level;
> +    int i;
> +    /* ORing pending registers sharing same pin */
> +    if (!ored_level) {
> +        for (i = 0; i < gic->num_irq; i++) {
> +            if ((gic->irq_state[i].map_pin & GIC_MAP_MSK) == pin &&
> +                    gic->irq_state[i].map_vp == vp &&
> +                    gic->irq_state[i].enabled) {
> +                ored_level |= gic->irq_state[i].pending;
> +            }
> +            if (ored_level) {
> +                /* no need to iterate all interrupts */
> +                break;
> +            }
> +        }
> +        if (((gic->vps[vp].compare_map & GIC_MAP_MSK) == pin) &&
> +                (gic->vps[vp].mask & GIC_VP_MASK_CMP_MSK)) {
> +            /* ORing with local pending register (count/compare) */
> +            ored_level |= (gic->vps[vp].pend & GIC_VP_MASK_CMP_MSK) >>
> +                          GIC_VP_MASK_CMP_SHF;
> +        }
> +    }
> +
> +#ifdef CONFIG_KVM

here too, kvm_enabled should evaluate to 0 if not.

> +    if (kvm_enabled())  {
> +        kvm_mips_set_ipi_interrupt(gic->vps[vp].env, pin + 
> GIC_CPU_PIN_OFFSET,
> +                                   ored_level);
> +    }
> +#endif
> +    qemu_set_irq(gic->vps[vp].env->irq[pin + GIC_CPU_PIN_OFFSET], 
> ored_level);
> +}
> +
> +static void gic_set_irq(void *opaque, int n_IRQ, int level)
> +{
> +    MIPSGICState *gic = (MIPSGICState *) opaque;
> +    int vp = gic->irq_state[n_IRQ].map_vp;
> +    int pin = gic->irq_state[n_IRQ].map_pin & GIC_MAP_MSK;
> +
> +    gic->irq_state[n_IRQ].pending = (level != 0);
> +
> +    if (!gic->irq_state[n_IRQ].enabled) {
> +        /* GIC interrupt source disabled */
> +        return;
> +    }
> +
> +    if (vp < 0 || vp >= gic->num_vps) {
> +        return;
> +    }
> +
> +    gic_set_vp_irq(gic, vp, pin, level);
> +}
> +
> +/* GIC VP Local Timer */
> +static uint32_t gic_vp_timer_update(MIPSGICState *gic, uint32_t vp_index)
> +{
> +    uint64_t now, next;
> +    uint32_t wait;
> +
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    wait = gic->vps[vp_index].comparelo - gic->sh_counterlo -
> +            (uint32_t)(now / TIMER_PERIOD);
> +    next = now + (uint64_t)wait * TIMER_PERIOD;
> +
> +    timer_mod(gic->vps[vp_index].gic_timer->qtimer, next);
> +    return wait;
> +}
> +
> +static void gic_vp_timer_expire(MIPSGICState *gic, uint32_t vp_index)
> +{
> +    uint32_t pin;
> +    pin = (gic->vps[vp_index].compare_map & GIC_MAP_MSK);
> +    gic_vp_timer_update(gic, vp_index);
> +    gic->vps[vp_index].pend |= (1 << 1);

Maybe worth having a definition for this.

> +
> +    if (gic->vps[vp_index].pend &
> +            (gic->vps[vp_index].mask & GIC_VP_MASK_CMP_MSK)) {
> +        if (gic->vps[vp_index].compare_map & 0x80000000) {

GIC_MAP_TO_PIN_MSK?

> +            /* it is safe to set the irq high regardless of other GIC IRQs */
> +            qemu_irq_raise(gic->vps[vp_index].env->irq
> +                           [pin + GIC_CPU_PIN_OFFSET]);

does pin need range checking here and elsewhere, so as not to overflow
the array if a big (EIC style) IRQ number is set by the guest?

> +        }
> +    }
> +}
> +
> +static uint32_t gic_get_sh_count(MIPSGICState *gic)
> +{
> +    int i;
> +    if (gic->sh_config & GIC_SH_CONFIG_COUNTSTOP_MSK) {
> +        return gic->sh_counterlo;
> +    } else {
> +        uint64_t now;
> +        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        for (i = 0; i < gic->num_vps; i++) {
> +            if (timer_pending(gic->vps[i].gic_timer->qtimer)
> +                && timer_expired(gic->vps[i].gic_timer->qtimer, now)) {
> +                /* The timer has already expired.  */
> +                gic_vp_timer_expire(gic, i);
> +            }
> +        }
> +        return gic->sh_counterlo + (uint32_t)(now / TIMER_PERIOD);
> +    }
> +}
> +
> +static void gic_store_sh_count(MIPSGICState *gic, uint64_t count)
> +{
> +    int i;
> +
> +    if ((gic->sh_config & GIC_SH_CONFIG_COUNTSTOP_MSK) ||
> +            !gic->vps[0].gic_timer) {
> +        gic->sh_counterlo = count;
> +    } else {
> +        /* Store new count register */
> +        gic->sh_counterlo = count -
> +            (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / TIMER_PERIOD);
> +        /* Update timer timer */
> +        for (i = 0; i < gic->num_vps; i++) {
> +            gic_vp_timer_update(gic, i);
> +        }

This looks racy. The time is read twice, and may have increased between
those reads. It sets counter based on the first time read, but sets the
actual timout based on the second read, so e.g. if you set the time to
just before a compare, it could miss the interrupt and have to wait an
entire cycle to get another one.

E.g.
(count = 0x200, no interupt for ages)
set compare = 0x100
set count = 0x0ff (should get an interrupt almost immediately)
        time1 = 10
        counter_lo = 0xff - (u32)(time1/period)
                   = 0xfe
        (gic_vp_timer_update:)
        time2 = 30
        next = time2 + (u32)(compare - counter_lo - (u32)(time2/period))*period
             = 30 + 0xffffffff*10

I suggest reading the time only once, and passing it into
gic_vp_timer_update() so that the setting up of timeout is consistent
with the exact value written.


> +    }
> +}
> +
> +static void gic_store_vp_compare(MIPSGICState *gic, uint32_t vp_index,
> +                                  uint64_t compare)
> +{
> +    gic->vps[vp_index].comparelo = (uint32_t) compare;
> +    gic_vp_timer_update(gic, vp_index);
> +
> +    gic->vps[vp_index].pend &= ~(1 << 1);

magic

> +    if (gic->vps[vp_index].compare_map & GIC_MAP_TO_PIN_MSK) {
> +        uint32_t pin = (gic->vps[vp_index].compare_map & GIC_MAP_MSK);
> +        gic_set_vp_irq(gic, vp_index, pin, 0);
> +    }

Is there a risk of the timer expiring after gic_vp_timer_update() and
before gic_set_vp_irq, in such a way that could cause a spurious
interrupt? (or perhaps qemu timers are more synchronous than that?).

> +}
> +
> +static void gic_vp_timer_cb(void *opaque)
> +{
> +    MIPSGICTimerState *gic_timer = opaque;
> +    gic_timer->gic->sh_counterlo++;

any idea why?

> +    gic_vp_timer_expire(gic_timer->gic, gic_timer->vp_index);
> +    gic_timer->gic->sh_counterlo--;
> +}
> +
> +static void gic_timer_start_count(MIPSGICState *gic)
> +{
> +    gic_store_sh_count(gic, gic->sh_counterlo);
> +}
> +
> +static void gic_timer_stop_count(MIPSGICState *gic)
> +{
> +    int i;
> +
> +    /* Store the current value */
> +    gic->sh_counterlo +=
> +        (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / TIMER_PERIOD);
> +    for (i = 0; i < gic->num_vps; i++) {
> +        timer_del(gic->vps[i].gic_timer->qtimer);
> +    }

If timers are asynchronous (?) then this also looks potentially racy.

> +}
> +
> +static void gic_timer_init(MIPSGICState *gic, uint32_t nvps)
> +{
> +    int i;
> +    for (i = 0; i < nvps; i++) {
> +        gic->vps[i].gic_timer = g_malloc0(sizeof(MIPSGICTimerState));
> +        gic->vps[i].gic_timer->gic = gic;
> +        gic->vps[i].gic_timer->vp_index = i;
> +        gic->vps[i].gic_timer->qtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                               &gic_vp_timer_cb,
> +                                               gic->vps[i].gic_timer);
> +    }
> +    gic_store_sh_count(gic, gic->sh_counterlo);
> +}
> +
> +/* GIC Read VP Local/Other Registers */
> +static uint64_t gic_read_vp(MIPSGICState *gic, uint32_t vp_index, hwaddr 
> addr,
> +                             unsigned size)
> +{
> +    switch (addr) {
> +    case GIC_VP_CTL_OFS:
> +        return gic->vps[vp_index].ctl;
> +    case GIC_VP_PEND_OFS:
> +        gic_get_sh_count(gic);
> +        return gic->vps[vp_index].pend;
> +    case GIC_VP_MASK_OFS:
> +        return gic->vps[vp_index].mask;
> +    case GIC_VP_COMPARE_MAP_OFS:
> +        return gic->vps[vp_index].compare_map;
> +    case GIC_VP_OTHER_ADDR_OFS:
> +        return gic->vps[vp_index].other_addr;
> +    case GIC_VP_IDENT_OFS:
> +        return vp_index;
> +    case GIC_VP_COMPARE_LO_OFS:
> +        return gic->vps[vp_index].comparelo;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Read %d bytes at GIC offset LOCAL/OTHER 
> 0x%"
> +                      PRIx64 "\n", size, addr);
> +        break;
> +    }
> +    return 0;
> +}
> +
> +static uint64_t gic_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    MIPSGICState *gic = (MIPSGICState *) opaque;
> +    uint32_t vp_index = gic_get_current_vp(gic);
> +    uint64_t ret = 0;
> +    int i, base, irq_src;
> +    uint32_t other_index;
> +
> +    switch (addr) {
> +    case GIC_SH_CONFIG_OFS:
> +        ret =  gic->sh_config;
> +        break;
> +    case GIC_SH_COUNTERLO_OFS:
> +        ret = gic_get_sh_count(gic);
> +        break;
> +    case GIC_SH_COUNTERHI_OFS:
> +        ret = 0;
> +        break;
> +    case GIC_SH_PEND_31_0_OFS ... GIC_SH_PEND_255_224_OFS:
> +        base = (addr - GIC_SH_PEND_31_0_OFS) * 8;
> +        for (i = 0; i < size * 8; i++) {
> +            ret |= (gic->irq_state[base + i].pending & 1) << i;

pending is bool, so i don't think there's a need to & 1.

(and what Leon said!)

> +        }
> +        break;
> +    case GIC_SH_MASK_31_0_OFS ... GIC_SH_MASK_255_224_OFS:
> +        base = (addr - GIC_SH_MASK_31_0_OFS) * 8;
> +        for (i = 0; i < size * 8; i++) {
> +            ret |= (gic->irq_state[base + i].enabled & 1) << i;

same

> +        }
> +        break;
> +    case GIC_SH_MAP0_PIN_OFS ... GIC_SH_MAP255_PIN_OFS:
> +        irq_src = (addr - GIC_SH_MAP0_PIN_OFS) / 4;
> +        ret = gic->irq_state[irq_src].map_pin;
> +        break;
> +    case GIC_SH_MAP0_VP31_0_OFS ... GIC_SH_MAP255_VP63_32_OFS:
> +        irq_src = (addr - GIC_SH_MAP0_VP31_0_OFS) / 32;
> +        if ((gic->irq_state[irq_src].map_vp) >= 0) {
> +            ret = 1 << (gic->irq_state[irq_src].map_vp);
> +        } else {
> +            ret = 0;
> +        }
> +        break;
> +    /* VP-Local Register */
> +    case GIC_VPLOCAL_BASE_ADDR ... (GIC_VPLOCAL_BASE_ADDR + 
> GIC_VL_BRK_GROUP):

maybe this'd be a good place to use those otherwise unused duplicate
definitions for the base and sizes of these regions.

> +        ret = gic_read_vp(gic, vp_index, addr - GIC_VPLOCAL_BASE_ADDR, size);
> +        break;
> +    /* VP-Other Register */
> +    case GIC_VPOTHER_BASE_ADDR ... (GIC_VPOTHER_BASE_ADDR + 
> GIC_VL_BRK_GROUP):

same

> +        other_index = gic->vps[vp_index].other_addr;
> +        ret = gic_read_vp(gic, other_index, addr - GIC_VPOTHER_BASE_ADDR,
> +                           size);
> +        break;
> +    /* User-Mode Visible section */
> +    case GIC_USERMODE_BASE_ADDR + GIC_USER_MODE_COUNTERLO:
> +        ret = gic_get_sh_count(gic);
> +        break;

Need to implement the hi register too, even if always zero?

E.g. this kernel patch appears to use it in read_gic_count:

http://patchwork.linux-mips.org/patch/11338/

> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Read %d bytes at GIC offset 0x%" PRIx64 
> "\n",
> +                      size, addr);
> +        break;
> +    }
> +    return ret;
> +}
> +
> +/* GIC Write VP Local/Other Registers */
> +static void gic_write_vp(MIPSGICState *gic, uint32_t vp_index, hwaddr addr,
> +                              uint64_t data, unsigned size)
> +{
> +    switch (addr) {
> +    case GIC_VP_CTL_OFS:
> +        gic->vps[vp_index].ctl &= ~GIC_VP_CTL_EIC_MODE_MSK;
> +        gic->vps[vp_index].ctl |= data & GIC_VP_CTL_EIC_MODE_MSK;

We don't appear to emulate EIC mode correctly (max 8 cpu pins for
example), so maybe best leaving this read-only?

> +        break;
> +    case GIC_VP_RMASK_OFS:
> +        gic->vps[vp_index].mask &= ~(data & GIC_VP_SET_RESET_MSK) &
> +                                   GIC_VP_SET_RESET_MSK;
> +        break;
> +    case GIC_VP_SMASK_OFS:
> +        gic->vps[vp_index].mask |= (data & GIC_VP_SET_RESET_MSK);
> +        break;
> +    case GIC_VP_COMPARE_MAP_OFS:
> +        gic->vps[vp_index].compare_map = data & GIC_MAP_TO_PIN_REG_MSK;
> +        break;
> +    case GIC_VP_OTHER_ADDR_OFS:
> +        if (data < gic->num_vps) {
> +            gic->vps[vp_index].other_addr = data;
> +        }
> +        break;
> +    case GIC_VP_COMPARE_LO_OFS:
> +        gic_store_vp_compare(gic, vp_index, data);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Write %d bytes at GIC offset LOCAL/OTHER "
> +                      "0x%" PRIx64" 0x%08lx\n", size, addr, data);
> +        break;
> +    }
> +}
> +
> +static void gic_write(void *opaque, hwaddr addr, uint64_t data, unsigned 
> size)
> +{
> +    int intr;
> +    MIPSGICState *gic = (MIPSGICState *) opaque;
> +    uint32_t vp_index = gic_get_current_vp(gic);
> +    int i, base, irq_src;
> +    uint32_t pre, other_index;
> +
> +    switch (addr) {
> +    case GIC_SH_CONFIG_OFS:
> +        pre = gic->sh_config;
> +        gic->sh_config = (gic->sh_config & ~GIC_SH_CONFIG_COUNTSTOP_MSK) |
> +                         (data & GIC_SH_CONFIG_COUNTSTOP_MSK);
> +        if (pre != gic->sh_config) {
> +            if ((gic->sh_config & GIC_SH_CONFIG_COUNTSTOP_MSK)) {
> +                gic_timer_stop_count(gic);
> +            } else {
> +                gic_timer_start_count(gic);
> +            }
> +        }
> +        break;
> +    case GIC_SH_COUNTERLO_OFS:
> +        if (gic->sh_config & GIC_SH_CONFIG_COUNTSTOP_MSK) {
> +            gic_store_sh_count(gic, data);

ah right, only writable when stopped. Still, gic_store_sh_count can be
called when starting the count, so the race in that function is still
there.

> +        }
> +        break;
> +    case GIC_SH_RMASK_31_0_OFS ... GIC_SH_RMASK_255_224_OFS:
> +        base = (addr - GIC_SH_RMASK_31_0_OFS) * 8;
> +        for (i = 0; i < size * 8; i++) {
> +            gic->irq_state[base + i].enabled &= !((data >> i) & 1);

bitwise ops on bools make me uncomfortable :-P

> +        }
> +        break;
> +    case GIC_SH_WEDGE_OFS:
> +        /* Figure out which VP/HW Interrupt this maps to */
> +        intr = data & 0x7FFFFFFF;

maybe use ~GIC_SH_WEDGE_RW_MSK?

> +        /* Mask/Enabled Checks */
> +        if (intr < gic->num_irq) {
> +            if (data & GIC_SH_WEDGE_RW_MSK) {
> +                gic_set_irq(gic, intr, 1);
> +            } else {
> +                gic_set_irq(gic, intr, 0);
> +            }
> +        }
> +        break;
> +    case GIC_SH_SMASK_31_0_OFS ... GIC_SH_SMASK_255_224_OFS:
> +        base = (addr - GIC_SH_SMASK_31_0_OFS) * 8;
> +        for (i = 0; i < size * 8; i++) {
> +            gic->irq_state[base + i].enabled |= (data >> i) & 1;
> +        }
> +        break;
> +    case GIC_SH_MAP0_PIN_OFS ... GIC_SH_MAP255_PIN_OFS:
> +        irq_src = (addr - GIC_SH_MAP0_PIN_OFS) / 4;
> +        gic->irq_state[irq_src].map_pin = data;

should this mask out reserved bits?

> +        break;
> +    case GIC_SH_MAP0_VP31_0_OFS ... GIC_SH_MAP255_VP63_32_OFS:
> +        irq_src = (addr - GIC_SH_MAP0_VP31_0_OFS) / 32;
> +        gic->irq_state[irq_src].map_vp = (data) ? ctz64(data) : -1;

spurious brackets around data.

> +        break;
> +    case GIC_VPLOCAL_BASE_ADDR ... (GIC_VPLOCAL_BASE_ADDR + 
> GIC_VL_BRK_GROUP):
> +        gic_write_vp(gic, vp_index, addr - GIC_VPLOCAL_BASE_ADDR, data, 
> size);
> +        break;
> +    case GIC_VPOTHER_BASE_ADDR ... (GIC_VPOTHER_BASE_ADDR + 
> GIC_VL_BRK_GROUP):
> +        other_index = gic->vps[vp_index].other_addr;
> +        gic_write_vp(gic, other_index, addr - GIC_VPOTHER_BASE_ADDR,
> +                      data, size);
> +        break;
> +    case GIC_USERMODE_BASE_ADDR + GIC_USER_MODE_COUNTERLO:
> +    case GIC_USERMODE_BASE_ADDR + GIC_USER_MODE_COUNTERHI:
> +        /* do nothing. Read-only section */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Write %d bytes at GIC offset 0x%" PRIx64
> +                      " 0x%08lx\n", size, addr, data);
> +        break;
> +    }
> +}
> +
> +static void gic_reset(void *opaque)
> +{
> +    int i;
> +    MIPSGICState *gic = (MIPSGICState *) opaque;
> +    int numintrs = (gic->num_irq / 8) - 1;
> +
> +    numintrs =  (numintrs < 0) ? 0 : numintrs;
> +
> +    gic->sh_config      = (1 << GIC_SH_CONFIG_COUNTSTOP_SHF) |
> +                          (numintrs << GIC_SH_CONFIG_NUMINTRS_SHF) |
> +                          gic->num_vps;
> +    gic->sh_counterlo   = 0;


count stop reset value is 0, as is counterlo/hi. Need to start counter
so that counter becomes zero?

> +
> +    for (i = 0; i < gic->num_vps; i++) {
> +        gic->vps[i].ctl         = 0x0;
> +        gic->vps[i].pend        = 0x0;

curiously these tend to be reset to 1 according to manuals.

> +        gic->vps[i].mask        = 0

these too

> +        gic->vps[i].compare_map = GIC_MAP_TO_PIN_MSK;

newer cores reset counter to pin 5.

> +        gic->vps[i].comparelo   = 0x0;
> +        gic->vps[i].other_addr  = 0x0;
> +    }
> +
> +    for (i = 0; i < gic->num_irq; i++) {
> +        gic->irq_state[i].enabled        = false;
> +        gic->irq_state[i].pending        = false;
> +        gic->irq_state[i].map_pin        = GIC_MAP_TO_PIN_MSK;
> +        gic->irq_state[i].map_vp         = -1;
> +    }
> +}
> +
> +static const MemoryRegionOps gic_ops = {
> +    .read = gic_read,
> +    .write = gic_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .max_access_size = 8,
> +    },
> +};
> +
> +static void mips_gic_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    MIPSGICState *s = MIPS_GIC(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &gic_ops, s,
> +                          "mips-gic", GIC_ADDRSPACE_SZ);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    qemu_register_reset(gic_reset, s);
> +}
> +
> +static void mips_gic_realize(DeviceState *dev, Error **errp)
> +{
> +    MIPSGICState *s = MIPS_GIC(dev);
> +    CPUState *cs = first_cpu;
> +    int i;
> +
> +    if (s->num_vps > GIC_MAX_VPS) {
> +        error_setg(errp, "Exceed maximum CPUs %d", s->num_vps);
> +        return;
> +    }
> +    if (s->num_irq > GIC_MAX_INTRS) {
> +        error_setg(errp, "Exceed maximum GIC IRQs %d", s->num_irq);
> +        return;
> +    }
> +
> +    s->vps = g_new(MIPSGICVPState, s->num_vps);
> +    s->irq_state = g_new(MIPSGICIRQState, s->num_irq);
> +
> +    /* Register the env for all VPs with the GIC */
> +    for (i = 0; i < s->num_vps; i++) {
> +        if (cs != NULL) {
> +            s->vps[i].env = cs->env_ptr;
> +            cs = CPU_NEXT(cs);
> +        } else {
> +            fprintf(stderr, "Unable to initialize GIC - CPUState for "
> +                    "CPU #%d not valid!", i);

strings shouldn't be split

> +            return;
> +        }
> +    }
> +
> +    gic_timer_init(s, s->num_vps);
> +
> +    qdev_init_gpio_in(dev, gic_set_irq, s->num_irq);
> +    for (i = 0; i < s->num_irq; i++) {
> +        s->irq_state[i].irq = qdev_get_gpio_in(dev, i);
> +    }
> +}
> +
> +static Property mips_gic_properties[] = {
> +    DEFINE_PROP_INT32("num-vp", MIPSGICState, num_vps, 1),
> +    DEFINE_PROP_INT32("num-irq", MIPSGICState, num_irq, 256),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void mips_gic_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = mips_gic_properties;
> +    dc->realize = mips_gic_realize;
> +}
> +
> +static const TypeInfo mips_gic_info = {
> +    .name          = TYPE_MIPS_GIC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MIPSGICState),
> +    .instance_init = mips_gic_init,
> +    .class_init    = mips_gic_class_init,
> +};
> +
> +static void mips_gic_register_types(void)
> +{
> +    type_register_static(&mips_gic_info);
> +}
> +
> +type_init(mips_gic_register_types)
> diff --git a/include/hw/intc/mips_gic.h b/include/hw/intc/mips_gic.h
> new file mode 100644
> index 0000000..d0d6d4b
> --- /dev/null
> +++ b/include/hw/intc/mips_gic.h
> @@ -0,0 +1,269 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2000, 07 MIPS Technologies, Inc.
> + * Copyright (C) 2015 Imagination Technologies
> + *
> + */
> +#ifndef _MIPS_GIC_H
> +#define _MIPS_GIC_H
> +
> +/*
> + * GIC Specific definitions
> + */
> +
> +/* The MIPS default location */
> +#define GIC_BASE_ADDR           0x1bdc0000ULL
> +#define GIC_ADDRSPACE_SZ        (128 * 1024)
> +
> +/* GIC Address Space Offsets */
> +#define GIC_SHARED_BASE_ADDR    0x0000
> +#define GIC_VPLOCAL_BASE_ADDR   0x8000
> +#define GIC_VPOTHER_BASE_ADDR   0xC000
> +#define GIC_USERMODE_BASE_ADDR  0x10000
> +
> +/* Constants */
> +#define GIC_POL_POS     1
> +#define GIC_POL_NEG     0
> +#define GIC_TRIG_EDGE   1
> +#define GIC_TRIG_LEVEL  0
> +
> +#define MSK(n)              ((1 << (n)) - 1)

I'm guessing that should be 1u, since the result is sometimes left
shifted by 31.

> +
> +/* GIC Address Space */
> +#define SHARED_SECTION_OFS          0x0000
> +#define SHARED_SECTION_SIZE         0x8000
> +#define VP_LOCAL_SECTION_OFS        0x8000
> +#define VP_LOCAL_SECTION_SIZE       0x4000
> +#define VP_OTHER_SECTION_OFS        0xc000
> +#define VP_OTHER_SECTION_SIZE       0x4000
> +#define USM_VISIBLE_SECTION_OFS     0x10000
> +#define USM_VISIBLE_SECTION_SIZE    0x10000

duplicates the definitions above?

> +
> +/* Register Map for Shared Section */
> +
> +#define GIC_SH_CONFIG_OFS           0x0000
> +
> +/* Shared Global Counter */
> +#define GIC_SH_COUNTERLO_OFS        0x0010
> +#define GIC_SH_COUNTERHI_OFS        0x0014
> +#define GIC_SH_REVISIONID_OFS       0x0020
> +
> +/* Interrupt Polarity */
> +#define GIC_SH_POL_31_0_OFS         0x0100
> +#define GIC_SH_POL_63_32_OFS        0x0104
> +#define GIC_SH_POL_95_64_OFS        0x0108
> +#define GIC_SH_POL_127_96_OFS       0x010c
> +#define GIC_SH_POL_159_128_OFS      0x0110
> +#define GIC_SH_POL_191_160_OFS      0x0114
> +#define GIC_SH_POL_223_192_OFS      0x0118
> +#define GIC_SH_POL_255_224_OFS      0x011c
> +
> +/* Edge/Level Triggering */
> +#define GIC_SH_TRIG_31_0_OFS        0x0180
> +#define GIC_SH_TRIG_63_32_OFS       0x0184
> +#define GIC_SH_TRIG_95_64_OFS       0x0188
> +#define GIC_SH_TRIG_127_96_OFS      0x018c
> +#define GIC_SH_TRIG_159_128_OFS     0x0190
> +#define GIC_SH_TRIG_191_160_OFS     0x0194
> +#define GIC_SH_TRIG_223_192_OFS     0x0198
> +#define GIC_SH_TRIG_255_224_OFS     0x019c
> +
> +/* Dual Edge Triggering */
> +#define GIC_SH_DUAL_31_0_OFS        0x0200
> +#define GIC_SH_DUAL_63_32_OFS       0x0204
> +#define GIC_SH_DUAL_95_64_OFS       0x0208
> +#define GIC_SH_DUAL_127_96_OFS      0x020c
> +#define GIC_SH_DUAL_159_128_OFS     0x0210
> +#define GIC_SH_DUAL_191_160_OFS     0x0214
> +#define GIC_SH_DUAL_223_192_OFS     0x0218
> +#define GIC_SH_DUAL_255_224_OFS     0x021c
> +
> +/* Set/Clear corresponding bit in Edge Detect Register */
> +#define GIC_SH_WEDGE_OFS            0x0280
> +
> +/* Reset Mask - Disables Interrupt */
> +#define GIC_SH_RMASK_31_0_OFS       0x0300
> +#define GIC_SH_RMASK_63_32_OFS      0x0304
> +#define GIC_SH_RMASK_95_64_OFS      0x0308
> +#define GIC_SH_RMASK_127_96_OFS     0x030c
> +#define GIC_SH_RMASK_159_128_OFS    0x0310
> +#define GIC_SH_RMASK_191_160_OFS    0x0314
> +#define GIC_SH_RMASK_223_192_OFS    0x0318
> +#define GIC_SH_RMASK_255_224_OFS    0x031c
> +
> +/* Set Mask (WO) - Enables Interrupt */
> +#define GIC_SH_SMASK_31_0_OFS       0x0380
> +#define GIC_SH_SMASK_63_32_OFS      0x0384
> +#define GIC_SH_SMASK_95_64_OFS      0x0388
> +#define GIC_SH_SMASK_127_96_OFS     0x038c

> +#define GIC_SH_SMASK_159_128_OFS    0x0390
> +#define GIC_SH_SMASK_191_160_OFS    0x0394
> +#define GIC_SH_SMASK_223_192_OFS    0x0398
> +#define GIC_SH_SMASK_255_224_OFS    0x039c
> +
> +/* Global Interrupt Mask Register (RO) - Bit Set == Interrupt enabled */
> +#define GIC_SH_MASK_31_0_OFS        0x0400
> +#define GIC_SH_MASK_63_32_OFS       0x0404
> +#define GIC_SH_MASK_95_64_OFS       0x0408
> +#define GIC_SH_MASK_127_96_OFS      0x040c
> +#define GIC_SH_MASK_159_128_OFS     0x0410
> +#define GIC_SH_MASK_191_160_OFS     0x0414
> +#define GIC_SH_MASK_223_192_OFS     0x0418
> +#define GIC_SH_MASK_255_224_OFS     0x041c
> +
> +/* Pending Global Interrupts (RO) */
> +#define GIC_SH_PEND_31_0_OFS        0x0480
> +#define GIC_SH_PEND_63_32_OFS       0x0484
> +#define GIC_SH_PEND_95_64_OFS       0x0488
> +#define GIC_SH_PEND_127_96_OFS      0x048c
> +#define GIC_SH_PEND_159_128_OFS     0x0490
> +#define GIC_SH_PEND_191_160_OFS     0x0494
> +#define GIC_SH_PEND_223_192_OFS     0x0498
> +#define GIC_SH_PEND_255_224_OFS     0x049c
> +
> +#define GIC_SH_MAP0_PIN_OFS         0x0500
> +#define GIC_SH_MAP255_PIN_OFS       0x08fc
> +
> +#define GIC_SH_MAP0_VP31_0_OFS      0x2000
> +#define GIC_SH_MAP255_VP63_32_OFS   0x3fe4
> +
> +/* Register Map for Local Section */
> +#define GIC_VP_CTL_OFS              0x0000
> +#define GIC_VP_PEND_OFS             0x0004
> +#define GIC_VP_MASK_OFS             0x0008
> +#define GIC_VP_RMASK_OFS            0x000c
> +#define GIC_VP_SMASK_OFS            0x0010
> +#define GIC_VP_WD_MAP_OFS           0x0040
> +#define GIC_VP_COMPARE_MAP_OFS      0x0044
> +#define GIC_VP_TIMER_MAP_OFS        0x0048

May as well have the GIC_VP_FDC_MAP_OFS in there too @ 0x004c.

> +#define GIC_VP_PERFCTR_MAP_OFS      0x0050
> +#define GIC_VP_SWINT0_MAP_OFS       0x0054
> +#define GIC_VP_SWINT1_MAP_OFS       0x0058
> +#define GIC_VP_OTHER_ADDR_OFS       0x0080
> +#define GIC_VP_IDENT_OFS            0x0088
> +#define GIC_VP_WD_CONFIG0_OFS       0x0090
> +#define GIC_VP_WD_COUNT0_OFS        0x0094
> +#define GIC_VP_WD_INITIAL0_OFS      0x0098
> +#define GIC_VP_COMPARE_LO_OFS       0x00a0
> +#define GIC_VP_COMPARE_HI_OFS       0x00a4
> +#define GIC_VL_BRK_GROUP            0x3080
> +
> +/* User-Mode Visible Section Register */
> +/* Read-only alias for GIC Shared CounterLo */
> +#define GIC_USER_MODE_COUNTERLO     0x0000
> +/* Read-only alias for GIC Shared CounterHi */
> +#define GIC_USER_MODE_COUNTERHI     0x0004
> +
> +/* Masks */
> +#define GIC_SH_CONFIG_COUNTSTOP_SHF     28
> +#define GIC_SH_CONFIG_COUNTSTOP_MSK     (MSK(1) << 
> GIC_SH_CONFIG_COUNTSTOP_SHF)
> +
> +#define GIC_SH_CONFIG_COUNTBITS_SHF     24
> +#define GIC_SH_CONFIG_COUNTBITS_MSK     (MSK(4) << 
> GIC_SH_CONFIG_COUNTBITS_SHF)
> +
> +#define GIC_SH_CONFIG_NUMINTRS_SHF      16
> +#define GIC_SH_CONFIG_NUMINTRS_MSK      (MSK(8) << 
> GIC_SH_CONFIG_NUMINTRS_SHF)
> +

a few unnecessary newlines here IMO

> +#define GIC_SH_CONFIG_NUMVPS_SHF        0
> +#define GIC_SH_CONFIG_NUMVPS_MSK        (MSK(8) << GIC_SH_CONFIG_NUMVPS_SHF)

This is called PVPES (interAptiv, P5600) or PVPS (I6400), and only
interAptiv has it as 8 bits (the others 7 bits). interAptiv only defines
values 0 to 8, so IMO better to go with the newer definition.

> +
> +#define GIC_SH_WEDGE_RW_SHF             31
> +#define GIC_SH_WEDGE_RW_MSK             (MSK(1) << GIC_SH_WEDGE_RW_SHF)
> +
> +#define GIC_MAP_TO_PIN_SHF              31
> +#define GIC_MAP_TO_PIN_MSK              (MSK(1) << GIC_MAP_TO_PIN_SHF)
> +#define GIC_MAP_TO_NMI_SHF              30
> +#define GIC_MAP_TO_NMI_MSK              (MSK(1) << GIC_MAP_TO_NMI_SHF)
> +#define GIC_MAP_TO_YQ_SHF               29
> +#define GIC_MAP_TO_YQ_MSK               (MSK(1) << GIC_MAP_TO_YQ_SHF)
> +#define GIC_MAP_SHF                     0
> +#define GIC_MAP_MSK                     (MSK(6) << GIC_MAP_SHF)
> +#define GIC_MAP_TO_PIN_REG_MSK          \
> +    (GIC_MAP_TO_PIN_MSK | GIC_MAP_TO_NMI_MSK | GIC_MAP_TO_YQ_MSK | 
> GIC_MAP_MSK)

Note, only MT cores (i.e. interAptiv) have the YQ bit.

> +
> +/* GIC_VP_CTL Masks */

There was also a SWINT_RTBL and FDC_RTBL in bits 3 and 4.

> +#define GIC_VP_CTL_PERFCNT_RTBL_SHF     2
> +#define GIC_VP_CTL_PERFCNT_RTBL_MSK     (MSK(1) << 
> GIC_VP_CTL_PERFCNT_RTBL_SHF)
> +#define GIC_VP_CTL_TIMER_RTBL_SHF       1
> +#define GIC_VP_CTL_TIMER_RTBL_MSK       (MSK(1) << GIC_VP_CTL_TIMER_RTBL_SHF)
> +#define GIC_VP_CTL_EIC_MODE_SHF         0
> +#define GIC_VP_CTL_EIC_MODE_MSK         (MSK(1) << GIC_VP_CTL_EIC_MODE_SHF)
> +
> +/* GIC_VP_MASK Masks */
> +#define GIC_VP_MASK_WD_SHF          0
> +#define GIC_VP_MASK_WD_MSK          (MSK(1) << GIC_VP_MASK_WD_SHF)
> +#define GIC_VP_MASK_CMP_SHF         1
> +#define GIC_VP_MASK_CMP_MSK         (MSK(1) << GIC_VP_MASK_CMP_SHF)
> +#define GIC_VP_MASK_TIMER_SHF       2
> +#define GIC_VP_MASK_TIMER_MSK       (MSK(1) << GIC_VP_MASK_TIMER_SHF)
> +#define GIC_VP_MASK_PERFCNT_SHF     3
> +#define GIC_VP_MASK_PERFCNT_MSK     (MSK(1) << GIC_VP_MASK_PERFCNT_SHF)
> +#define GIC_VP_MASK_SWINT0_SHF      4
> +#define GIC_VP_MASK_SWINT0_MSK      (MSK(1) << GIC_VP_MASK_SWINT0_SHF)
> +#define GIC_VP_MASK_SWINT1_SHF      5
> +#define GIC_VP_MASK_SWINT1_MSK      (MSK(1) << GIC_VP_MASK_SWINT1_SHF)
> +#define GIC_VP_MASK_FDC_SHF         6
> +#define GIC_VP_MASK_FDC_MSK         (MSK(1) << GIC_VP_MASK_FDC_SHF)

The other register defs go in decreasing order of bit.

> +#define GIC_VP_SET_RESET_MSK        (MSK(7) << GIC_VP_MASK_WD_SHF)
> +
> +#define GIC_CPU_PIN_OFFSET          2
> +
> +#define TYPE_MIPS_GIC "mips-gic"
> +#define MIPS_GIC(obj) OBJECT_CHECK(MIPSGICState, (obj), TYPE_MIPS_GIC)
> +
> +/* Support up to 32 VPs and 256 IRQs */
> +#define GIC_MAX_VPS             32
> +#define GIC_MAX_INTRS           256
> +
> +typedef struct MIPSGICState MIPSGICState;
> +typedef struct MIPSGICTimerState MIPSGICTimerState;
> +typedef struct MIPSGICIRQState MIPSGICIRQState;
> +typedef struct MIPSGICVPState MIPSGICVPState;
> +
> +struct MIPSGICTimerState {
> +    QEMUTimer *qtimer;
> +    uint32_t vp_index;
> +    MIPSGICState *gic;
> +};
> +
> +struct MIPSGICIRQState {
> +    bool enabled;
> +    bool pending;

seems a little wasteful compared to masks, but ok.

> +    uint32_t map_pin;
> +    int32_t map_vp;
> +    qemu_irq irq;
> +};
> +
> +struct MIPSGICVPState {
> +    uint32_t ctl;
> +    uint32_t pend;
> +    uint32_t mask;
> +    uint32_t compare_map;
> +    uint32_t comparelo;
> +    uint32_t other_addr;
> +
> +    CPUMIPSState *env;
> +    MIPSGICTimerState *gic_timer;

any reason to have the gic timers as pointers to separately allocated
memory rather than putting it straight in the struct? AFAICT its not
really that dynamic.

> +};
> +
> +struct MIPSGICState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +
> +    /* Shared Section Registers */
> +    uint32_t sh_config;
> +    uint32_t sh_counterlo;
> +    MIPSGICIRQState *irq_state;
> +
> +    /* VP Local/Other Section Registers */
> +    MIPSGICVPState *vps;
> +
> +    int32_t num_vps;
> +    int32_t num_irq;
> +};
> +
> +#endif /* _MIPS_GIC_H */
> -- 
> 1.7.1
> 

Cheers
James

Attachment: signature.asc
Description: Digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]