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

[Shannon Zhao]

On 2016/5/26 22:55, Peter Maydell wrote:
> +static uint8_t gicd_read_ipriorityr(GICv3State *s, MemTxAttrs attrs, int irq)
> +{
> +    /* Read the value of GICD_IPRIORITYR<n> for the specified interrupt,
> +     * honouring security state (these are RAZ/WI for Group 0 or Secure
> +     * Group 1 interrupts).
> +     */
> +    uint32_t prio;
> +
> +    if (irq < GIC_INTERNAL || irq >= s->num_irq) {
> +        return 0;
> +    }
> +
> +    prio = s->gicd_ipriority[irq];
> +
> +    if (!attrs.secure && !(s->gicd_ctlr & GICD_CTLR_DS)) {
> +        if (!gicv3_gicd_group_test(s, irq)) {
> +            /* Fields for Group 0 or Secure Group 1 interrupts are RAZ/WI */
Here this check assure this interrupt belongs to Group 0 and NS access
is not permitted, so it should return 0. But it doesn't say anything
about Secure Group 1.

> +            return 0;
> +        }
> +        /* NS view of the interrupt priority */
> +        prio = (prio << 1) & 0xff;
> +    }
So maybe here it should check if attrs.secure is true and the Group is
1, then return 0.

> +    return prio;
> +}
> +
> +static void gicd_write_ipriorityr(GICv3State *s, MemTxAttrs attrs, int irq,
> +                                  uint8_t value)
> +{
> +    /* Write the value of GICD_IPRIORITYR<n> for the specified interrupt,
> +     * honouring security state (these are RAZ/WI for Group 0 or Secure
> +     * Group 1 interrupts).
> +     */
> +    if (irq < GIC_INTERNAL || irq >= s->num_irq) {
> +        return;
> +    }
> +
> +    if (!attrs.secure && !(s->gicd_ctlr & GICD_CTLR_DS)) {
> +        if (!gicv3_gicd_group_test(s, irq)) {
> +            /* Fields for Group 0 or Secure Group 1 interrupts are RAZ/WI */
Same here.

> +            return;
> +        }
> +        /* NS view of the interrupt priority */
> +        value = 0x80 | (value >> 1);
> +    }
> +    s->gicd_ipriority[irq] = value;
> +}

-- 
Shannon