qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 13/16] hw/intc/arm_gic: Change behavior of IA


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v2 13/16] hw/intc/arm_gic: Change behavior of IAR writes
Date: Tue, 14 Apr 2015 20:31:36 +0100

On 30 October 2014 at 22:12, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Grouping (GICv2) and Security Extensions change the behavior of IAR
> reads. Acknowledging Group0 interrupts is only allowed from Secure
> state and acknowledging Group1 interrupts from Secure state is only
> allowed if AckCtl bit is set.

Subject says "IAR writes" but it means "IAR reads".

>
> Signed-off-by: Fabian Aggeler <address@hidden>
>
> ---
>
> v1 -> v2
> - Fix issue in gic_acknowledge_irq() where the GICC_CTLR_S_ACK_CTL flag is
>   applied without first checking whether the read is secure or non-secure.
>   Secure reads of IAR when AckCtl is 0 return a spurious ID of 1022, but
>   non-secure ignores the flag.
> ---
>  hw/intc/arm_gic.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 2d83225..7eb72df 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -190,11 +190,36 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
>      int ret, irq, src;
>      int cm = 1 << cpu;
>      irq = s->current_pending[cpu];
> +    bool isGrp0;
>      if (irq == 1023
>              || GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
>          DPRINTF("ACK no pending IRQ\n");
>          return 1023;
>      }
> +
> +    if (s->revision >= 2 || s->security_extn) {
> +        isGrp0 = GIC_TEST_GROUP0(irq, (1 << cpu));
> +        if ((isGrp0 && (!s->enabled_grp[0]
> +                    || !(s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0)))
> +           || (!isGrp0 && (!s->enabled_grp[1]
> +                    || !(s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1)))) {
> +            return 1023;
> +        }
> +
> +        if ((s->revision >= 2 && !s->security_extn)
> +                || (s->security_extn && !ns_access())) {
> +            if (!isGrp0 && !ns_access() &&
> +                !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
> +                DPRINTF("Read of IAR ignored for Group1 interrupt %d "
> +                        "(AckCtl disabled)\n", irq);
> +                return 1022;
> +            }
> +        } else if (s->security_extn && ns_access() && isGrp0) {
> +            DPRINTF("Non-secure read of IAR ignored for Group0 interrupt 
> %d\n",
> +                    irq);
> +            return 1023;
> +        }
> +    }

This doesn't quite line up with the pseudocode in the GIC spec.
It's probably going to be easier to read with some utility functions
for 'grouping enabled' etc.

>      s->last_active[irq][cpu] = s->running_irq[cpu];
>
>      if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> --
> 1.8.3.2
>

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]