Re: [Qemu-devel] [PATCH v4 2/9] ppc/xics: Fix migration failure with kernel-irqchip=off

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

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

On Mon, Sep 19, 2016 at 11:59:30AM +0530, Nikunj A Dadhania wrote:
> With a single cpu VM running with kernel-irqchip=off and a flood ping
> running in the guest. Migration fails once in few times.
> 
> Found that whenever there is an interrupt (in this case lsi int 3 from
> e1000), we raise an interrupt using qemu_irq_pulse() and also see that
> the kvm ioctl is complete.

Uh.. for an lsi qemu_irq_pulse() should not ever be used - that should
only be used for edge or message interrupts.

> 67351@1468011062.810020:xics_set_irq_lsi set_irq_lsi: srcno 3 [irq 0x1003]
> 67351@1468011062.810031:xics_icp_irq cpu 0 trying to deliver irq 0x1003 priority 0x5
> 67351@1468011062.810038:xics_icp_raise raising IRQ new XIRR=0xff001003
> new pending priority=0x5
> 
> After migration on the target side, interrupts(prio 0x5) are rejected as
> there is a interrupt pending (pending_priority 0x5). Moreover, we never
> get an icp_accept from the guest, so it hangs and crashes.

Sorry, I'm still having a lot of trouble following this description of
what's happening.

> Basically, resend the irq pulse(lsi) to the guest.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/intc/xics.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 69162f0..f765b08 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -209,7 +209,7 @@ static const TypeInfo xics_common_info = {
>  #define CPPR(ss)   (((ss)->xirr) >> 24)
>  
>  static void ics_reject(ICSState *ics, int nr);
> -static void ics_resend(ICSState *ics);
> +static void ics_resend(ICSState *ics, int server);
>  static void ics_eoi(ICSState *ics, int nr);
>  
>  static void icp_check_ipi(XICSState *xics, int server)
> @@ -238,7 +238,7 @@ static void icp_resend(XICSState *xics, int server)
>      if (ss->mfrr < CPPR(ss)) {
>          icp_check_ipi(xics, server);
>      }
> -    ics_resend(xics->ics);
> +    ics_resend(xics->ics, server);
>  }
>  
>  void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
> @@ -512,13 +512,24 @@ static void ics_reject(ICSState *ics, int nr)
>      }
>  }
>  
> -static void ics_resend(ICSState *ics)
> +static void ics_resend(ICSState *ics, int server)
>  {
>      int i;
> +    ICPState *ss = ics->xics->ss + server;
> +    ICSIRQState *irq;
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
>          /* FIXME: filter by server#? */
> -        if (ics->irqs[i].flags & XICS_FLAGS_IRQ_LSI) {
> +        irq = &ics->irqs[i];
> +        if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) {
> +            continue;
> +        }
> +
> +        if (irq->flags & XICS_FLAGS_IRQ_LSI) {
> +            if (irq->status & XICS_STATUS_SENT) {
> +                qemu_irq_raise(ss->output);
> +                continue;

Directly reraising the CPU irq line from an ics function rather than
an icp function seems very dubious.  It really seems like instead we
need to be recalculating the output line state from the ICP state,
after we've done all the ICS resends.

> +            }
>              resend_lsi(ics, i);
>          } else {
>              resend_msi(ics, i);

-- 
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: 819 bytes --]