Re: [Qemu-arm] [Qemu-devel] [PATCH v2 12/22] hw/intc/arm_gicv3: Implement GICv3 redistributor registers

[Peter Maydell]

On 14 June 2016 at 04:09, Shannon Zhao <address@hidden> wrote:
>
>
> On 2016/5/26 22:55, Peter Maydell wrote:
>> Implement the redistributor registers of a GICv3.

>> +    case GICR_ISENABLER0:
>> +    case GICR_ICENABLER0:
>> +        *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_ienabler0);

> GICv3 SPEC says when GICD_CTLR.DS == 0, for GICD_ICENABLER<n> bits
> corresponding to Group 0 and Secure Group 1 interrupts are RAZ/WI to
> Non-secure accesses. But for GICR_ICENABLER0 all bits are RAZ/WI to
> Non-secure accesses. It doesn't care the interrupt Group. If so,
> gicr_read_bitmap_reg() wrongly uses the return value of mask_group() as
> the mask.

The spec says for GICR_ICENABLER0 "When GICD_CTLR.DS == 0, bits corresponding
to Secure SGIs and PPIs are RAZ/WI to Non-secure accesses". So we are
correct to look at the group bit I think.

>> +        return MEMTX_OK;
>> +    case GICR_ISPENDR0:
>> +    case GICR_ICPENDR0:
>> +    {
>> +        /* The pending register reads as the logical OR of the pending
>> +         * latch and the input line level for level-triggered interrupts.
>> +         */
>> +        uint32_t val = cs->gicr_ipendr0 | (~cs->edge_trigger & cs->level);
>> +        *data = gicr_read_bitmap_reg(cs, attrs, val);
>> +        return MEMTX_OK;
>> +    }
>> +    case GICR_ISACTIVER0:
>> +    case GICR_ICACTIVER0:
>> +        *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_iactiver0);
>> +        return MEMTX_OK;
>> +    case GICR_IPRIORITYR ... GICR_IPRIORITYR + 0x1f:
>> +    {
>> +        int i, irq = offset - GICR_IPRIORITYR;
>> +        uint32_t value = 0;
>> +
>> +        for (i = irq + 3; i >= irq; i--, value <<= 8) {
>> +            value |= gicr_read_ipriorityr(cs, attrs, i);
>> +        }
>> +        *data = value;
>> +        return MEMTX_OK;
>> +    }
>> +    case GICR_ICFGR0:
>> +    case GICR_ICFGR1:
>> +    {
>> +        /* Our edge_trigger bitmap is one bit per irq; take the correct
>> +         * half of it, and spread it out into the odd bits.
>> +         */
>> +        uint32_t value;
>> +
>> +        value = cs->edge_trigger & mask_group(cs, attrs);
> For GICR_ICFGR0, it doesn't need to use mask_group() as mask because the
> SPEC doesn't say it's RAZ/WI to a Group 0 or Secure Group 1 interrupt.

This is a bug in the GICv3 spec which will be fixed in a newer
rev of the document. GICR_ICFGR0 should say that "When GICD_CTLR.DS==0,
a register bit that corresponds to a Group 0 or Secure Group 1
interrupt is RAZ/WI to Non-secure accesses".

>> +        value = extract32(value, (offset == GICR_ICFGR1) ? 16 : 0, 16);
>> +        value = half_shuffle32(value) << 1;
>> +        *data = value;
>> +        return MEMTX_OK;
>> +    }
>> +    case GICR_IGRPMODR0:
>> +        if ((cs->gic->gicd_ctlr & GICD_CTLR_DS) || !attrs.secure) {
>> +            /* RAZ/WI if security disabled, or if
>> +             * security enabled and this is an NS access
>> +             */
>> +            *data = 0;
>> +            return MEMTX_OK;
>> +        }
>> +        *data = cs->gicr_igrpmodr0;
>> +        return MEMTX_OK;
>> +    case GICR_NSACR:
>> +        if (!attrs.secure && !(cs->gic->gicd_ctlr & GICD_CTLR_DS)) {
>> +            *data = 0;
>> +            return MEMTX_OK;
>> +        }
>> +        *data = cs->gicr_nsacr;
> Look like this is not consistent with the SPEC.
> The SPEC says
> "When GICD_CTLR.DS == 1, this register is RAZ/WI.
> When GICD_CTLR.DS == 0, this register is Secure, and is RAZ/WI to
> Non-secure accesses."
>
> So when GICD_CTLR.DS == 1, it should make *data = 0.

Agreed. This should have the same kind of condition as GICR_IGRPMODR0:

        if ((cs->gic->gicd_ctlr & GICD_CTLR_DS) || !attrs.secure) {
            /* RAZ/WI if security disabled, or if
             * security enabled and this is an NS access
             */
            *data = 0;
            return MEMTX_OK;

(similarly for writes)

thanks
-- PMM