On 14 June 2016 at 04:09, Shannon Zhao <zhaoshengl...@huawei.com> 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
