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

[Leon Alrae]

On 27/10/15 17:12, Yongbok Kim wrote:
> +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;

The "pending" field is bool and "i" can be up to 63, which means this
may try to do the left-shifting above the width.

> +        }
> +        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):
> +        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):
> +        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;
> +    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;
> +        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);

The "%lx" format for data needs to be corrected as this generates
warnings on my mingw32 (I noticed the same in gcr):
hw/intc/mips_gic.c:333:23: warning: format ‘%lx’ expects argument of
type ‘long unsigned int’, but argument 5 has type ‘uint64_t’ [-Wformat]

> +        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;

If I understand correctly numintrs < 0 indicates that the "num-irq"
property has been set to an incorrect value. I believe this should do
the same thing as when it exceeds the max value, i.e. die early and loudly.

Thanks,
Leon