Re: [Qemu-devel] [PATCH 8/8] openpic: Fix problem when IRQ transitions from edge to level

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4634 bytes --]

On Sun, Sep 17, 2017 at 06:15:48PM +0100, Mark Cave-Ayland wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Some interrupts get triggered before the OS has setup the
> right interrupt type. If an edge interrupt is latched that
> way, not delivered (still masked), then the interrupt is
> changed to level and isn't asserted anymore, it will be
> stuck "pending", causing an interrupt flood. This can happen
> with the PMU GPIO interrupt for example.
> 
> There are a few other corner cases like this, so let's keep
> track of the input "level" so we can properly re-evaluate
> when the trigger type changes.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/intc/openpic.c |   32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index debfcbf..34749f8 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -236,6 +236,7 @@ typedef struct IRQSource {
>      int last_cpu;
>      int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
>      int pending;    /* TRUE if IRQ is pending */
> +    int cur_level;  /* Cache current level */
>      IRQType type;
>      bool level:1;   /* level-triggered */
>      bool nomask:1;  /* critical interrupts ignore mask on some FSL MPICs */
> @@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level)
>      }
>  
>      src = &opp->src[n_IRQ];
> -    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n",
> -            n_IRQ, level, src->ivpr);
> +    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n",
> +            n_IRQ, level, src->ivpr, src->level, src->cur_level);
> +
> +    /* Keep track of the current input level in order to properly deal
> +     * with the configuration of the source changing from edge to level
> +     * after it's been latched. Otherwise the interrupt can get stuck.
> +     */
> +    src->cur_level = level;
> +
>      if (src->level) {
> -        /* level-sensitive irq */
>          src->pending = level;
>          openpic_update_irq(opp, n_IRQ);
>      } else {
> -        /* edge-sensitive irq */
> +        /* edge-sensitive irq
> +         *
> +         * In an ideal world we would only set pending on an "edge", ie
> +         * if level is set and src->cur_level as clear. However that would
> +         * require all the devices to properly send "pulses" rather than
> +         * just "raise" which isn't the case today.
> +         */
>          if (level) {
>              src->pending = 1;
>              openpic_update_irq(opp, n_IRQ);
> @@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
>      switch (opp->src[n_IRQ].type) {
>      case IRQ_TYPE_NORMAL:
>          opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK);
> +
> +        /* If we switched to level we need to re-evaluate "pending" based
> +         * on the actual input state.
> +         */
> +        if (opp->src[n_IRQ].level) {
> +            opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level;
> +        }
>          break;
>  
>      case IRQ_TYPE_FSLINT:
> @@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
>          break;
>      }
>  
> +    /* Re-evaluate a level irq */
>      openpic_update_irq(opp, n_IRQ);
>      DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
>              opp->src[n_IRQ].ivpr);
> @@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
>      }
>  
>      if (!src->level) {
> -        /* edge-sensitive IRQ */
> +        /* edge-sensitive IRQ or level dropped */
>          src->ivpr &= ~IVPR_ACTIVITY_MASK;
>          src->pending = 0;
>          IRQ_resetbit(&dst->raised, irq);
> @@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = {
>          VMSTATE_UINT32(destmask, IRQSource),
>          VMSTATE_INT32(last_cpu, IRQSource),
>          VMSTATE_INT32(pending, IRQSource),
> +        VMSTATE_INT32(cur_level, IRQSource),
>          VMSTATE_END_OF_LIST()

This alters the migration stream without bumping the version number.
I suspect it will be best to move this hunk into your other patch
updating the migration of the openpic.

>      }
>  };

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]