Re: [Qemu-devel] [PATCH v2 16/16] hw/intc/arm_gic: add gic_update() for grouping

[Christoffer Dall]

On Mon, Nov 10, 2014 at 3:43 PM, Greg Bellows <address@hidden> wrote:
[...]

> On 7 November 2014 06:44, Daniel Thompson <address@hidden>
> wrote:
>>
>> On 30/10/14 22:12, Greg Bellows wrote:
>> > From: Fabian Aggeler <address@hidden>
>> >
>> > GICs with grouping (GICv2 or GICv1 with Security Extensions) have a
>> > different exception generation model which is more complicated than
>> > without interrupt grouping. We add a new function to handle this model.
>> >
>> > Signed-off-by: Fabian Aggeler <address@hidden>
>> >
>> > ---
>> >
>> > v1 -> v2
>> > - Fix issue in gic_update_with_grouping() using the wrong combination of
>> >   flag and CPU control bank for checking if group 1 interrupts are
>> > enabled.
>> > ---
>> >  hw/intc/arm_gic.c      | 87
>> > +++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  hw/intc/gic_internal.h |  1 +
>> >  2 files changed, 87 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> > index 808aa18..e33c470 100644
>> > --- a/hw/intc/arm_gic.c
>> > +++ b/hw/intc/arm_gic.c
>> > @@ -52,6 +52,87 @@ static inline bool ns_access(void)
>> >      return true;
>> >  }
>> >
>> > +inline void gic_update_with_grouping(GICState *s)
>> > +{
>> > +    int best_irq;
>> > +    int best_prio;
>> > +    int irq;
>> > +    int irq_level;
>> > +    int fiq_level;
>> > +    int cpu;
>> > +    int cm;
>> > +    bool next_int;
>> > +    bool next_grp0;
>> > +    bool gicc_grp0_enabled;
>> > +    bool gicc_grp1_enabled;
>> > +
>> > +    for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
>> > +        cm = 1 << cpu;
>> > +        gicc_grp0_enabled = s->cpu_control[cpu][0] &
>> > GICC_CTLR_S_EN_GRP0;
>> > +        gicc_grp1_enabled = s->cpu_control[cpu][1] &
>> > GICC_CTLR_NS_EN_GRP1;
>> > +        next_int = 0;
>> > +        next_grp0 = 0;
>> > +
>> > +        s->current_pending[cpu] = 1023;
>> > +        if ((!s->enabled_grp[0] && !s->enabled_grp[1])
>> > +                || (!gicc_grp0_enabled && !gicc_grp1_enabled)) {
>> > +            qemu_irq_lower(s->parent_irq[cpu]);
>> > +            qemu_irq_lower(s->parent_fiq[cpu]);
>> > +            return;
>>
>> Shouldn't that be continue? Otherwise the state of CPU[N-1] influences
>> whether interrupts can be delivered to CPU[N].
>>
>>

> I see what you are saying, but historically the code looks like it has
> always returned so I'd have to investigate it more as I am still learning
> the code myself.  If this is a regression, it would be one inherited from
> the previous gic_update() function.

This does look like an existing bug, and I agree it should be continue
(you could optimize the disable IRQ state by using return if the
distributor disable all IRQs, but the code complexity cost is probably
not worth the optimization for the unusual situation.

Thanks,
-Christoffer