Re: [Qemu-devel] [RFC PATCH v2 3/6] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 14 October 2013 16:36, Peter Maydell <address@hidden> wrote:
> Are you sure the logic in this function is right? (ie that we
> should only clear the sgi_source[][] bit for this source, and
> not completely? If nothing else, I think the interrupt should
> stay pending if some other source cpu still wants it to be
> pending. That is, I think we need to track the pending status
> separately for each (irq,target-cpu,source-cpu) separately for
> SGIs. (I'm not totally sure I have this right though, the spec
> is quite confusing.)

Having spoken to Marc I'm now a bit less confused. For
SGIs:
 * there is effectively a pending bit for each
   (irq-number, source-cpu, target-cpu) tuple
 * for a v2 GIC these are guest-visible in GICD_CPENDSGIRn
   and GICD_SPENDSGIRn; for a pre-v2 GIC the state still
   exists, it's just not guest-visible
 * the overall "pending state" bit in GICD_ISPENDRn and
   GICD_ICPENDRn is effectively the logical OR of the
   pending bits for each source-cpu; writes to this bit
   are ignored
 * when the guest reads from GICC_IAR, if the SGI is
   pending for multiple source-cpus for this target-cpu
   then we clear the pending bit for the (int, src, tgt)
   tuple in question, but it will still be set for the
   other (int,src,tgt) tuples for the other source-cpus
   (and so the GICD_ISPENDRn bit will still be set)

Tangentially, I notice that we don't correctly handle
the PENDING bit for level triggered interrupts, since
we do:

    /* Clear pending flags for both level and edge triggered interrupts.
       Level triggered IRQs will be reasserted once they become inactive.  */
    gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
                                  GIC_SGI_SRC(new_irq, cpu));

in gic_acknowledge_irq(). This is wrong, because section
3.2.4 is clear for a level triggered interrupt that if the
interrupt signal remains asserted (which it usually will be)
then we go from Pending to Active+Pending (whereas our
current implementation goes from Pending to Active and
then resets Pending later in gic_complete_irq()).

-- PMM