Re: [Qemu-devel] [PATCH v3 01/20] intc/arm_gic: Implement write to GICD_ISACTIVERn and GICD_ICACTIVERn registers

[Peter Maydell]

On 29 June 2018 at 14:29, Luc Michel <address@hidden> wrote:
> Implement write access to GICD_ISACTIVERn and GICD_ICACTIVERn registers
> in the GICv2. Those registers allow to set or clear the active state of
> an IRQ in the distributor.
>
> Signed-off-by: Luc Michel <address@hidden>
> ---
>  hw/intc/arm_gic.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index ea0323f969..5755a4fb2c 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -982,9 +982,46 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
>              }
>          }
> +    } else if (offset < 0x380) {
> +        /* Interrupt Set Active.  */

These are read-only for the GICv1 and 11MPCore, so we need
to guard this code with a check on s->revision == 2:
           if (s->revision != 2) {
               goto bad_reg;
           }

> +        irq = (offset - 0x300) * 8 + GIC_BASE_IRQ;
> +        if (irq >= s->num_irq) {
> +            goto bad_reg;
> +        }
> +
> +        /* This register is banked per-cpu for PPIs */
> +        int cm = irq < GIC_INTERNAL ? (1 << cpu) : ALL_CPU_MASK;
> +
> +        for (i = 0; i < 8; i++) {
> +            if (s->security_extn && !attrs.secure &&
> +                !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
> +                continue; /* Ignore Non-secure access of Group0 IRQ */
> +            }
> +
> +            if (value & (1 << i)) {
> +                GIC_DIST_SET_ACTIVE(irq + i, cm);

You don't introduce this macro until patch 2, so this patch
has to go after that, so that we still compile at all points
in the patch sequence.

> +            }
> +        }
>      } else if (offset < 0x400) {
> -        /* Interrupt Active.  */
> -        goto bad_reg;
> +        /* Interrupt Clear Active.  */
> +        irq = (offset - 0x380) * 8 + GIC_BASE_IRQ;
> +        if (irq >= s->num_irq) {
> +            goto bad_reg;
> +        }

These registers only exist for GICv2, so again a check
on s->revision is required.

You also need to implement reading of GICD_ICACTIVERn for
GICv2 -- the code in gic_dist_readb() only implements reads of
GICD_ISACTIVERn (which is all that GICv1 needed).

> +
> +        /* This register is banked per-cpu for PPIs */
> +        int cm = irq < GIC_INTERNAL ? (1 << cpu) : ALL_CPU_MASK;
> +
> +        for (i = 0; i < 8; i++) {
> +            if (s->security_extn && !attrs.secure &&
> +                !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
> +                continue; /* Ignore Non-secure access of Group0 IRQ */
> +            }
> +
> +            if (value & (1 << i)) {
> +                GIC_DIST_CLEAR_ACTIVE(irq + i, cm);
> +            }
> +        }
>      } else if (offset < 0x800) {
>          /* Interrupt Priority.  */
>          irq = (offset - 0x400) + GIC_BASE_IRQ;
> --
> 2.17.1

Looks good otherwise.

thanks
-- PMM